Optional positioning and stickiness for callout view #7646
Conversation
frederoni
added
annotations
|
@frederoni, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @boundsj to be potential reviewers. |
frederoni
force-pushed
the
fred-callout-view-improvements-7483
branch
from
4004ca1 to
c358bc6
Compare
| A Boolean value indicating whether the callout view should be positioned automatically. | ||
| The exact position can be adjusted by overriding -[UIView setCenter:] | ||
| */ | ||
| @property (nonatomic, assign) BOOL automaticPositioning; |
There was a problem hiding this comment.
A Boolean-typed property shouldn’t be named with a noun phrase. How about anchoredToAnnotation, with a getter isAnchoredToAnnotation?
| A Boolean value indicating whether the callout view should be dismissed or not | ||
| when the map view changes viewport. | ||
| */ | ||
| @property (nonatomic, assign) BOOL staysOpen; |
There was a problem hiding this comment.
Can we reverse the logic and call it dismissesAutomatically? I think that would make the behavior clearer, as evidenced by the description you wrote for this property.
| A Boolean value indicating whether the callout view should be positioned automatically. | ||
| The exact position can be adjusted by overriding -[UIView setCenter:] | ||
| */ | ||
| @property (nonatomic, assign) BOOL automaticPositioning; |
There was a problem hiding this comment.
Positioning automatically can mean many things. I think the fundamental difference between MGLCompactCalloutView and the custom callout view that ran into this bug is that MGLCompactCalloutView is relatively positioned, while that custom callout view is absolutely positioned – kind of like MGLRotationAlignmentMap versus MGLRotationAlignmentViewport. So another possibility would be to call this property relativelyPositioned.
There was a problem hiding this comment.
The words “automatic” or “relative” are too ambiguous here, for me. Of @1ec5’s suggestions, I like anchoredToAnnotation, because it makes this property’s action on the callout’s position and the annotation clear.
| A Boolean value indicating whether the callout view should be positioned automatically. | ||
| The exact position can be adjusted by overriding -[UIView setCenter:] | ||
| */ | ||
| @property (nonatomic, assign) BOOL automaticPositioning; |
There was a problem hiding this comment.
Mark these two properties as optional; adding a required member to a public protocol is a backwards-incompatible change. We’ll have to figure out what to do if the properties aren’t implemented.
| A Boolean value indicating whether the callout view should be positioned automatically. | ||
| The exact position can be adjusted by overriding -[UIView setCenter:] | ||
| */ | ||
| @property (nonatomic, assign) BOOL automaticPositioning; |
There was a problem hiding this comment.
Mark these properties as readonly, because MGLMapView doesn’t care if the class implements -setAutomaticPositioning: – it’ll never call that setter anyways.
frederoni
force-pushed
the
fred-callout-view-improvements-7483
branch
from
f48c2a7 to
15df576
Compare
|
|
||
| /** | ||
| A Boolean value indicating whether the callout view should be dismissed automatically | ||
| when the map view changes viewport. Note that a single tap on the map view |
There was a problem hiding this comment.
s/when the map view changes viewport/when the map view’s viewport changes/
| /** | ||
| A Boolean value indicating whether the callout view should be dismissed automatically | ||
| when the map view changes viewport. Note that a single tap on the map view | ||
| still dismisses the callout view. |
There was a problem hiding this comment.
…regardless of the value of this property.
| /** | ||
| A Boolean value indicating whether the callout view should be anchored to | ||
| the corresponding annotation. The exact position can be adjusted by | ||
| overriding -[UIView setCenter:] |
There was a problem hiding this comment.
What happens if this property (and the other one) is omitted?
There was a problem hiding this comment.
Good question. As of 46dd0a6 , it defaults to dismissesAutomatically.
There was a problem hiding this comment.
Can you point out the default behavior in the documentation for both properties (so developers know whether they need to implement them or not)?
| /** | ||
| A Boolean value indicating whether the callout view should be anchored to | ||
| the corresponding annotation. The exact position can be adjusted by | ||
| overriding -[UIView setCenter:] |
There was a problem hiding this comment.
Nit: avoid passive voice:
You can adjust the callout view’s precise location by overriding
-[UIView setCenter:].
| @synthesize anchoredToAnnotation = _anchoredToAnnotation; | ||
| @synthesize dismissesAutomatically = _dismissesAutomatically; | ||
|
|
||
| - (instancetype)init { |
There was a problem hiding this comment.
-init is not the designated initializer for UIView instances; that’d be either -initWithFrame: or -initWithCoder:. This class’s +platformCalloutView implementation happens to call -init, but this is a bit of an antipattern.
Instead of maintaining ivars for read-only variables, implement -anchoredToAnnotation and -dismissesAutomatically to return YES and NO, respectively.
There was a problem hiding this comment.
Good to go as soon as the documentation requested in #7646 (comment) is added.
frederoni
force-pushed
the
fred-callout-view-improvements-7483
branch
from
7462995 to
a59a382
Compare
|
Let’s also update the changelog entry for this feature/fix. Currently, it’s:
|
|
The changelog entry is correct: in v3.3.7, panning incorrectly dismissed the callout view; in v3.4.0, the callout view will stay open. We can update the beta release notes to note this fix, however. |
This proposal makes the
MGLCalloutViewa bit more versatile by adding ananchoredToAnnotationand adismissesAutomaticallyproperty. It also fixes issues like #7483 by falling back to the same behavior as in 3.4.0 beta 4 by default./cc @friedbunny @1ec5 @boundsj