[core] Replace mbgl::MapChange with mbgl::MapObserver #8377

brunoabinader · 2017-03-13T15:02:20Z

Implements mbgl::MapObserver, replacing mbgl::MapChange enumerator and mbgl::Backend::notifyMapChange. Now mbgl::Backend inherits from mbgl::MapObserver, which provides non-pure virtual equivalents for the enumerator items.

Caveats:

Qt preserves the QMapboxGL::MapChange enumerator from the public API.
Android refactor still requires mbgl::MapChange - this is not necessary but finishing up this platform refactor can happen on a separate PR. Once we finish cleaning up the Android platform, then we can safely remove mbgl::MapChange source code entirely.

Added suggested reviewers for platform-specific patches as well as core changes - please replace yourself with someone else from the mobile team otherwise.

Fixes #6383.

mention-bot · 2017-03-13T15:02:30Z

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

jfirebaugh · 2017-03-13T18:35:33Z

include/mbgl/map/map_observer.hpp

+        return mapObserver;
+    }
+
+    virtual void onRegionWillChange() {}


Suggest s/Region/Camera/g.

Ack. For now I'm abstaining from renaming the public APIs for Qt & Android. For iOS & macOS I'm adding entries to CHANGELOG.md about the renaming.

Renaming the public API for iOS and macOS would be a backwards-incompatible change that would require a major version bump. Fortunately, this PR doesn’t touch the public API (MGLMapView.h) at all, only the internal implementation of MGLMapView (MGLMapView.mm), so no changelog entry is required.

jfirebaugh · 2017-03-13T18:38:02Z

include/mbgl/map/map_observer.hpp

+    }
+
+    virtual void onRegionWillChange() {}
+    virtual void onRegionWillChangeAnimated() {}


Would it make sense to provide a boolean (or two-valued-enum) parameter to on*Change , rather than having separate on*Change and on*ChangeAnimated methods? Similarly with on*Rendering{Frame,Map}/on*Rendering{Frame,Map}FullyRendered.

I think so - I'm adding two enums to MapObserver: CameraChangeMode and RenderMode.

jfirebaugh · 2017-03-13T18:39:09Z

include/mbgl/map/map_observer.hpp

+    virtual void onWillStartRenderingFrame() {}
+    virtual void onDidFinishRenderingFrame() {}
+    virtual void onDidFinishRenderingFrameFullyRendered() {}
+    virtual void onWillStartRenderingMap() {}


Is there a useful difference between on*RenderingFrame and on*RenderingMap?

onWillStartRenderingMap() and onDidFinishRenderingMap() are based on MapKit’s -[MKMapViewDelegate mapViewWillStartRenderingMap:] (“when one or more tiles are revealed and require rendering”) and -[MKMapViewDelegate mapViewDidFinishRenderingMap:fullyRendered:] (“finishes rendering all of the currently visible tiles to the best of its ability”), which have analogues in our iOS and macOS SDKs.

If our iOS documentation is to be believed, on*RenderingFrame() would be called on each frame of a camera animation or property transition, even where no additional tiles need to be fetched and displayed. However, I don’t know for sure if that’s how we’ve implemented these notifications, given #2775.

/cc @friedbunny

In terms of usefulness, we’ve found the “rendering frame” notifications to be useful in situations where the developer wants to synchronize a view atop the map or override some annotation view behavior that we apply on every frame. But the “rendering map” notifications remain more generally useful for knowing when it’s safe to snapshot a map view or have the UI change once the map is quiescent.

1ec5 · 2017-03-13T19:54:18Z

platform/ios/src/MGLMapView.mm

- (void)notifyMapChange:(mbgl::MapChange)change
-{
-    // Ignore map updates when the Map object isn't set.
+- (void)onRegionWillChange:(bool)animated {


In Objective-C, use BOOL (YES/NO) instead of bool (true/false).

1ec5 · 2017-03-13T19:55:42Z

platform/ios/src/MGLMapView.mm

- (void)notifyMapChange:(mbgl::MapChange)change
-{
-    // Ignore map updates when the Map object isn't set.
+- (void)onRegionWillChange:(bool)animated {


Remove “on” from the names of this method and others like it. Also, ensure that there’s a word before the parameter that describes the parameter, so that the meaning of the YES or NO is apparent at the call site: -regionWillChangeAnimated:.

-cameraWillChangeAnimated: would also be acceptable.

The macOS SDK implementation of MGLMapViewDelegate already uses the term “camera” instead of “region”. The iOS SDK implementation blindly copied MapKit’s use of “region” without bringing over MapKit’s MKRegion struct.

We’ll want to fix the public interface when we’re ready for backwards-incompatible changes, but for now, either name works for this internal method.

1ec5 · 2017-03-13T19:56:15Z

platform/ios/src/MGLMapView.mm

-            break;
-        }
-        case mbgl::MapChangeSourceDidChange:
+- (void)onRegionIsChanging {


-regionIsChanging:

1ec5 · 2017-03-13T19:56:24Z

platform/ios/src/MGLMapView.mm

+    }
+}
+
+- (void)onRegionDidChange:(bool)animated {


-regionDidChangeAnimated:

1ec5 · 2017-03-13T19:56:38Z

platform/ios/src/MGLMapView.mm

+    }
+}
+
+- (void)onWillStartLoadingMap {


-mapViewWillStartLoadingMap

1ec5 · 2017-03-13T19:58:08Z

platform/ios/src/MGLMapView.mm

+    }
+}
+
+- (void)onDidFinishRenderingMap:(bool)fullyRendered {


-mapViewDidFinishRenderingMapFullyRendered:

1ec5 · 2017-03-13T19:59:14Z

platform/ios/src/MGLMapView.mm

+    }
+}
+
+- (void)onDidFinishLoadingStyle {


-mapViewDidFinishLoadingStyle

1ec5 · 2017-03-13T20:00:21Z

platform/ios/src/MGLMapView.mm

+    }
+}
+
+- (void)onSourceDidChange {


iOS doesn’t need to do anything in response to source changes, since attribution is displayed on-demand. You can safely remove this method and make its caller a no-op.

1ec5 · 2017-03-13T20:01:18Z

platform/macos/src/MGLMapView.mm

+    }
+}
+
+- (void)onSourceDidChange {


Pass in the mbgl::style::Source and rename this method to -sourceDidChange:.

I'm 👍 with the renaming, but I'd prefer to defer the source argument change as a platform-specific change meant to a separate PR in the same way @tobrun did in #8389 for Android.

1ec5 · 2017-03-13T20:07:00Z

src/mbgl/map/map.cpp

 }

 void Map::Impl::onStyleError() {
-    backend.notifyMapChange(MapChangeDidFailLoadingMap);
+    observer.onDidFailLoadingMap();


We should plumb res.error through to here or implement onResourceError() instead to allow the SDKs to provide more information about the failure. Currently, in order to fudge parity with MapKit, the iOS and macOS SDKs have to synthesize an uninformative NSError, but res.error’s message field would be perfect for this purpose.

Same as above, I'd prefer if we could move this to a platform-specific refactor.

This suggestion isn’t platform-specific but rather an improvement on the code being added in this PR. Tracking in #8398.

I see - thanks for the clarification. Let's handle this in #8398 👍

tobrun

Android side of things is 👍 , ticketed follow up ticket to migrate the android code base to this system in #8389.

1ec5

iOS and macOS changes look good. Looking forward to the core improvements in #8397 and especially #8398.

1ec5 · 2017-03-14T18:50:59Z

platform/ios/src/MGLMapView.mm

@@ -1167,7 +1167,8 @@ - (void)touchesBegan:(__unused NS_SET_OF(UITouch *) *)touches withEvent:(__unuse
 }

 - (void)notifyGestureDidBegin {
-    [self notifyMapChange:mbgl::MapChangeRegionWillChange];
+    bool animated = false;


s/bool/BOOL/ and s/false/NO/ (×2)

I'm a bit confused - I've seen bool usage when the code is a glue between C++ and Objective-C, and BOOL on Objective-C code along this source file - should we open an issue then to fix this for all remaining bools?

Ok, I think I got it now - this function is not part of the C++ glue and thus should use the Objective-C notation - got it :) Fixing now.

👍 As a rule of thumb, in Objective-C++ code, if you’re inside an Objective-C class or category (look for @implementation and @end), use Objective-C types; if you’re inside a C++ class or struct, use C++ types. There are some exceptions: for example, I find it useful to use static_cast over C-style casts throughout Objective-C++ code. And std::vector can often be faster than NSMutableArray. This article suggests some more ways C++ can improve Objective-C code, but a lot of it is a matter of preference.

brunoabinader · 2017-03-15T14:24:26Z

Qt public API refactor handled in #8419.

brunoabinader added Core The cross-platform C++ core, aka mbgl GLFW iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Node.js node-mapbox-gl-native Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL refactor labels Mar 13, 2017

brunoabinader self-assigned this Mar 13, 2017

brunoabinader requested review from jfirebaugh, tmpsantos, 1ec5 and tobrun March 13, 2017 15:02

brunoabinader force-pushed the map-observer branch 3 times, most recently from b3a3780 to de9c074 Compare March 13, 2017 15:41

jfirebaugh reviewed Mar 13, 2017

View reviewed changes

1ec5 suggested changes Mar 13, 2017

View reviewed changes

tobrun mentioned this pull request Mar 14, 2017

Remove deprecated notifyMapChange #8389

Closed

tobrun approved these changes Mar 14, 2017

View reviewed changes

brunoabinader force-pushed the map-observer branch from de9c074 to 81b0a2b Compare March 14, 2017 15:48

This was referenced Mar 14, 2017

mbgl::MapObserver::onSourceDidChange() should accept a source parameter #8397

Closed

-[MGLMapViewDelegate mapViewDidFailLoadingMap:withError:] error is uninformative #8398

Closed

1ec5 approved these changes Mar 14, 2017

View reviewed changes

brunoabinader added 5 commits March 15, 2017 13:38

[core] Replace MapChange enum with MapObserver

325f9fe

[Qt] Support mbgl::MapObserver

12f4b3f

[glfw] Support mbgl::MapObserver

848073b

[node] Support mbgl::MapObserver

4f370e1

[macos] Support mbgl::MapObserver

34099f2

brunoabinader force-pushed the map-observer branch from 81b0a2b to 71eee82 Compare March 15, 2017 11:44

brunoabinader added 2 commits March 15, 2017 14:32

[ios] Support mbgl::MapObserver

c4fb74f

[android] Support mbgl::MapObserver

ff2be9f

brunoabinader force-pushed the map-observer branch from 71eee82 to ff2be9f Compare March 15, 2017 12:33

brunoabinader merged commit a07e6ae into master Mar 15, 2017

brunoabinader deleted the map-observer branch March 15, 2017 12:53

brunoabinader mentioned this pull request Mar 15, 2017

[Qt] Refactor QMapboxGL API to reflect MapObserver semantics #8419

Closed

brunoabinader mentioned this pull request Mar 22, 2017

Region change notifications should include camera delta #4384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Replace mbgl::MapChange with mbgl::MapObserver #8377

[core] Replace mbgl::MapChange with mbgl::MapObserver #8377

brunoabinader commented Mar 13, 2017

mention-bot commented Mar 13, 2017

jfirebaugh Mar 13, 2017

brunoabinader Mar 14, 2017

1ec5 Mar 14, 2017 •

edited

Loading

jfirebaugh Mar 13, 2017

brunoabinader Mar 14, 2017

jfirebaugh Mar 13, 2017

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017 •

edited

Loading

1ec5 Mar 13, 2017 •

edited

Loading

1ec5 Mar 13, 2017 •

edited

Loading

1ec5 Mar 13, 2017

1ec5 Mar 13, 2017

brunoabinader Mar 14, 2017

1ec5 Mar 14, 2017

1ec5 Mar 13, 2017

brunoabinader Mar 14, 2017

1ec5 Mar 14, 2017

brunoabinader Mar 15, 2017

tobrun left a comment

1ec5 left a comment

1ec5 Mar 14, 2017

brunoabinader Mar 15, 2017

brunoabinader Mar 15, 2017

1ec5 Mar 15, 2017

brunoabinader commented Mar 15, 2017

[core] Replace mbgl::MapChange with mbgl::MapObserver #8377

[core] Replace mbgl::MapChange with mbgl::MapObserver #8377

Conversation

brunoabinader commented Mar 13, 2017

mention-bot commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun left a comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoabinader commented Mar 15, 2017

1ec5 Mar 14, 2017 •

edited

Loading

1ec5 Mar 13, 2017 •

edited

Loading

1ec5 Mar 13, 2017 •

edited

Loading

1ec5 Mar 13, 2017 •

edited

Loading