-
-
Notifications
You must be signed in to change notification settings - Fork 213
Fix issue were pins stay on the map when you have maps on multiple pages #450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a pre-condition in AddPins to clear existing annotations on iOS by calling RemoveAnnotations(PlatformView.Annotations) before iterating and adding new pins. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant CustomMapHandler
participant PlatformView as MKMapView (iOS)
Caller->>CustomMapHandler: AddPins(mapPins)
alt Existing annotations present
CustomMapHandler->>PlatformView: RemoveAnnotations(PlatformView.Annotations)
note right of PlatformView: Clears current markers
end
loop For each pin in mapPins
CustomMapHandler->>PlatformView: AddAnnotation(pin)
end
CustomMapHandler-->>Caller: Return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)✅ Copyable Unit Test edits generated.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MauiMaps/Platforms/iOS/CustomMapHandler.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
| if (PlatformView.Annotations.Length > 0) | ||
| { | ||
| PlatformView.RemoveAnnotations(PlatformView.Annotations); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filter annotation removal to preserve system annotations.
Removing all annotations indiscriminately may unintentionally clear system-managed annotations such as the user location marker. Additionally, the async image loading for CustomPin (lines 144-157) can introduce a race condition: if AddPins is called again before image callbacks complete, those pending annotations might be added after the new clear operation, leaving stale pins on the map.
Consider filtering the removal to only target annotations added by this handler:
-if (PlatformView.Annotations.Length > 0)
-{
- PlatformView.RemoveAnnotations(PlatformView.Annotations);
-}
+var annotationsToRemove = PlatformView.Annotations
+ .Where(a => a is CustomAnnotation || a is MKPointAnnotation)
+ .ToArray();
+if (annotationsToRemove.Length > 0)
+{
+ PlatformView.RemoveAnnotations(annotationsToRemove);
+}This preserves system annotations while still addressing the .NET 9 issue where pins persist across page navigations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (PlatformView.Annotations.Length > 0) | |
| { | |
| PlatformView.RemoveAnnotations(PlatformView.Annotations); | |
| } | |
| var annotationsToRemove = PlatformView.Annotations | |
| .Where(a => a is CustomAnnotation || a is MKPointAnnotation) | |
| .ToArray(); | |
| if (annotationsToRemove.Length > 0) | |
| { | |
| PlatformView.RemoveAnnotations(annotationsToRemove); | |
| } |
| return; | ||
| } | ||
|
|
||
| if (PlatformView.Annotations.Length > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomcurran could you please explain why we need to clear all annotations? I see we removed annotations on line 116.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this to be needed to get the correct behaviour. See the description for information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please provide example to reproduce the issue?
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @VladislavAntonyuk. * #450 (comment) The following files were modified: * `MauiMaps/Platforms/iOS/CustomMapHandler.cs`
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Caution The CodeRabbit agent failed during execution: Clone operation failed |
1 similar comment
|
Caution The CodeRabbit agent failed during execution: Clone operation failed |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Caution The CodeRabbit agent failed during execution: Clone operation failed |
Clears all the annotations prior to adding new ones. We found it was needed as of .NET 9. Related code. Mirrors the behaviour in MauiMKMapView.
Summary by CodeRabbit