Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Don't deselect annotations on pan/zoom, even if the annotation moves out of the visible bounds #8022

Merged
merged 1 commit into from
Feb 11, 2017
Merged

Don't deselect annotations on pan/zoom, even if the annotation moves out of the visible bounds #8022

merged 1 commit into from
Feb 11, 2017

Conversation

Ben305
Copy link
Contributor

@Ben305 Ben305 commented Feb 10, 2017 •
edited
Loading

This fixes #8021

Don't deselect annotations on pan/zoom, even if the annotation moves out of the visible bounds. Now MapBox behaves like Google Maps and MapKit.

@mention-bot
Copy link

@Ben305, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @boundsj to be potential reviewers.

@Ben305 Ben305 force-pushed the 8021-deselection-on-region-change branch from 4bd3886 to 28ae886 Compare February 10, 2017 21:24
@1ec5
Copy link
Contributor

1ec5 commented Feb 10, 2017

Thanks for this PR. Based on #8021 (comment), I think we could get away with a fix that doesn’t add any additional options to the API, but I’m curious whether that would address your use case.

@1ec5 1ec5 added annotations Annotations on iOS and macOS or markers on Android iOS Mapbox Maps SDK for iOS needs discussion labels Feb 10, 2017
@Ben305 Ben305 force-pushed the 8021-deselection-on-region-change branch from 28ae886 to 0fb4c3e Compare February 11, 2017 13:21
@Ben305 Ben305 changed the title Add property deselectOnRegionChange to MGLAnnotationView Add property deselectWhenOutOfBounds to MGLMapView Feb 11, 2017
frederoni
frederoni reviewed Feb 11, 2017
View reviewed changes
platform/ios/src/MGLMapView.mm Outdated
@@ -4729,7 +4736,7 @@ - (void)notifyMapChange:(mbgl::MapChange)change
rect = annotationView.frame;
}

if ( ! CGRectIntersectsRect(rect, self.frame)) {
if ( ! CGRectIntersectsRect(rect, self.frame) && self.deselectWhenOutOfBounds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced this property is needed. I would prefer to just remove these lines unless I can see a use case where deselect when out of bounds should be the default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to remove this, I just added it because I thought that @1ec5 was not fine with removing the current way how deselection is handled completely.

Copy link
Contributor

@1ec5 1ec5 Feb 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Given #8021 (comment), I’ve changed my mind; if MapKit and Google Maps avoid deselecting the annotation, then that’s what developers expect, and an extra option is unnecessary.

Are there any implications for callout views? Does any of the callout code assume that a callout view (if it exists) is always visible as long as its annotation is selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I have updated my PR to that annotations are not deselected any longer. If the annotation has a callout view which responds to dismissAutomatically, the annotation is deselected if that function returns true.

@1ec5 1ec5 added Google Maps parity For feature parity with the Google Maps SDK for Android or iOS MapKit parity For feature parity with MapKit on iOS or macOS labels Feb 11, 2017
@Ben305 Ben305 force-pushed the 8021-deselection-on-region-change branch from 0fb4c3e to 27838ff Compare February 11, 2017 19:18
@Ben305 Ben305 changed the title Add property deselectWhenOutOfBounds to MGLMapView Don't deselect annotations on pan/zoom, even if the annotation moves out of the visible bounds Feb 11, 2017
1ec5
1ec5 approved these changes Feb 11, 2017
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Thanks! Would you do the honors and update the iOS changelog to mention this change?

@Ben305
[ios] Do not deselect annotations any longer when map is zoomed/panne…
a78b784 
…d or annotation is moved out of the visible bounds
@Ben305 Ben305 force-pushed the 8021-deselection-on-region-change branch from 27838ff to a78b784 Compare February 11, 2017 19:32
@Ben305
Copy link
Contributor Author

Ben305 commented Feb 11, 2017

Happy to do that. Done ;-)

@1ec5 1ec5 merged commit b9be66f into mapbox:master Feb 11, 2017
@frederoni frederoni mentioned this pull request Feb 14, 2017
Closed
@1ec5 1ec5 mentioned this pull request Mar 10, 2017
Closed
@captainbarbosa captainbarbosa mentioned this pull request Dec 20, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@frederoni frederoni frederoni left review comments

@1ec5 1ec5 1ec5 approved these changes

Assignees
No one assigned
Labels
annotations Annotations on iOS and macOS or markers on Android Google Maps parity For feature parity with the Google Maps SDK for Android or iOS iOS Mapbox Maps SDK for iOS MapKit parity For feature parity with MapKit on iOS or macOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Callout-less annotation is deselected when user pans or zooms the map
4 participants
@Ben305 @mention-bot @1ec5 @frederoni