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

Fixed account type setting; reworded IB designable message #1615

Merged
merged 5 commits into from
May 21, 2015

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented May 21, 2015

This PR fixes some regressions from #1553, as well as some bugs that’ve been lingering for longer:

  • We need to look in the host app’s Info.plist, not MapboxGL.framework’s.
  • Currently, if you set MGLMapboxMetricsEnabledSettingShownInApp = YES in Info.plist, you still get the assertion failure in -[MGLMapboxEvents init] because the access token is read in first, causing the shared MGLMapboxEvents to be created. This change rearranges the order that the settings are read in, since one is dependent on the other.
  • Interface Builder was still showing an “Access Token” inspectable, which would’ve led to an assertion failure if set.
  • The IB designable message is only relevant to developers displaying Mapbox-hosted maps, so the message should reflect that scope.
  • The IB designable message can’t detect whether the access token is set in Info.plist, and it can’t detect whether a Settings bundle has been added. So I nixed the “A-OK” Mapbox blue designable screen – we just show steps to get started no matter what and point to the “First steps with Mapbox GL for iOS” guide.

Fixes #1613.

1ec5 added 4 commits May 21, 2015 01:49
Need to look in the main bundle for Info.plist settings, because the developer has no access to the framework’s Info.plist. Need to check MGLMapboxMetricsEnabledSettingShownInApp before MGLMapboxAccessToken; otherwise, `MGLMapboxEvents` inspects the Settings bundle plist before MGLMapboxMetricsEnabledSettingShownInApp is even checked.
Access tokens are only required for showing Mapbox-hosted maps.
IB designables inside frameworks have no access whatsoever to the host app’s bundle (especially if the host app lacks any designables of its own). So it has no way of knowing whether the access token is set in Info.plist. We could continue to check whether it was set programmatically, but we’d still have no way of knowing whether the host app has a Settings bundle, the other prerequisite for displaying Mapbox maps. Therefore, this change removes the Mapbox logo from the designable and displays a reworded reminder unconditionally. It now displays the URL of the “First steps with Mapbox GL for iOS” guide. I kept Mapbox blue in the designable, but as a border, so you can easily tell where the view ends (a major usability problem for custom views in IB).
@1ec5 1ec5 added bug iOS Mapbox Maps SDK for iOS labels May 21, 2015
@1ec5 1ec5 added this to the iOS Beta 2 milestone May 21, 2015
@friedbunny
Copy link
Contributor

Currently, if you set MGLMapboxMetricsEnabledSettingShownInApp = YES in Info.plist, you still get the assertion failure in -[MGLMapboxEvents init] because the access token is read in first, causing the shared MGLMapboxEvents to be created. This change rearranges the order that the settings are read in, since one is dependent on the other.

Mmm, am looking at this now because it was crashing the tests. Have a couple questions:

  1. Why use [MGLAccountManager sharedManager].mapboxMetricsEnabledSettingShownInApp but then self.accessToken in +[MGLAccountManager load]?
  2. Why start [MGLMapboxEvents sharedManager] in +[MGLAccountManager setAccessToken:] and not at the end of +[MGLAccountManager load]?

@friedbunny
Copy link
Contributor

MapViewTests pass with these changes (after applying 0c78314), thanks to the reordering of the first [MGLMapboxEvents sharedManager] call.

@1ec5
Copy link
Contributor Author

1ec5 commented May 21, 2015

Why use [MGLAccountManager sharedManager].mapboxMetricsEnabledSettingShownInApp but then self.accessToken in +[MGLAccountManager load]?

#1553 made +[MGLAccountManager setMapboxMetricsEnabledSettingShownInApp:] unavailable as a precursor to removing it entirely. (Although I see it’s still implemented.) As far as I can tell, there’s no way to make a method un-unavailable privately without subclassing. If you can think of a way to override the unavailable() attribute, I’m all ears!

Why start [MGLMapboxEvents sharedManager] in +[MGLAccountManager setAccessToken:] and not at the end of +[MGLAccountManager load]?

Nothing that MGLMapboxEvents does makes sense without an access token. The events API (the server API, that is) requires an access token. Moreover, Mapbox Metrics shouldn’t be active if the app is using a non-Mapbox-hosted style and tile server.

This class method is marked unavailable in MGLAccountManager.h, so this code can never be reached.
@1ec5
Copy link
Contributor Author

1ec5 commented May 21, 2015

Here’s what the designable looks like now in various form factors:

compactcompact
anyany
small
smallish

As you can see, I also added some additional constraints so that the designable looks more presentable when it’s too small to show everything.

1ec5 added a commit that referenced this pull request May 21, 2015
Fixed account type setting; reworded IB designable message
@1ec5 1ec5 merged commit a2a6330 into master May 21, 2015
@1ec5 1ec5 removed the in progress label May 21, 2015
@1ec5 1ec5 deleted the 1ec5-plist-order branch May 21, 2015 21:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IB designable fails to recognize access token set in Info.plist
2 participants