Warn when accessing MGLStyle before style is loaded #7639

boundsj · 2017-01-09T05:27:45Z

No description provided.

mention-bot · 2017-01-09T05:27:48Z

@boundsj, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @friedbunny to be potential reviewers.

boundsj · 2017-01-09T05:27:56Z

~~Fixes #7512~~

boundsj · 2017-01-09T05:32:01Z

@1ec5 the macOS MGLMapView implementation triggers the warning itself because -[MGLMapView updateAttributionView] calls -[MGLStyle(Private) attributionInfosWithFontSize:linkColor:] before the style is loaded. Would it be ok to remove the call to updateAttributionView in commonInit?

1ec5 · 2017-01-09T06:14:11Z

Would it be ok to remove the call to updateAttributionView in commonInit?

Removing the call to -updateAttributionView from -installAttributionView would prevent the attribution view from ever appearing. Removing the call to -installAttributionView from -commonInit would make more sense (since -notifyMapChange also calls it), but that leads to some Auto Layout exceptions at runtime:

2017-01-08 22:13:04.885 Mapbox GL[57716:2437714] *** +[NSLayoutConstraint constraintWithItem:attribute:relatedBy:toItem:attribute:multiplier:constant:]: A multiplier of 0 or a nil second item together with a location for the first attribute creates an illegal constraint of a location equal to a constant. Location attributes must be specified in pairs
2017-01-08 22:13:04.944 Mapbox GL[57716:2437714] *** +[NSLayoutConstraint constraintWithItem:attribute:relatedBy:toItem:attribute:multiplier:constant:]: A multiplier of 0 or a nil second item together with a location for the first attribute creates an illegal constraint of a location equal to a constant. Location attributes must be specified in pairs

1ec5 · 2017-01-09T06:16:17Z

platform/ios/src/MGLMapView.mm

+    if (_isWaitingForStyleLoad) {
+        static dispatch_once_t onceToken;
+        dispatch_once(&onceToken, ^{
+            NSLog(@"WARNING: -[MGLMapView style] was called before -[MGLMapViewDelegate mapView:didFinishLoadingStyle:]. Wait for the map to finish loading the style before attempting to change the style at runtime.");


Mention in the message that the warning will only appear once.

1ec5 · 2017-01-09T06:21:59Z

platform/ios/src/MGLMapView.mm

@@ -390,6 +393,8 @@ - (void)commonInit
 {
    MGLinitializeRunLoop();

+    _isWaitingForStyleLoad = YES;


Setting or reloading the style URL should also cause us to wait for the style to load. If you want to be sure the flag gets set at the right times, replace any occurrences of _style with self.style, then override -setStyle: to set this flag.

Fixed in 80f96a1

1ec5 · 2017-01-09T06:23:48Z

platform/macos/src/MGLMapView.mm

@@ -2275,6 +2281,17 @@ - (void)updateAnnotationCallouts {
    return _mbglMap;
 }

+- (MGLStyle *)style
+{
+    if (_isWaitingForStyleLoad) {


Add && _style to silence the initial warning triggered by the attribution view. The style isn’t set by the time -commonInit calls -installAttributionView.

1ec5 · 2017-01-09T06:28:48Z

As an alternative to adding a private flag to MGLMapView, we could add a loaded property to MGLStyle, so that developers could determine synchronously whether the style is loaded yet. But -[MGLMapView style] is a good bottleneck, so tracking the loaded state in MGLMapView is good enough for now.

boundsj · 2017-01-09T22:26:15Z

Removing the call to -installAttributionView from -commonInit would make more sense (since -notifyMapChange also calls it)

I tested that and hit this:

* thread #1: tid = 0x1504da4, 0x0000000100105024 Mapbox`::-[MGLMapView style](self=0x000000010281c890, _cmd="style") + 20 at MGLMapView.mm:2296, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000100105024 Mapbox`::-[MGLMapView style](self=0x000000010281c890, _cmd="style") + 20 at MGLMapView.mm:2296
    frame #1: 0x00007fff951dce3c Foundation`-[NSObject(NSKeyValueCoding) valueForKey:] + 283
    frame #2: 0x00007fff951dca0a Foundation`-[NSObject(NSKeyValueCoding) valueForKeyPath:] + 260
    frame #3: 0x00007fff91334dc5 AppKit`-[NSBinder valueForBinding:resolveMarkersToPlaceholders:] + 164
    frame #4: 0x00007fff9167bce0 AppKit`-[NSObjectDetailBinder refreshDetailContent] + 78
    frame #5: 0x00007fff9132ccbb AppKit`-[NSObject(NSKeyValueBindingCreation) bind:toObject:withKeyPath:options:] + 773
    frame #6: 0x00007fff912a2899 AppKit`-[NSIBObjectData nibInstantiateWithOwner:options:topLevelObjects:] + 1284
    frame #7: 0x00007fff91299b8e AppKit`loadNib + 375
    frame #8: 0x00007fff912990d4 AppKit`+[NSBundle(NSNibLoading) _loadNibFile:nameTable:options:withZone:ownerBundle:] + 308
    frame #9: 0x00007fff914f270a AppKit`+[NSBundle(NSNibLoadingInternal) _loadNibFile:externalNameTable:options:withZone:] + 150
    frame #10: 0x00007fff914f24e1 AppKit`-[NSWindowController loadWindow] + 323
    frame #11: 0x00007fff912e4d79 AppKit`-[NSWindowController window] + 84
    frame #12: 0x00007fff91d1df4d AppKit`-[NSDocument(NSPersistentUISupport) restoreDocumentWindowWithIdentifier:state:completionHandler:] + 161
    frame #13: 0x00007fff91821dfd AppKit`-[NSDocumentControllerPersistentRestoration loadedDocument:forAutoID:] + 165
    frame #14: 0x00007fff9182848c AppKit`-[NSDocumentController _restorePersistentDocumentWithState:completionHandler:] + 1014
    frame #15: 0x00007fff91828cd1 AppKit`__76+[NSDocumentController restoreWindowWithIdentifier:state:completionHandler:]_block_invoke_2 + 553
    frame #16: 0x00007fff91828a9f AppKit`__76+[NSDocumentController restoreWindowWithIdentifier:state:completionHandler:]_block_invoke + 119
    frame #17: 0x00007fff91828a22 AppKit`+[NSDocumentController restoreWindowWithIdentifier:state:completionHandler:] + 101
    frame #18: 0x00007fff91606029 AppKit`-[NSApplication(NSPersistentUIRestorationSupport) restoreWindowWithIdentifier:state:completionHandler:] + 182
    frame #19: 0x00007fff9160597f AppKit`-[NSApplication(NSPersistentUIRestorationSupport) _restoreWindowWithRestoration:completionHandler:] + 637
    frame #20: 0x00007fff915cf08e AppKit`-[NSPersistentUIRestorer restoreStateFromRecords:usingDelegate:completionHandler:] + 2137
    frame #21: 0x00007fff91d1cb45 AppKit`-[NSPersistentUIManager restoreAllPersistentStateWithCompletionHandler:] + 272
    frame #22: 0x00007fff912d9b2c AppKit`-[NSApplication _reopenWindowsAsNecessaryIncludingRestorableState:completionHandler:] + 368
    frame #23: 0x00007fff912d98a6 AppKit`-[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] + 539
    frame #24: 0x00007fff912d9505 AppKit`-[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] + 661
    frame #25: 0x00007fff951e6ebd Foundation`-[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] + 290
    frame #26: 0x00007fff951e6d37 Foundation`_NSAppleEventManagerGenericHandler + 102
    frame #27: 0x00007fff945e90da AE`aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) + 544
    frame #28: 0x00007fff945e8e51 AE`dispatchEventAndSendReply(AEDesc const*, AEDesc*) + 39
    frame #29: 0x00007fff945e8d5d AE`aeProcessAppleEvent + 312
    frame #30: 0x00007fff92d3f7bf HIToolbox`AEProcessAppleEvent + 55
    frame #31: 0x00007fff912d4dad AppKit`_DPSNextEvent + 1833
    frame #32: 0x00007fff91a4f21f AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 2789
    frame #33: 0x00007fff912c9465 AppKit`-[NSApplication run] + 926
    frame #34: 0x00007fff91293d80 AppKit`NSApplicationMain + 1237
    frame #35: 0x0000000100011382 Mapbox GL`main(argc=3, argv=0x00007fff5fbfed70) + 34 at main.m:4
    frame #36: 0x00007fffa8cfb255 libdyld.dylib`start + 1

I'm not sure about a way around this offhand. Since this works for the iOS SDK, can we either land this PR as is and file a bug for macOS specifically about the warning getting triggered -or- can I remove the macOS implementation and create a follow up ticket to implement it?

1ec5 · 2017-01-09T22:53:07Z

I suggested a fix in #7639 (comment) – I think this would be a good fix to have on both platforms. I don’t think we should be removing any calls to -installAnnotationView or -updateAnnotationView – that was a rhetorical flourish. 😉

boundsj · 2017-01-10T04:42:39Z

I suggested a fix in #7639 (comment) – I think this would be a good fix to have on both platforms.

@1ec5 I'm sorry, I somehow missed #7639 (comment) before.

What I was trying to say in #7639 (comment) is that, when I remove the call to installAttributionView (I understand it would be better to not do this), the macOS SDK still tries to access style as illustrated in the trace I posted. This is because of AppKit's calls to NSBundle(NSNibLoading) and NSBinder. Unfortunately, adding && _style does not prevent this because style is set by the time the binding happens. Adding && _style does solve the problem for the case of the attribution view setup (as you suggested).

I'll add && _style in any case but I'd like to come back to the binding issue on macOS later since this works for iOS unless there is something else you think I should try.

1ec5 · 2017-01-10T05:10:22Z

I see, that’s an interesting edge case that’ll affect any macOS code that binds to the style key path. In fact, it would also affect any iOS code that observes the style key path as well. Neither would be an especially common use case, but it happens to be essential for the functionality in macosapp. I’m fine with addressing this issue as tail work.

As far as solutions to this problem, what if we avoid initializing and setting _style until the style has loaded? The style property would be nullable; it would return nil until the style finishes loading and would remain nil if the style fails to load.

1ec5 · 2017-01-10T15:51:57Z

As far as solutions to this problem, what if we avoid initializing and setting _style until the style has loaded? The style property would be nullable; it would return nil until the style finishes loading and would remain nil if the style fails to load.

Going to make this a separate PR. Making style nullable (and thus optional in Swift) a pretty large change that requires changes to tests and example code.

_style check has been added.

1ec5 · 2017-01-10T16:53:17Z

A workaround for the KVO-triggered warning is difficult to come by, even if we make the style property nullable. I think this behavior is symptomatic of incorrect semantics on our part: if we add the warning to the getter, we’re making assumptions about how the caller is going to use the return value. What if the caller is only calling -style in order to check whether the style is loaded yet?

I have a change ready that makes the style property nullable (nilling it out when setting the style URL and setting it to a new object only once the style has finished loading). Do you think we could make that change and remove the warning? I think the optionality will be obvious enough to Swift developers as something to check when their synchronous code doesn’t seem to have an effect. Unfortunately, for Objective-C developers, there will still be no hint as to the problem without debugging or poring through documentation.

1ec5 · 2017-01-10T18:22:33Z

I have a change ready that makes the style property nullable (nilling it out when setting the style URL and setting it to a new object only once the style has finished loading).

These changes are in #7664.

boundsj · 2017-01-11T06:21:58Z

I'm convinced. Closing in favor of #7664 cc @1ec5

[ios, macos] Warn when accessing MGLStyle before style is loaded

3bd1874

boundsj added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Jan 9, 2017

boundsj added this to the ios-v3.4.0 milestone Jan 9, 2017

boundsj self-assigned this Jan 9, 2017

boundsj requested a review from 1ec5 January 9, 2017 05:27

boundsj added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jan 9, 2017

1ec5 previously requested changes Jan 9, 2017

View reviewed changes

1ec5 reviewed Jan 9, 2017

View reviewed changes

[ios, macos] Ensure style loading flag is always set

80f96a1

[ios, macos] And style!

ed9ef4f

1ec5 mentioned this pull request Jan 10, 2017

Make MGLMapView.style property nullable #7664

Merged

boundsj closed this Jan 11, 2017

boundsj deleted the boundsj-warn-about-style-access branch March 2, 2017 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when accessing MGLStyle before style is loaded #7639

Warn when accessing MGLStyle before style is loaded #7639

boundsj commented Jan 9, 2017

mention-bot commented Jan 9, 2017

boundsj commented Jan 9, 2017 •

edited

Loading

boundsj commented Jan 9, 2017

1ec5 commented Jan 9, 2017

1ec5 Jan 9, 2017

1ec5 Jan 9, 2017

boundsj Jan 9, 2017

1ec5 Jan 9, 2017

1ec5 commented Jan 9, 2017

boundsj commented Jan 9, 2017

1ec5 commented Jan 9, 2017 •

edited

Loading

boundsj commented Jan 10, 2017

1ec5 commented Jan 10, 2017

1ec5 commented Jan 10, 2017

1ec5 commented Jan 10, 2017

1ec5 commented Jan 10, 2017

boundsj commented Jan 11, 2017

Warn when accessing MGLStyle before style is loaded #7639

Warn when accessing MGLStyle before style is loaded #7639

Conversation

boundsj commented Jan 9, 2017

mention-bot commented Jan 9, 2017

boundsj commented Jan 9, 2017 • edited Loading

boundsj commented Jan 9, 2017

1ec5 commented Jan 9, 2017

1ec5 Jan 9, 2017

Choose a reason for hiding this comment

1ec5 Jan 9, 2017

Choose a reason for hiding this comment

boundsj Jan 9, 2017

Choose a reason for hiding this comment

1ec5 Jan 9, 2017

Choose a reason for hiding this comment

1ec5 commented Jan 9, 2017

boundsj commented Jan 9, 2017

1ec5 commented Jan 9, 2017 • edited Loading

boundsj commented Jan 10, 2017

1ec5 commented Jan 10, 2017

1ec5 commented Jan 10, 2017

1ec5 commented Jan 10, 2017

1ec5 commented Jan 10, 2017

boundsj commented Jan 11, 2017

boundsj commented Jan 9, 2017 •

edited

Loading

1ec5 commented Jan 9, 2017 •

edited

Loading