[iOS] Enable change position of UI elements #13556

lloydsheng · 2018-12-12T11:30:05Z

We need to enable change position of UI elements. This PR was exposed APIs which can be used to customize the position of scalebar, compass, logo and attribution.
Those UI elements can be easily placed on four corners and adjust position with offset.

Implement layout codes
Add comments and update changelog
Manual testing on different devices and systems

/cc @danesfeder @suntony @chriswu42 @jmkiley @macro-shen

lloydsheng · 2018-12-13T10:17:43Z

@jmkiley @1ec5 Can you have an 👀 at this PR?

julianrex · 2018-12-13T15:38:53Z

@fabian-guerra can you take a look at this PR please.

julianrex · 2018-12-13T15:54:52Z

Thanks @lloydsheng for this PR! I'm excited to see this appear - and think it'll address a lot of existing requests. For example, I believe it could fix #11068.

We do have a number of other constraint related tickets in the backlog - I wonder if we can role some of those into this PR?

@frederoni's Adopt Layour Anchors PR Adopt layout anchors #12147 - I would like to adopt this approach in this PR (or vice-versa). Please take a look.
Performance: -[MGLMapView updateConstraints] regenerates constraints unconditionally #12142 - which reminds me - we should add some tests to this PR to see how performance has changed.
Add support for contentInsetAdjustmentBehavior: Add contentInsetAdjustmentBehavior property to MGLMapView #12143

lloydsheng · 2018-12-17T10:53:10Z

Adopt Layour Anchors PR #12147 - I would like to adopt this approach in this PR (or vice-versa). Please take a look.

@julianrex We can't adopt this approach. as we have to cherry pick the PR to iOS 8 branches. I've optimized the performance according to #12144 cc: @frederoni

Performance: #12142 - which reminds me - we should add some tests to this PR to see how performance has changed.

The performance would be better than before, casue it reinstall constraints only when needed now. I'll have tests for comparison.

Add support for contentInsetAdjustmentBehavior: #12143

Can this be done with another PR?

fabian-guerra

This looks good just some comments.

I realized that if someone adds a negative offset may hide our logo or attribute, not sure if it makes sense add a check for this.
Do you think it makes sense handling the case were a developer may inadvertently set the position of an ornament to a place where already exists one thus overlapping?
Have you considered exposing the constraint arrays instead? Or make a delegate that returns the ornaments constraints?
Could you please add screenshots for multiple size devices and for pre iOS 11.
For all public API methods please add their respective log message (take a look at other setter methods for reference).

platform/ios/src/MGLMapView.h

platform/ios/src/MGLMapView.mm

fabian-guerra · 2018-12-18T17:06:06Z

This PR fixes #11068

lloydsheng · 2018-12-21T07:32:03Z

@fabian-guerra

Do you think it makes sense handling the case were a developer may inadvertently set the position of an ornament to a place where already exists one thus overlapping?

It's easy to find out if there is overlapping.

Have you considered exposing the constraint arrays instead? Or make a delegate that returns the ornaments constraints?

Yes, but I think property setting APIs are more simple and easy to use.

Could you please add screenshots for multiple size devices and for pre iOS 11.

Screenshots for multiple size devices and systems

Device	❌ Navigation Bar	✅ Navigation Bar
iPhone XS Max - 12.1
iPhone X - 12.1
iPhone 6 Plus - 9.0
iPhone5 9.0

/cc @jmkiley @chriswu42

friedbunny · 2019-01-02T17:18:04Z

Layout Anchors

Adopt Layour Anchors PR #12147 - I would like to adopt this approach in this PR (or vice-versa). Please take a look.

@julianrex We can't adopt this approach. as we have to cherry pick the PR to iOS 8 branches. I've optimized the performance according to #12144 cc: @frederoni

I support @julianrex’s request — the “new” layout anchors API represents a significant improvement in both performance (according to Apple, anyway) and concision, and there’s no reason not to adopt it in the mainline SDK.

Tests

The screenshots are much appreciated, @lloydsheng, but this PR currently has a noticeable lack of automated/reproducible tests. Ornament layout has been a consistent source of bugs over the years and we should be extremely careful when making significant changes to it. (E.g., lack of tests is why #12147 hasn’t been merged yet.)

frederoni · 2019-01-02T18:04:42Z

We should be able to merge this PR with #12147
What we need is one extra layer of abstraction to support iOS 8. I'm thinking something along the lines of -[UIView(MGLLayoutAnchor) mgl_constraintEqualTo:constant:], MGLLayoutAnchor would have to be fully defined on iOS 8 but it would only act as an alias to NSLayoutAnchor when provided by the OS.

fabian-guerra · 2019-01-07T19:41:55Z

@julianrex @friedbunny could you please explain which parts of #12147 this PR should include? So far I've seen safeAreaLayoutGuide + anchors on both; with a major difference that this pr allows ornaments get placed elsewhere.

I agree that there are missing automated tests. Do you think that checking the constraint values would be enough? Or what do you have in mind?

Thanks @lloydsheng for your patience, as Jason already pointed out ornaments placement has caused some issues in the past that's why the team has been extra careful reviewing the pr.

lloydsheng · 2019-01-08T09:19:43Z

What we need is one extra layer of abstraction to support iOS 8.

@frederoni I prefer the straightforward layout codes the PR adopted. Cause the map view only has few ornaments and the UI hierarchy is simple. I already abstracted several methods to avoid duplication of code. As @fabian-guerra saild. It actually adopted safeAreaLayoutGuide, anchors for concision and + (void)activateConstraints: for better performance. Is it worth adding some categories/classes for a extra layer of abstraction to support iOS 8?

Jason already pointed out ornaments placement has caused some issues in the past that's why the team has been extra careful reviewing the pr.

@fabian-guerra @friedbunny I understand that. I can submit the demo code I used for manual testing if it's needed. This feature was strongly required by our main customers in China. I've spent many time to test it on different systems and screens of different sizes to make sure it works well. If you have any ideas for automated/reproducible tests please tell me. 🙏

cc @chriswu42

fabian-guerra · 2019-01-08T19:59:04Z

@fabian-guerra @friedbunny I understand that. I can submit the demo code I used for manual testing if it's needed. This feature was strongly required by our main customers in China. I've spent many time to test it on different systems and screens of different sizes to make sure it works well. If you have any ideas for automated/reproducible tests please tell me.

Unless @friedbunny has something different in mind I think you could add the manual test code as part of the iosapp & test each ornament's frame in MGLMapViewTests.

lloydsheng · 2019-01-10T09:59:51Z

@fabian-guerra @friedbunny I found that there were some tests for ornament layout in this file https://github.com/mapbox/mapbox-gl-native/blob/master/platform/ios/test/MGLMapViewLayoutTests.m. I will add more tests of this type to test different positions and offsets for each ornament.

fabian-guerra · 2019-01-10T19:52:18Z

I will add more tests of this type to test different positions and offsets for each ornament.
👍🏼@lloydsheng

lloydsheng · 2019-01-16T10:58:25Z

@fabian-guerra @friedbunny I've added the demo code and tests. please take a look at it.

friedbunny · 2019-01-16T23:18:19Z

platform/ios/src/MGLMapView.mm

@@ -6313,6 +6413,7 @@ - (void)updateScaleBar
    if ( ! self.scaleBar.hidden)
    {
        [(MGLScaleBar *)self.scaleBar setMetersPerPoint:[self metersPerPointAtLatitude:self.centerCoordinate.latitude]];
+        [self installScaleBarConstraints];


Placing this here can cause installScaleBarConstraints be to called many times a second (i.e., every time the camera changes and the scale is updated). Constantly updating the constraints in this way seems like it could have a negative effect on performance — can/should this be avoided?

I don't have a better way at the moment. As constraints of the scalebar should be updated every time when metersPerPoint value changed.

platform/ios/src/MGLMapView.mm

platform/ios/app/MBXOrnamentsViewController.m

fabian-guerra · 2019-01-17T00:34:38Z

platform/ios/app/MBXOrnamentsViewController.m

@@ -0,0 +1,92 @@
+#import "MBXOrnamentsViewController.h"


This example should be moved once this PR lands to https://github.com/mapbox/ios-sdk-examples repo as we use the latter for manual testing (with Objc / Swift versions).

lloydsheng · 2019-01-17T09:18:55Z

@fabian-guerra @friedbunny I've updated, Thanks for your comments.

lloydsheng · 2019-01-23T06:12:18Z

@fabian-guerra @friedbunny Can you please take a look at this PR?

fabian-guerra

This PR is almost done, I just added a comment regarding what Jason was asking.

platform/ios/src/MGLMapView.mm

lloydsheng · 2019-01-29T07:21:57Z

@fabian-guerra @friedbunny @frederoni I've adopted layout anchors and removed iOS8 compatible codes. Please take a look at it. 🙏

fabian-guerra

👍🏼

…left label

…box ornaments

This reverts commit e22d28c.

…" (#13870) This reverts commit e22d28c.

…of mapview ornaments (#13863) * Cherry-pick PR #13556 to adding options to customize positions of mapbox ornaments

lloydsheng requested a review from a team December 12, 2018 11:30

lloydsheng changed the title ~~[WIP] Enable change position of UI elements~~ [iOS] Enable change position of UI elements Dec 13, 2018

lloydsheng requested a review from 1ec5 December 13, 2018 10:16

julianrex requested review from jmkiley and fabian-guerra and removed request for jmkiley December 13, 2018 15:27

julianrex requested review from frederoni and removed request for 1ec5 December 13, 2018 15:55

fabian-guerra reviewed Dec 18, 2018

View reviewed changes

friedbunny reviewed Jan 16, 2019

View reviewed changes

fabian-guerra reviewed Jan 17, 2019

View reviewed changes

fabian-guerra reviewed Jan 25, 2019

View reviewed changes

platform/ios/src/MGLMapView.mm Outdated Show resolved Hide resolved

fabian-guerra approved these changes Jan 29, 2019

View reviewed changes

lloydsheng added 14 commits January 30, 2019 14:55

make default position offsets as constants

9a2eebf

imporve comments and fix typo

9982dee

remove unused code

4b69f87

Fix issue that intrinsicContentSize didn't consider width of the far …

8da0469

…left label

Correct top and bottom constraint setting

b09d8e4

add negative check

720676f

Improve log description

725a508

Add ornaments manual test demo

7eaa947

Add ornament tests

7b00bec

Fix misspelled method name

c2cb745

Correct format specifier

92352ae

Fix retain cycle

eeacc0e

Use anchors APIs; Remove iOS8 layout code

2f2886c

update sdk files

f46280d

lloydsheng force-pushed the lloyd-ui-position branch from c8fde61 to f46280d Compare January 30, 2019 06:57

lloydsheng merged commit e22d28c into master Jan 30, 2019

friedbunny added the iOS Mapbox Maps SDK for iOS label Jan 30, 2019

friedbunny added this to the release-k milestone Jan 30, 2019

friedbunny deleted the lloyd-ui-position branch January 30, 2019 19:27

lloydsheng added a commit that referenced this pull request Feb 2, 2019

Cherry-pick PR #13556 to adding options to customize positions of map…

5e577cf

…box ornaments

lloydsheng mentioned this pull request Feb 2, 2019

[ios] Cherry-pick PR #13911 to adding options to customize positions of mapview ornaments #13863

Merged

This was referenced Feb 4, 2019

Revert "[ios] Enable customize positions of mapview ornaments (#13556)" #13870

Merged

[ios] Scalebar position and size is incorrect #13873

Closed

julianrex pushed a commit that referenced this pull request Feb 5, 2019

Revert "[ios] Enable customize positions of mapview ornaments (#13556)"

a4a4f6e

This reverts commit e22d28c.

julianrex added a commit that referenced this pull request Feb 5, 2019

[ios] Reverts "Enable customize positions of mapview ornaments (#13556)…

5ba4a6a

…" (#13870) This reverts commit e22d28c.

lloydsheng restored the lloyd-ui-position branch February 11, 2019 01:46

lloydsheng mentioned this pull request Feb 12, 2019

[iOS] Enable developers to change position of ornaments #13911

Merged

lloydsheng added a commit that referenced this pull request Mar 20, 2019

[ios] Cherry-pick PR #13911 to adding options to customize positions …

dab3ab7

…of mapview ornaments (#13863) * Cherry-pick PR #13556 to adding options to customize positions of mapbox ornaments

awdhootlele mentioned this pull request Jul 10, 2019

Can we update compass icon positioning and image? rnmapbox/maps#229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] Enable change position of UI elements #13556

[iOS] Enable change position of UI elements #13556

lloydsheng commented Dec 12, 2018 •

edited

Loading

lloydsheng commented Dec 13, 2018 •

edited

Loading

julianrex commented Dec 13, 2018

julianrex commented Dec 13, 2018

lloydsheng commented Dec 17, 2018 •

edited

Loading

fabian-guerra left a comment •

edited

Loading

fabian-guerra commented Dec 18, 2018

lloydsheng commented Dec 21, 2018 •

edited

Loading

friedbunny commented Jan 2, 2019

frederoni commented Jan 2, 2019

fabian-guerra commented Jan 7, 2019

lloydsheng commented Jan 8, 2019

fabian-guerra commented Jan 8, 2019

lloydsheng commented Jan 10, 2019 •

edited

Loading

fabian-guerra commented Jan 10, 2019

lloydsheng commented Jan 16, 2019

friedbunny Jan 16, 2019

lloydsheng Jan 17, 2019

fabian-guerra Jan 17, 2019 •

edited

Loading

lloydsheng commented Jan 17, 2019

lloydsheng commented Jan 23, 2019

fabian-guerra left a comment

lloydsheng commented Jan 29, 2019

fabian-guerra left a comment

[iOS] Enable change position of UI elements #13556

[iOS] Enable change position of UI elements #13556

Conversation

lloydsheng commented Dec 12, 2018 • edited Loading

lloydsheng commented Dec 13, 2018 • edited Loading

julianrex commented Dec 13, 2018

julianrex commented Dec 13, 2018

lloydsheng commented Dec 17, 2018 • edited Loading

fabian-guerra left a comment • edited Loading

Choose a reason for hiding this comment

fabian-guerra commented Dec 18, 2018

lloydsheng commented Dec 21, 2018 • edited Loading

Screenshots for multiple size devices and systems

friedbunny commented Jan 2, 2019

Layout Anchors

Tests

frederoni commented Jan 2, 2019

fabian-guerra commented Jan 7, 2019

lloydsheng commented Jan 8, 2019

fabian-guerra commented Jan 8, 2019

lloydsheng commented Jan 10, 2019 • edited Loading

fabian-guerra commented Jan 10, 2019

lloydsheng commented Jan 16, 2019

friedbunny Jan 16, 2019

Choose a reason for hiding this comment

lloydsheng Jan 17, 2019

Choose a reason for hiding this comment

fabian-guerra Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

lloydsheng commented Jan 17, 2019

lloydsheng commented Jan 23, 2019

fabian-guerra left a comment

Choose a reason for hiding this comment

lloydsheng commented Jan 29, 2019

fabian-guerra left a comment

Choose a reason for hiding this comment

lloydsheng commented Dec 12, 2018 •

edited

Loading

lloydsheng commented Dec 13, 2018 •

edited

Loading

lloydsheng commented Dec 17, 2018 •

edited

Loading

fabian-guerra left a comment •

edited

Loading

lloydsheng commented Dec 21, 2018 •

edited

Loading

lloydsheng commented Jan 10, 2019 •

edited

Loading

fabian-guerra Jan 17, 2019 •

edited

Loading