-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
frederoni
commented
Jan 8, 2017
•
edited
Loading
edited
@frederoni, thanks for your PR! By analyzing this pull request, we identified @incanus, @1ec5 and @boundsj to be potential reviewers. |
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.
In addition to the comments below:
- Remember to add a blurb about the scale bar to the iOS changelog and about the distance formatter to both changelogs.
MKMapView exposes aAh, I see you’ve already noted that in the PR description.showsScale
property that allows the developer to keep the scale visible at all times, even when the map is at rest. At a glance, it seems like that would be straightforward to implement in this PR.
|
||
#import "MGLScaleBarView.h" | ||
|
||
@interface MGLScaleBarView (Private) |
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 we fold this into MGLScaleBarView.h, which already has Project visibility?
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 like @friedbunny's suggestion of exposing some style properties. Perhaps alternating colors, border color etc and then we'll need this private header.
toItem:self | ||
attribute:NSLayoutAttributeTop | ||
multiplier:1 | ||
constant:5+self.contentInset.top]]; |
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.
Do you think we should explicitly constrain the scale bar and compass to each other and the scale bar and logo to each other, the way we currently constrain the logo and attribution button to each other?
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 don't see any constraints between the logo and the attribution button.
The maximum width of the scale bar is screenWidth/2-padding
which might be a bit too wide, especially on iPads but I don't think the answer is to constrain it to the compass. It makes positioning the components with contentInsets
more prone to misplacement.
platform/ios/src/MGLMapView.mm
Outdated
@@ -1223,6 +1258,12 @@ - (void)handlePanGesture:(UIPanGestureRecognizer *)pan | |||
{ | |||
CGPoint offset = CGPointMake(velocity.x * self.decelerationRate / 4, velocity.y * self.decelerationRate / 4); | |||
_mbglMap->moveBy({ offset.x, offset.y }, MGLDurationInSeconds(self.decelerationRate)); | |||
__weak MGLMapView *weakSelf = self; |
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 we eliminate some of this repetitiveness by moving the -fadeOut
call to the MapChangeRegionDidChangeAnimated
handler?
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 tried that first but then it'll fade out while you zoom or pan if you don't move consistently. :/
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.
Good point. In that case, perhaps -notifyGestureDidEndWithDrift:
is what you want. If not, try factoring out the animation into a similar method.
platform/ios/src/MGLScaleBarView.m
Outdated
#import "MGLScaleBarView_Private.h" | ||
|
||
@interface MGLScaleBarView() | ||
@property (nonatomic, assign, getter=isVisible) BOOL visible; |
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.
This seems redundant to UIView.hidden
. Are the two orthogonal to each other somehow?
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.
hidden
would hide the view instantly so I use this property to flag the view as visible/invisible when fadeIn/fadeOut begins in order to prevent unwanted calls to those methods.
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.
In that case, CALayer.hidden
and CALayer.opacity
are both animatable.
platform/ios/src/MGLScaleBarView.m
Outdated
|
||
CGFloat barWidth = floorf(self.bounds.size.width / self.bars.count); | ||
|
||
[self.bars enumerateObjectsUsingBlock:^(UIView * _Nonnull bar, NSUInteger idx, BOOL * _Nonnull stop) { |
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.
Since you aren’t making use of functional programming, an ordinary for-in loop should be fine here.
platform/ios/src/MGLScaleBarView.m
Outdated
@property (nonatomic) CALayer *borderLayer; | ||
@end | ||
|
||
static const CGFloat BAR_HEIGHT = 4; |
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.
Use SCREAMING_SNAKE_CASE for macros and MGLUpperCamelCase for constants.
platform/ios/src/MGLScaleBarView.m
Outdated
- (void)setRow:(NSArray<NSNumber *> *)row { | ||
if (![row isEqualToArray:_row]) { | ||
[_bars makeObjectsPerformSelector:@selector(removeFromSuperview)]; | ||
[_labels makeObjectsPerformSelector:@selector(removeFromSuperview)]; |
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.
Would setting self.subviews
to the empty array also work here, or would that mess up constraints?
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.
-[UIView subviews]
is a readonly property
platform/ios/src/MGLScaleBarView.m
Outdated
NSMutableArray *labels = [NSMutableArray array]; | ||
for (NSUInteger i = 0; i <= self.row.lastObject.integerValue; i++) { | ||
UILabel *label = [[UILabel alloc] init]; | ||
label.text = @"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.
Is this a placeholder string, or does it need to be localized?
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.
Yep, this needs to be localized.
platform/ios/src/MGLScaleBarView.m
Outdated
|
||
_visible = NO; | ||
_primaryColor = [UIColor colorWithRed:62.0/255.0 green:62.0/255.0 blue:62.0/255.0 alpha:1]; | ||
_secondaryColor = [UIColor colorWithRed:247.0/255.0 green:247.0/255.0 blue:247.0/255.0 alpha:1]; |
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.
The text and border colors should switch between white-on-black and black-on-white, depending on the background color of the current map style — e.g., the satellite and dark styles would need colors that are inverted from the current black-on-white.
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.
This will never be perfect, even if the developer gets full control over the scale bar’s colors. Consider Mapbox Satellite: a scale bar that’s readable over a lush forest may be unreadable over tundra and vice versa.
Apple Maps and Google Maps solve this problem by treating the scale bar’s labels just like any other labels on the base map: in Standard mode, the labels are set in black with a subtle, white stroke for contrast:
In Satellite mode, the labels are inverted to white with a subtle, black stroke. Another nice touch in Apple Maps is that the scale bar has slight translucency in Satellite mode to avoid becoming distracting.
Apple Maps on macOS, Satellite
Our SDK has to cope with a wider variety of color schemes and challenges, since the style could contain anything. Here are a few possibilities:
- Keep the appearance as-is. The scale bar isn’t the most important ornament on the screen; a little squinting may not be a showstopper.
- Place a blurred background beneath the scale bar, as the macOS SDK does for attribution. Note that this effect would be similar but not identical to a visual effect view, whose effect is a bit too vibrant for a control that shouldn’t be conspicuous.
- Choose between a light and dark appearance based on the style’s background color, as @friedbunny suggests.
- Find the most similar symbol style layer and mimic its appearance. This could yield the best results – the highest fidelity to the style – but could also be somewhat unpredictable.
Whichever approach we take, I do think we should apply a stroke to the bar and labels at a minimum.
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.
Find the most similar symbol style layer and mimic its appearance.
Or even a way for the developer to specify which symbol style layer they want the scale bar and logo view to match?
platform/ios/src/MGLScaleBarView.h
Outdated
#import <UIKit/UIKit.h> | ||
|
||
@interface MGLScaleBarView : UIView | ||
|
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.
Should we expose any sort of styling properties publicly? Colors, typeface, position?
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 think knobs along these lines could be tail work. That way we can address customizability of the other ornaments at the same time in a consistent manner.
platform/ios/src/MGLScaleBarView.m
Outdated
} | ||
|
||
- (void)layoutBars { | ||
|
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.
Nit: there’s an inconsistent line break here.
platform/ios/src/MGLMapView.mm
Outdated
@@ -230,6 +231,8 @@ @interface MGLMapView () <UIGestureRecognizerDelegate, | |||
@property (nonatomic) EAGLContext *context; | |||
@property (nonatomic) GLKView *glView; | |||
@property (nonatomic) UIImageView *glSnapshotView; | |||
@property (nonatomic, readwrite) MGLScaleBarView *scaleBarView; |
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.
We expose the other views that sit on top of the map (compass, logo, attribution) as public MGLMapView properties — I think it would be consistent and useful to do the same for this scale view. At minimum, it would allow a developer to hide the scale bar.
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.
If we expose the view as an ordinary UIView, the developer can still hide the scale bar and we retain more flexibility to iterate on the view’s design and structure.
Since this PR is scheduled for iOS SDK v3.5.0, it needs to be rebased and retargeted at the release-ios-v3.5.0-android-v5.0.0 branch. |
fca1f61
to
e545cb5
Compare
I could see adding customization options causing this PR to drag on a bit longer, whereas the feature is already quite polished. How about we make MGLScaleBar private and change the public type of the |
MGLScaleBarView is now private and supports imperial units. I had to change the roundingIncrement of MGLDistanceFormatter to avoid rounding less than 12.5 feet down to 0 and you can zoom into 2 feet per bar near the poles. This PR is ready for another round 👀 |
ff056a5
to
7de5c27
Compare
toItem:self | ||
attribute:NSLayoutAttributeTop | ||
multiplier:1 | ||
constant:5+self.contentInset.top]]; |
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.
Should we stick to a more standard constant like 4 points?
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.
We've been using 5 on the compassView for quite a while. Let's revisit when we replace these constraints in #7716
platform/ios/src/MGLMapView.mm
Outdated
}]; | ||
} else { | ||
[self unrotateIfNeededForGesture]; | ||
[self notifyScaleBarGestureDidEnd]; |
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.
In the past, atomic gestures like double-tapping avoided calling -notifyGestureDidBegin
and -notifyGestureDidEndWithDrift:
. We should have these gestures call -notifyGestureDidBegin
and -notifyGestureDidEndWithDrift:
one right after the other, and merge -notifyScaleBarGestureDidEnd
into -notifyGestureDidEndWithDrift:
. That way there are fewer things to keep track of every time we finish a gesture.
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 wonder if this will also help with an issue I’m seeing in which pinching multiple times in quick succession causes the scale bar to stay visible; only pinching again gets the scale bar unstuck.
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.
-[MGLMapView handlePanGesture:]
calls -notifyGestureDidBegin
.
We have to keep them separated if non-zoom-related gestures like panning shouldn't trigger the scale bar. Should we extract into -notifyGestureDidBegin
and -notifyZoomChangingGestureDidBegin
?
/cc @friedbunny
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.
@frederoni I think we have to keep the scale bar unhiding in -cameraIsChanging
, otherwise programmatic zooming won’t trigger it. If that’s true, then bifurcating -notifyGestureDidBegin
wouldn’t do us any good. (Have we considered programmatic zooming?)
If we don’t care about programmatic zoom triggering the scale bar, then perhaps we can keep a single method but have it take an enum, e.g., -notifyGestureDidBeginWithType:
and MGLGestureType
, that would define whether or not it was initiated by a zoom-related gesture. (Or just a -notifyGestureDidBeginWithZoom:
boolean.)
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.
Have we considered programmatic zooming?
MapKit only shows the scale in response to user gestures, not in response to programmatic camera changes. However, there is an -[MKMapCamera showsScale]
property that determines whether the scale bar is always visible: if the developer wants the scale bar to be visible during programmatic zooming, they can set and unset showsScale
around that change to the map’s region or camera.
platform/ios/src/MGLMapView.mm
Outdated
|
||
|
||
if (!self.scaleBarView.hidden) { | ||
[(MGLScaleBarView *)self.scaleBarView setMetersPerPoint:[self metersPerPointAtLatitude:self.centerCoordinate.latitude]]; |
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.
What if the scale bar is hidden while the user zooms the map, but then the developer unhides the scale bar afterwards? Would the scale bar remain accurate? Can we make it safe to call -[MGLScaleBarView setMetersPerPoint:metersPerPointAtLatitude:]
even when MGLScaleBarView is hidden?
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.
The scale bar remains faded out until you set metersPerPoint
making it safe. I'd prefer not to set metersPerPoint
if the view is hidden to avoid excessive layout code from being executed.
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.
Sorry, something isn’t clicking for me: if the developer hides the scale bar using mapView.scaleView.hidden = YES
, then the user zooms way in, then the developer sets mapView.scaleView.hidden = NO
, wouldn’t the scale bar appear with the wrong scale until the user starts zooming again?
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.
If the zoom interaction occurred less than one second ago, it would show the incorrect scale for the remaining milliseconds up to 1 sec but since it was hidden, we never call -fadeIn
leaving the view.alpha at 0 making the incorrect scale invisible. You have to set metersPerPoint
again to make it visible and then it would show the correct scale.
However, it is possible to trigger the case you think of but it's slightly harder in practice;
Using scaleView.hidden = NO
, zoom to fade in the scale bar, hide the scale bar, change the zoom, and unhide the scale bar all within 1 second in order for it to show an incorrect scale.
Though, we can remove the if-statement if the overhead is exaggerated.
platform/ios/src/MGLScaleBarView.h
Outdated
|
||
@property (nonatomic, assign) CLLocationDistance metersPerPoint; | ||
|
||
- (void)fadeOut; |
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.
Even though this class is private, it would be good to give it some documentation.
platform/ios/src/MGLMapView.h
Outdated
map and fades out when the interaction stops. It’s positioned | ||
in the upper-left corner. | ||
*/ | ||
@property (nonatomic, readonly) UIView *scaleBarView; |
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.
“Scale bar view” is redundant. Let’s call it either a “scale bar” (scaleBar
, MGLScaleBar
) or a “scale view” (scaleView
, MGLScaleView
).
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.
Let's rename to MGLScaleView, it aligns better with Cocoa naming conventions.
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.
For what it’s worth, “bar” is primarily used in UITabBar.
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.
waiting with renaming the class to keep the reviewing process intact
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 see that you've renamed the class to MGLScaleBar. Just to clarify, my comment above was meant to point out that UITabBar is a "bar" but almost nothing else is. Regardless, I'm comfortable with either name. 👍
platform/ios/src/MGLMapView.h
Outdated
A control indicating the scale of the map. | ||
|
||
The scale bar becomes visible when a user interacts with the | ||
map and fades out when the interaction stops. It’s positioned |
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.
Nit: wrap to 80 columns.
platform/ios/src/MGLMapView.h
Outdated
A control indicating the scale of the map. | ||
|
||
The scale bar becomes visible when a user interacts with the | ||
map and fades out when the interaction stops. It’s positioned |
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.
Nit: avoid contractions, which make the documentation sound too informal.
platform/ios/src/MGLScaleBarView.m
Outdated
_row = @[@0, @0]; | ||
|
||
// @[meters, numberOfBars] | ||
_metricTable = @[@[@1, @2], |
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.
There seems to be quite a bit of Objective-C overhead in managing this tabular data (in terms of syntax, not necessarily performance). These tables should be statically initialized as C arrays of anonymous structs. For example, you could declare an inline type:
static const struct {
CLLocationDistance distance;
NSUInteger numberOfBars;
} MGLNumberOfBarsByDistance[] = {
{ .distance = 1, .numberOfBars = 2 }, // or simply { 1, 2 }
…
};
Then everything is fully typed, so you can iterate over the rules like this:
for (NSUInteger i = 0; i < sizeof(MGLNumberOfBarsByDistance) / sizeof(MGLNumberOfBarsByDistance[0]); i++ {
// …
CLLocationDistance distance = MGLNumberOfBarsByDistance[i].distance;
// …
}
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.
It seems that I'm adding more overhead when using static C arrays of anonymous structs, either in terms of manual memory management when allocating new C arrays or using a C++ list to return a subset of the rows needed for -[MGLScaleBarView validRows]
and -[MGLScaleBarView preferredRowInRows:]
. I'm leaning towards rewriting those methods in objc++.
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.
There are two important points here:
- The tables never change, so they should be allocated statically. A static C structure would be best; if an NSArray is really necessary, it should be created inside a dispatch_once block.
- The table is already sorted by distance, so the valid rows are always contiguous. Therefore, we can represent the valid rows as a range of indices into the static table rather than allocating a new subarray each time we relayout.
This code doesn't benefit from NSArray's mutability or its ability to hold objects.
platform/ios/src/MGLScaleBarView.m
Outdated
self.actualWidth, | ||
MGLBarHeight+self.borderWidth*2); | ||
|
||
self.containerView.layer.cornerRadius = 2.0f; |
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.
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 tend to agree. I wanted to round the background two pixels and the border one pixel but rounding one pixel renders differently in CG compared to Sketch.
platform/ios/src/MGLScaleBarView.m
Outdated
UIColor *textColor = self.textColor; | ||
|
||
CGContextRef context = UIGraphicsGetCurrentContext(); | ||
CGContextSetLineWidth(context, 1); |
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.
The stroke width here (around the outside of the label’s numbers/text) feels a bit too thin and diminishes legibility.
It does look like widening the stroke width causes the left/right outside edges to get cropped a bit, though... especially the zero character — its stroke is very uneven and sharp when 2
.
The scale bar, as of 7de5c27: |
08a7733
to
9a7bfd2
Compare
platform/ios/src/MGLScaleBar.mm
Outdated
}; | ||
|
||
static const struct { | ||
struct MGLRow row; |
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.
This is one extra level of indirection. All we need is an array of MGLRows; this creates an array of anonymous structs containing MGLRows. With a simple array of MGLRows, we can eliminate some of the braces below.
My earlier suggestion was that, as long as you don't need to refer to the specific type of these rows anywhere else, it's possible to inline MGLRow here, without naming it, and use auto
everywhere else you'd refer to the row's type.
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.
Anyhow, since the Imperial table needs access to the same type as this table, it does make sense to name MGLRow, just not to then stuff it into an anonymous struct as this code does.
180f0ad
to
94fe3ac
Compare
A couple more observations about MapKit’s scale bar implementation:
|
Leave show/hide up to the developer simplifies a lot indeed. 👍 |
9f01100
to
c113e89
Compare
platform/ios/src/MGLScaleBar.mm
Outdated
? MGLMetricTable[count-1].distance | ||
: MGLImperialTable[count-1].distance; | ||
|
||
self.alpha = maximumDistance > allowedDistance ? 0 : 1; |
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.
MapKit fades the scale bar in and out as it appears and disappears due to the maximum distance threshold. We could use a UIView animation block here perhaps.
platform/ios/src/MGLMapView.h
Outdated
@@ -226,6 +226,12 @@ IB_DESIGNABLE | |||
- (IBAction)reloadStyle:(id)sender; | |||
|
|||
/** | |||
A control indicating the scale of the map. The scale bar is positioned in the | |||
upper-left corner. |
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.
Note that the view is hidden by default.
c113e89
to
c6fa267
Compare
platform/ios/src/MGLScaleBar.mm
Outdated
CGFloat alpha = maximumDistance > allowedDistance ? .0f : 1.0f; | ||
|
||
if(self.alpha != alpha) { | ||
[UIView animateWithDuration:.2f delay:0 options:UIViewAnimationOptionBeginFromCurrentState animations:^{ |
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.
FYI, most of MGLMapView’s animations occur within 300 milliseconds, via MGLAnimationDuration
. (200 milliseconds is fine, though.)