Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
x:Class="Maui.Controls.Sample.Pages.MapsGalleries.CustomPinIconGallery"
Title="Custom Pin Icons">
<Grid RowDefinitions="Auto,*">
<StackLayout Orientation="Horizontal" Padding="10" Spacing="10">
<Button Text="Add Custom Pins" Clicked="OnAddCustomPinsClicked"/>
<Button Text="Add Default Pin" Clicked="OnAddDefaultPinClicked"/>
<Button Text="Clear" Clicked="OnClearClicked"/>
</StackLayout>

<FlexLayout Wrap="Wrap" Padding="10" AlignItems="Center">
<Button Text="Add Custom Pins" Clicked="OnAddCustomPinsClicked" Margin="5"/>
<Button Text="Add Default Pin" Clicked="OnAddDefaultPinClicked" Margin="5"/>
<Button Text="Toggle Icon" Clicked="OnToggleIconClicked" Margin="5"/>
<Button Text="Move &amp; Rename" Clicked="OnMoveRenameClicked" Margin="5"/>
<Button Text="Clear" Clicked="OnClearClicked" Margin="5"/>
</FlexLayout>

<maps:Map x:Name="CustomPinMap" Grid.Row="1"/>
</Grid>
</ContentPage>
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@ namespace Maui.Controls.Sample.Pages.MapsGalleries
{
public partial class CustomPinIconGallery : ContentPage
{
int _iconState;
int _moveCount;

public CustomPinIconGallery()
{
InitializeComponent();

// Center on Seattle
CustomPinMap.MoveToRegion(MapSpan.FromCenterAndRadius(
new Microsoft.Maui.Devices.Sensors.Location(47.6062, -122.3321),
Expand Down Expand Up @@ -54,6 +57,40 @@ void OnAddDefaultPinClicked(object? sender, EventArgs e)
CustomPinMap.Pins.Add(defaultPin);
}

// Swaps ImageSource on every existing pin at runtime, to test whether the marker icon
// refreshes live. Cycles custom A -> custom B -> null (platform default) so both the
// custom-to-custom swap and the null/non-null boundary transitions are exercised.
void OnToggleIconClicked(object? sender, EventArgs e)
{
_iconState = (_iconState + 1) % 3;
ImageSource? source = _iconState switch
{
0 => ImageSource.FromFile("dotnet_bot.png"),
1 => ImageSource.FromFile("coffee.png"),
_ => null,
};

foreach (var pin in CustomPinMap.Pins)
{
pin.ImageSource = source;
}
}

// Location and Label update live via their per-platform mappers; use this alongside Toggle Icon
// to confirm ImageSource now refreshes live at runtime too.
void OnMoveRenameClicked(object? sender, EventArgs e)
{
_moveCount++;

foreach (var pin in CustomPinMap.Pins)
{
pin.Location = new Microsoft.Maui.Devices.Sensors.Location(
pin.Location.Latitude + 0.01,
pin.Location.Longitude);
pin.Label = $"Moved x{_moveCount}";
}
}

void OnClearClicked(object? sender, EventArgs e)
{
CustomPinMap.Pins.Clear();
Expand Down
95 changes: 95 additions & 0 deletions src/Controls/tests/DeviceTests/Elements/Map/MapTests.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@
using System.Reflection;
using System.Threading.Tasks;
using MapKit;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Maps;
using Microsoft.Maui.Devices.Sensors;
using Microsoft.Maui.Hosting;
using Microsoft.Maui.Maps;
using Microsoft.Maui.Maps.Handlers;
using Microsoft.Maui.Maps.Platform;
using UIKit;
using Xunit;
using static Microsoft.Maui.DeviceTests.AssertHelpers;

namespace Microsoft.Maui.DeviceTests
{
Expand Down Expand Up @@ -94,5 +97,97 @@ await AttachAndRun<MapHandler>(map, async handler =>
}
});
}

[Fact(DisplayName = "Pin ImageSource Runtime Change Updates Annotation View")]
public async Task PinImageSourceRuntimeChangeUpdatesAnnotationView()
{
// Regression test for MauiMKMapView.UpdatePinImage: a Pin's ImageSource can change at
// runtime after the pin is already on the map. Crossing the null/non-null boundary must
// swap the annotation view type (MKMarkerAnnotationView <-> custom MKAnnotationView),
// and changing between two custom images must refresh the image in place.
SetupBuilder();

var location = new Location(47.6062, -122.3321);
var pin = new Pin
{
Label = "Test Pin",
Location = location,
};

var map = new Map
{
Pins = { pin }
};

await AttachAndRun<MapHandler>(map, async handler =>
{
await Task.Yield();

var platformView = handler.PlatformView;
Assert.NotNull(platformView);

// The harness attaches the platform view without sizing it, and MapKit only creates
// annotation views for a laid-out map, so give it a real frame first.
platformView.Frame = new CoreGraphics.CGRect(0, 0, 320, 480);

map.MoveToRegion(new MapSpan(location, 0.01, 0.01));

// 1. Pin renders with the default marker view (no ImageSource set).
MKAnnotationView GetCurrentView()
{
if (pin.MarkerId is not IMKAnnotation a)
return null;

return platformView.ViewForAnnotation(a);
}

await AssertEventually(
() => GetCurrentView() is not null,
timeout: 5000,
message: "Timed out waiting for the pin's annotation view to be created.");

var initialView = GetCurrentView();
Assert.True(initialView is MKMarkerAnnotationView or MKPinAnnotationView,
$"Expected a default marker view, got {initialView?.GetType().Name ?? "null"}.");

// 2. null -> custom: switching to a custom image swaps in a plain MKAnnotationView
// showing that image (the annotation is removed/re-added, so re-resolve it each poll).
pin.ImageSource = ImageSource.FromFile("red.png");

await AssertEventually(
() => GetCurrentView() is MKAnnotationView v
&& v is not MKMarkerAnnotationView
&& v is not MKPinAnnotationView
&& v.Image is not null,
timeout: 5000,
message: "Timed out waiting for the pin's annotation view to switch to a custom image view.");

// 3. custom -> custom: changing to another custom image refreshes the image in place,
// the view stays a custom (non-marker) view.
var previousImage = GetCurrentView().Image;
pin.ImageSource = ImageSource.FromFile("black.png");

await AssertEventually(
() =>
{
var view = GetCurrentView();
return view is not null
&& view is not MKMarkerAnnotationView
&& view is not MKPinAnnotationView
&& view.Image is not null
&& !ReferenceEquals(view.Image, previousImage);
},
timeout: 5000,
message: "Timed out waiting for the pin's custom image to be updated to the new image.");

// 4. custom -> null: clearing the ImageSource reverts the pin to the default marker view.
pin.ImageSource = null;

await AssertEventually(
() => GetCurrentView() is MKMarkerAnnotationView or MKPinAnnotationView,
timeout: 5000,
message: "Timed out waiting for the pin's annotation view to revert to the default marker view.");
});
}
}
}
9 changes: 6 additions & 3 deletions src/Core/maps/src/Handlers/MapPin/MapPinHandler.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using CoreLocation;
using MapKit;
using Microsoft.Maui.Handlers;
using Microsoft.Maui.Maps.Platform;

namespace Microsoft.Maui.Maps.Handlers
{
Expand All @@ -27,11 +28,13 @@ public static void MapAddress(IMapPinHandler handler, IMapPin mapPin)
mKPointAnnotation.Subtitle = mapPin.Address;
}

// Note: ImageSource is handled in MauiMKMapView.GetViewForAnnotation
// because the image is set on the MKAnnotationView, not on the IMKAnnotation
// The initial image is applied in MauiMKMapView.GetViewForAnnotation (set on the
// MKAnnotationView, not the IMKAnnotation). This handles runtime ImageSource changes by
// refreshing the annotation view of a pin that is already on the map.
public static void MapImageSource(IMapPinHandler handler, IMapPin mapPin)
{
// No-op: Image is applied when the annotation view is created in GetViewForAnnotation
if (mapPin.Parent?.Handler?.PlatformView is MauiMKMapView mapView)
mapView.UpdatePinImage(mapPin);
}
}
}
66 changes: 62 additions & 4 deletions src/Core/maps/src/Platform/iOS/MauiMKMapView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public class MauiMKMapView : MKMapView
object? _lastTouchedView;
UITapGestureRecognizer? _mapClickedGestureRecognizer;
bool _isClusteringEnabled;
IMKAnnotation? _suppressClickForAnnotation;

UILongPressGestureRecognizer? _mapLongClickedGestureRecognizer;
List<IMapElement>? _trackedMapElements;
Expand Down Expand Up @@ -344,21 +345,64 @@ void ZoomToShowClusterPins(IMKAnnotation[] annotations)
SetVisibleMapRect(paddedRect, true);
}

// Refreshes a pin already on the map after its ImageSource changed at runtime. Pins with no
// individual view (off-screen or collapsed into a cluster) are skipped; GetViewForAnnotation
// applies the current ImageSource when they appear.
internal void UpdatePinImage(IMapPin pin)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Regression PreventionUpdatePinImage introduces the runtime ImageSource refresh path for iOS/MacCatalyst but no automated device test covers it. The three key transitions—custom→custom swap, custom→null (revert to platform default), and null→custom (first assignment after pin is already on map)—are exercised only by the new manual gallery buttons. A device test that adds a pin, changes its ImageSource through each boundary, and asserts the annotation view type via ViewForAnnotation would close this regression gap (the pre-flight review flagged this gap independently).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 09934f1: a device test in MapTests.iOS.cs ("Pin ImageSource Runtime Change Updates Annotation View") covering the three transitions on a pin already on the map — null→custom asserts the view swaps to a custom MKAnnotationView with a loaded image, custom→custom asserts the image instance changes in place, custom→null asserts the view reverts to MKMarkerAnnotationView. The annotation is re-resolved on every poll since boundary transitions remove/re-add it, and the platform view is given an explicit frame because the test harness attaches it unsized and MapKit only materializes annotation views for a laid-out map.

{
if (pin.MarkerId is not IMKAnnotation annotation || ViewForAnnotation(annotation) is not MKAnnotationView view)
return;

bool hasCustomImage = pin.ImageSource is not null;
bool viewShowsCustomImage = view is not MKMarkerAnnotationView && view is not MKPinAnnotationView;

if (hasCustomImage == viewShowsCustomImage)
{
// The current view type still matches; refresh the image in place. Don't pre-clear the
// custom image: ApplyCustomImageAsync only assigns on success, so a failed load keeps the
// previous icon (matching Android) instead of blanking a visible pin.
if (hasCustomImage)
ApplyCustomImageAsync(view, pin).FireAndForget();
else
view.Image = null;
return;
}

// ImageSource crossed the null/non-null boundary: custom-image pins and default pins use
// different annotation view types, so re-add the annotation to let GetViewForAnnotation
// recreate the view through the standard path. Restore selection without raising a
// synthetic PinClicked.
bool wasSelected = SelectedAnnotations?.Any(a => ReferenceEquals(a, annotation) || a.Handle == annotation.Handle) == true;
RemoveAnnotation(annotation);
AddAnnotation(annotation);

if (wasSelected)
{
// DidSelectAnnotationView may fire after the view is (re)created rather than inside
// SelectAnnotation, so suppression is annotation-matched rather than a flag scoped to
// this call; it's consumed by the matching event, or dropped in Cleanup.
_suppressClickForAnnotation = annotation;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

⚠️ [moderate] iOS/MacCatalyst Lifecycle — This suppression state can survive a detach/reattach if SelectAnnotation does not deliver DidSelectAnnotationView before the map view is removed from the window. MauiMKMapView instances keep their annotations across Cleanup()/Startup(), so the next real tap on that same annotation after navigation/Shell tab switching can be swallowed as a synthetic selection. Clear _suppressClickForAnnotation during Cleanup (or before reconnect) so one-shot state cannot leak across lifecycle transitions.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6ce52cf. Cleanup() now sets _suppressClickForAnnotation = null, so the one-shot state can't survive a detach/reattach (navigation, Shell tab switch) and swallow the next real tap on that annotation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness_suppressClickForAnnotation is set and relies on DidSelectAnnotationView firing for the programmatic SelectAnnotation(annotation, false) call to consume it. If MapKit defers or drops that event—e.g. the annotation view has not yet been recreated when SelectAnnotation is called (because GetViewForAnnotation runs asynchronously after AddAnnotation), or on MacCatalyst where the delegate-firing contract differs—the suppressor stays set until Cleanup(). The next genuine user tap then matches _suppressClickForAnnotation and silently swallows the PinClicked event. Consider clearing _suppressClickForAnnotation inside GetViewForAnnotation once the view is created for the matching annotation, giving a tighter safety net than waiting for the selection event or a full window detach.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not applying the suggested placement: clearing _suppressClickForAnnotation inside GetViewForAnnotation would run when the view is (re)created — which is before the deferred DidSelectAnnotationView for the programmatic restore is delivered. The suppressor would be gone by the time that event arrives, re-raising the synthetic PinClicked this mechanism exists to prevent (the exact bug fixed after the round-3 finding). The residual risk being described — MapKit never delivering the matching didSelect — is real but bounded: the suppressor is annotation-matched and one-shot, so at worst one tap on that specific pin is swallowed, after which it is consumed and cleared (and Cleanup() clears it on detach). Whether MapKit can actually drop that delivery (e.g. re-added annotation absorbed into a cluster) is native timing behavior we can't settle statically; tightening the window further needs on-device verification, which I'd rather do as a follow-up than guess at here.

SelectAnnotation(annotation, false);
}
}

async System.Threading.Tasks.Task ApplyCustomImageAsync(MKAnnotationView annotationView, IMapPin pin)
{
_handlerRef.TryGetTarget(out IMapHandler? handler);
if (handler?.MauiContext == null || pin.ImageSource == null)
return;

// Capture the annotation before the async operation to detect reuse
// Capture the annotation and requested source before the async operation, to detect both
// view reuse and a newer ImageSource change that started while this load was in flight.
var targetAnnotation = annotationView.Annotation;
var requestedSource = pin.ImageSource;

try
{
using var result = await pin.ImageSource.GetPlatformImageAsync(handler.MauiContext);
using var result = await requestedSource.GetPlatformImageAsync(handler.MauiContext);

// Verify the annotation view hasn't been reused for a different pin
if (annotationView.Annotation != targetAnnotation)
// Drop this load if the view was disposed/released, reused, or the pin's ImageSource changed since.
if (annotationView.Handle == IntPtr.Zero || annotationView.Annotation != targetAnnotation || !ReferenceEquals(pin.ImageSource, requestedSource))
return;

if (result?.Value is UIImage image)
Expand Down Expand Up @@ -628,6 +672,10 @@ void Cleanup()
RegionChanged -= MkMapViewOnRegionChanged;
DidSelectAnnotationView -= MkMapViewOnAnnotationViewSelected;
DidUpdateUserLocation -= MkMapViewOnUserLocationUpdated;

// Annotations survive detach/reattach, so drop any pending click suppression to prevent it
// from swallowing the next real tap after navigation or a Shell tab switch.
_suppressClickForAnnotation = null;
_clusterIconCache.Clear();
_clusterImageOwner = null;
_clusterImageVersion = int.MinValue;
Expand All @@ -652,6 +700,16 @@ void MkMapViewOnAnnotationViewSelected(object? sender, MKAnnotationViewEventArgs
{
var annotation = e.View.Annotation;

// Selection was restored programmatically by UpdatePinImage; the user did not tap the pin.
// Only consume (and clear) the suppressor when this event is for the matching annotation, so
// a user tap on a different pin while the restore is pending isn't swallowed by mistake.
if (annotation is not null && _suppressClickForAnnotation is not null &&
(ReferenceEquals(annotation, _suppressClickForAnnotation) || annotation.Handle == _suppressClickForAnnotation.Handle))
{
_suppressClickForAnnotation = null;
return;
}

// Handle cluster annotation selection
if (annotation is not null && TryGetClusterAnnotation(annotation) is MKClusterAnnotation clusterAnnotation)
{
Expand Down
Loading