Make telemetry on/off setting available in-app #3445

friedbunny · 2016-01-06T08:21:04Z

Changes the ℹ️ action sheet to include a "Mapbox Telemetry" entry that pops up a modal dialog with the telemetry on/off setting. This replaces "Adjust privacy settings", which kicked the user out to the system Settings.app.

Because the telemetry on/off setting is now always available (so long as the ℹ️ button is not hidden), there is no longer the requirement that the developer provide such a setting in their app's Settings.bundle or specify in Info.plist that they were otherwise providing this setting in-app.

Providing the telemetry setting in Settings.app (via Settings.bundle) is still a recommended best-practice and completely supported, it's just no longer a requirement.

~~Blocked by #3336, which causes this in-app setting changing to not take immediate effect.~~

/cc @1ec5

friedbunny · 2016-01-06T08:28:38Z

I'll squash this down before merging, but I wanted to preserve some code for discussion.

I think we should remove +[MGLAccountManager mapboxMetricsEnabledSettingShownInApp] entirely before merging, but for now I've made it deprecated and throw an exception. This should never have been used externally.
-[MGLMapboxEvents init] no longer has to care if Settings.bundle exists, so I ripped all of that out.

1ec5 · 2016-01-06T09:05:27Z

for now I've made it deprecated and throw an exception

That would make the method unavailable, not deprecated. There’s an attribute for marking a method as unavailable, too, but I don’t think we need the backwards-incompatibility in this case. Just turn the method into a no-op with an NSLog about the deprecation.

-[MGLMapboxEvents init] no longer has to care if Settings.bundle exists, so I ripped all of that out.

What if the ℹ️ button is hidden? Presumably the developer will implement the attribution elsewhere in the application, but then we should clarify the MGLMapView.attributionButton documentation. Currently it only talks about copyright notices, but the user opt-out is also important to get right.

1ec5 · 2016-01-06T09:07:09Z

include/mbgl/ios/MGLAccountManager.h

- Whether in-app user metrics opt-out is configured. If set to the default value of `NO`, a user opt-out preference is expected in a `Settings.bundle` that shows in the application's section within the system Settings app.
- */
-+ (BOOL)mapboxMetricsEnabledSettingShownInApp;
+ Whether in-app user telemetry opt-out is configured. If set to the default value of `NO`, a user opt-out preference was expected in a `Settings.bundle` that shows in the application's section within the system Settings app. Telemetry settings were added directly to the in-app ℹ️ menu, obseleting this method.


Jazzy doesn’t recognize the deprecated attribute yet. Our convention from the appledoc days is to omit documentation for deprecated and unavailable methods, but if this information is still valuable, I’d stick the last sentence in a @note.

Ultimately killed the docs here.

friedbunny · 2016-01-06T09:15:51Z

What if the ℹ️ button is hidden?

We could repurpose/rename the MGLMapboxMetricsEnabledSettingShownInApp plist key, checking if ℹ️ is hidden and shouting if the key is not set, but I don't think I'm in favor of that — improved docs for MGLMapView.attributionButton will hopefully suffice.

1ec5 · 2016-01-06T19:29:09Z

platform/ios/src/MGLAccountManager.m

@@ -60,7 +52,7 @@ + (instancetype) sharedManager {
 }

 + (BOOL) mapboxMetricsEnabledSettingShownInApp {
-    return [MGLAccountManager sharedManager].mapboxMetricsEnabledSettingShownInApp;
+    NSLog(@"mapboxMetricsEnabledSettingShownInApp is no longer necessary, the Mapbox iOS SDK has changed to always provide a telemetry setting in-app.");


Nit: , → ;.

1ec5 · 2016-01-06T19:29:28Z

Looks good!

friedbunny · 2016-01-06T21:39:31Z

Incoming MGLMapView.attributionButton docs updates:

The new, second note links to https://www.mapbox.com/help/metrics-opt-out-for-users/, which will need updating.

1ec5 · 2016-01-08T23:05:06Z

platform/ios/src/MGLMapboxEvents.h

+ (void) validate;
+
+// Main thread only
+ (void) ensureMetricsOptoutExists;


We should be asserting that we’re on the main thread, using MGLAssertIsMainThread(), instead of using these Java annotation–like comments, which mean nothing.

We do MGLAssertIsMainThread() in most cases here — I'll go ahead and do so for ensureMetricsOptoutExists, too.

1ec5 · 2016-01-08T23:08:55Z

The setting is still called Mapbox Metrics in a couple places. (Don’t ask me why it’s duplicated…)

Mind updating the strings?

1ec5 · 2016-01-08T23:17:22Z

platform/ios/src/MGLMapboxEvents.m

+             @"End users must be able to opt out of Mapbox Telemetry in your app, either inside Settings (via Settings.bundle) or inside this app. "
+             @"By default, this opt-out control is included as a menu item in the attribution action sheet. "
+             @"If you reimplement the opt-out control inside this app, disable this assertion by setting MGLMapboxMetricsEnabledSettingShownInApp to YES in Info.plist."
+             @"\r\n\r\nSee https://www.mapbox.com/ios-sdk/#telemetry_opt_out for more information."


\r is extraneous on Unix-based systems. (On the Mac, it results in Windows-style selection, so I wouldn’t be surprised if VoiceOver trips over it in iOS.)

- Deprecate and no-op `+[MGLAccountManager mapboxMetricsEnabledSettingShownInApp]` - Check for attribution button hiding and make sure ToS are understood

friedbunny added iOS Mapbox Maps SDK for iOS refactor telemetry Integration with Mapbox Telemetry libraries labels Jan 6, 2016

friedbunny added this to the ios-v3.1.0 milestone Jan 6, 2016

mourner added the in progress label Jan 6, 2016

1ec5 reviewed Jan 6, 2016
View reviewed changes

friedbunny force-pushed the tell-me-more branch 2 times, most recently from 44952a8 to 09ab2ed Compare January 8, 2016 22:47

1ec5 reviewed Jan 8, 2016
View reviewed changes

[ios] Add telemetry setting directly to attribution button action sheet

45a0f48

- Deprecate and no-op `+[MGLAccountManager mapboxMetricsEnabledSettingShownInApp]` - Check for attribution button hiding and make sure ToS are understood

friedbunny force-pushed the tell-me-more branch from 8080b92 to 45a0f48 Compare January 9, 2016 06:24

friedbunny added ready and removed in progress labels Jan 9, 2016

friedbunny self-assigned this Jan 9, 2016

friedbunny merged commit 45a0f48 into master Jan 9, 2016

mourner removed the ready label Jan 9, 2016

friedbunny deleted the tell-me-more branch January 9, 2016 06:38

mikemorris mentioned this pull request Feb 11, 2016

Android Telemetry User Opt Out UI #3907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make telemetry on/off setting available in-app #3445

Make telemetry on/off setting available in-app #3445

friedbunny commented Jan 6, 2016

friedbunny commented Jan 6, 2016

1ec5 commented Jan 6, 2016

1ec5 Jan 6, 2016

friedbunny Jan 6, 2016

friedbunny commented Jan 6, 2016

1ec5 Jan 6, 2016

1ec5 commented Jan 6, 2016

friedbunny commented Jan 6, 2016

1ec5 Jan 8, 2016

friedbunny Jan 8, 2016

1ec5 commented Jan 8, 2016

1ec5 Jan 8, 2016

Make telemetry on/off setting available in-app #3445

Make telemetry on/off setting available in-app #3445

Conversation

friedbunny commented Jan 6, 2016

friedbunny commented Jan 6, 2016

1ec5 commented Jan 6, 2016

1ec5 Jan 6, 2016

Choose a reason for hiding this comment

friedbunny Jan 6, 2016

Choose a reason for hiding this comment

friedbunny commented Jan 6, 2016

1ec5 Jan 6, 2016

Choose a reason for hiding this comment

1ec5 commented Jan 6, 2016

friedbunny commented Jan 6, 2016

1ec5 Jan 8, 2016

Choose a reason for hiding this comment

friedbunny Jan 8, 2016

Choose a reason for hiding this comment

1ec5 commented Jan 8, 2016

1ec5 Jan 8, 2016

Choose a reason for hiding this comment