MGLStyle key-value coding compliance #6097

1ec5 · 2016-08-19T21:16:53Z

This PR makes the runtime styling API’s entry points key-value coding compliant.

The -[MGLMapView style] factory method has been replaced with a KVC-compliant style property whose value changes only when reloading the style or changing the style URL. This makes the API more convenient in Swift and more predictable overall.

New layers and sources properties have been added to MGLStyle that make it easy to enumerate style layers or sources without having to hard-code style-specific identifiers in application code. See #5999, #7030, and #7031 for examples of how these properties would be used.

The layers array is ordered from front to back, for consistency with Cocoa, as opposed to mbgl’s back-to-front ordering. There are complementary -insertLayer:aboveLayer: and -insertLayer:belowLayer: methods, for consistency with MapKit’s -[MKMapView insertOverlay:aboveOverlay:] and -[MKMapView insertOverlay:belowOverlay:]. Please double-check my index arithmetic to make sure I didn’t make any off-by-one errors. The sources property is a unordered set. A dictionary mapping identifiers to source objects would better conform to the style specification but would encourage some antipatterns we want to avoid; see #7011 (comment) for details.

The mutable layers and sources collections comply with KVC conventions for to-many relationships, which entails additional methods that mbgl::Map and mbgl::style::Style wouldn’t ordinarily provide. These KVC methods could be optimized further with corresponding methods in mbgl; however, memory management would become a lot less clear. For example, who owns an mbgl::style::Layer if it’s already part of a style but is included in an array passed into -[MGLStyle setLayers:]? Rather than muddle the pointer ownership model or implement complex array diffing, this PR sticks to the safest, most straightforward implementation despite a possible performance tradeoff.

As a real-world stress test of KVC compliance, a sidebar has been added to macosapp that displays the layers in the current style and updates the list automatically using Cocoa bindings. The sidebar can be opened and closed via View ‣ Show/Hide Layers or ⇧⌘L. Layers are shown with their identifiers and icons representing their types. Hidden layers are shown in italics. To toggle one or more layers’ visibility, select them and double-click on them. Or use the context menu, which also has an option to delete the selected layers.

Finally, to illustrate one common use of the new APIs, macosapp now has an option to localize all labels in the user’s preferred language, and the Style toolbar button displays the name of the current style when a custom style is shown.

Hiding layers using the new Layers sidebar in macosapp.

Make MGLMapView’s style property KVC-compliant
Make layers API KVC-compliant
Reverse layers to go from front to back
Implement -[MGLStyle insertLayer:aboveLayer:] (reprising [macos, ios] Insert layer above #6730)
Make sources API KVC-compliant
Fix race condition involving change notifications when switching styles
Issue change notifications for layers when adding shape annotations
Add layers sidebar to macosapp
Allow showing/hiding layers sidebar

Fixes #6003, fixes #6096, fixes #6931.

/cc @frederoni @jfirebaugh

jfirebaugh · 2016-08-22T17:13:02Z

include/mbgl/map/map.hpp

 style::Layer* getLayer(const std::string& layerID);
+ void setLayers(std::vector<style::Layer*>&);


Map/Style must have ownership of the layers, so this signature needs to be void setLayers(std::vector<std::unique_ptr<style::Layer>>).

Fixed in e43fbf4.

jfirebaugh · 2016-10-19T16:54:13Z

src/mbgl/map/map.cpp

@@ -794,6 +798,16 @@ style::Layer* Map::getLayer(const std::string& layerID) {
 return nullptr;
 }

+void Map::setLayers(std::vector<std::unique_ptr<style::Layer>> layers) {
+ impl->view.activate();


Needs an if (!impl->style) guard and impl->styleMutated = true; like the other mutators.

jfirebaugh · 2016-10-19T17:01:29Z

platform/darwin/src/MGLStyle.mm

+ std::vector<std::unique_ptr<mbgl::style::Layer>> rawLayers;
+ rawLayers.reserve(layers.count);
+ for (MGLStyleLayer *layer in layers.reverseObjectEnumerator) {
+ rawLayers.push_back(std::unique_ptr<mbgl::style::Layer>(layer.layer));


Constructing a std::unique_ptr from anything other than the result of new is usually incorrect (and even then, you should be using std::make_unique). What's the ownership model of layer.layer?

It’s complicated: #6254. 😕 Maybe we need to tackle that issue first.

To respect ownership of the layers passed into -setLayers: – some of which may exist in the style and some of which may be new – we need to ability to remove a layer from the style and regain ownership of it in order to re-add it. So we’re blocked on mbgl::Map::removeLayer() returning ownership of the layer: #6959.

frederoni · 2016-11-25T16:11:49Z

Great work here @1ec5
This PR will help a lot with debugging and taking runtime styling even further.

jfirebaugh · 2016-11-28T22:40:14Z

src/mbgl/map/map.cpp

 if (impl->style) {
 impl->styleMutated = true;
 return impl->style->getLayer(layerID);
 }
 return nullptr;
 }

+void Map::setLayers(std::vector<std::unique_ptr<Layer>> layers) {
+ if (!impl->style) {
+ impl->styleMutated = true;


This goes in the opposite branch -- we're only mutating the style if we have one.

👍 I ended up not using this method in the SDK due to the memory management conundrum in the PR description. Should I keep this method around to complement getLayers() or remove it?

Yes, please remove it if it's not used.

jfirebaugh · 2016-11-28T22:47:39Z

platform/darwin/src/MGLBackgroundStyleLayer.mm

@@ -51,6 +51,9 @@ - (void)addToMapView:(MGLMapView *)mapView belowLayer:(MGLStyleLayer *)otherLaye
 - (void)removeFromMapView:(MGLMapView *)mapView
 {
 auto removedLayer = mapView.mbglMap->removeLayer(self.identifier.UTF8String);
+ if (!removedLayer) {
+ return;
+ }
 _pendingLayer = std::move(reinterpret_cast<std::unique_ptr<mbgl::style::BackgroundLayer> &>(removedLayer));


Not part of this PR, but this cast is undefined behavior. You need to write something like:

template <class T, class U> std::unique_ptr<T> dynamic_ptr_cast(std::unique_ptr<U> p) { T* t = dynamic_cast<T*>(p.get()); if (!t) return nullptr; p.release(); return t; }

And be aware that this will return a null unique_ptr (and deallocate the input) if the dynamic_cast fails, i.e. if the input is not the anticipated derived type. In this particular code, it looks like this could happen if the user was aliasing *StyleLayer objects while at the same time removing a layer and adding another layer with the same identifier but a different layer type.

Added new layers and sources properties to MGLStyle that contain all the style’s layers and sources, respectively. These properties are KVC-compliant with all the mutable to-many methods. Layers are ordered from topmost to bottommost, for consistency with Cocoa APIs where front/first means top and back/last means bottom. Also added storage for mbgl::style::Source in MGLSource proper for wrapping AnnotationSource. Until the style finishes loading, its name property is set to nil. Fixes #6003.

Replaced -[MGLMapView style] with a property. Keep the MGLStyle object around for the lifetime of the style. Bracket changes to layers in willChange and didChange calls. The built-in point annotation layer is added after the style is finished loading but before the map is finished loading. Cause a second wave of change notifications to go out, about both sources and layers. Issue change notifications for style layers when shape annotations are added or removed.

Fixed a crash that often occurred when switching styles. MGLMapView was ahead of MapDocument in the responder chain, so calls to -setStyle: were going to MGLMapView’s private -setStyle: method. The sender was being passed in the first parameter instead of an MGLStyle method.

Added a sidebar to the main document window in macosapp that lists the layers in the current style and updates whenever the style changes or a layer is added or removed programmatically. Display an icon beside each layer in the sidebar that indicates the kind of layer. Double-click a layer or layers to toggle their visibility, which is an undoable action. Added a menu item and toolbar button to toggle the Layers sidebar. Added a context menu for toggling visibility of and deleting layers selected in the Layers sidebar. Checked in the original SVGs for layer icons.

Display the custom style’s name in the Style toolbar button.

Added a setting to macosapp to change all symbol layers in the current style to use the preferred language if available. If the setting is off, change all symbol layers to use local languages.

1ec5 added iOS Mapbox Maps SDK for iOS refactor macOS Mapbox Maps SDK for macOS Core The cross-platform C++ core, aka mbgl runtime styling labels Aug 19, 2016

1ec5 added this to the ios-v3.4.0 milestone Aug 19, 2016

1ec5 self-assigned this Aug 19, 2016

1ec5 added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Aug 19, 2016

1ec5 force-pushed the 1ec5-style-layers-6003 branch from f0d8a22 to 6f19b2f Compare August 20, 2016 18:48

1ec5 mentioned this pull request Aug 21, 2016

MGLStyle.styleClasses should be an NSSet #6104

Closed

1ec5 changed the title ~~MGLStyle.layers; key-value compliance~~ Key-value compliant layer API Aug 21, 2016

jfirebaugh reviewed Aug 22, 2016
View reviewed changes

1ec5 force-pushed the 1ec5-style-layers-6003 branch from 25f0c27 to 03d59a0 Compare September 9, 2016 21:00

This was referenced Sep 15, 2016

"no such layer" exception after style update #6341

Closed

Notify MGLMapViewDelegate when style loading completes #6412

Closed

This was referenced Sep 22, 2016

Add API for managing a style’s icon images #6436

Closed

Interacting with annotation points in style layer #6505

Closed

1ec5 force-pushed the 1ec5-style-layers-6003 branch 2 times, most recently from b11c4d6 to 40d77ae Compare October 17, 2016 06:56

1ec5 mentioned this pull request Oct 17, 2016

Source-driven attribution #5999

Merged

9 tasks

1ec5 force-pushed the 1ec5-style-layers-6003 branch from 40d77ae to 25f1529 Compare October 17, 2016 11:07

jfirebaugh mentioned this pull request Oct 18, 2016

[macos, ios] Insert layer above #6730

Closed

1ec5 force-pushed the 1ec5-style-layers-6003 branch 2 times, most recently from abe1845 to dc4aef8 Compare October 19, 2016 08:33

jfirebaugh suggested changes Oct 19, 2016

View reviewed changes

incanus mentioned this pull request Oct 21, 2016

MGLVectorStyleLayer.sourceLayerIdentifier is never optional #6780

Closed

boundsj added the release blocker Blocks the next final release label Oct 30, 2016

1ec5 changed the base branch from master to release-ios-v3.4.0 November 10, 2016 10:43

This was referenced Nov 11, 2016

Automatically adapt Mapbox Streets–sourced layers for Dynamic Type #7030

Open

Automatically adapt Mapbox Streets–sourced layers for user preferred language #7031

Closed

1ec5 force-pushed the 1ec5-style-layers-6003 branch from 297337a to a789dd5 Compare November 23, 2016 20:06

1ec5 added the ✓ ready for review label Nov 23, 2016

1ec5 added 2 commits November 23, 2016 16:02

[ios, macos] Added debug descriptions for runtime styling classes

7670444

[ios, macos] Reorganized MGLStyle

f71dd14

1ec5 force-pushed the 1ec5-style-layers-6003 branch from a789dd5 to ec57aea Compare November 24, 2016 00:02

1ec5 force-pushed the 1ec5-style-layers-6003 branch 2 times, most recently from c45be63 to 3dd8910 Compare November 28, 2016 22:00

jfirebaugh suggested changes Nov 28, 2016

View reviewed changes

1ec5 force-pushed the 1ec5-style-layers-6003 branch from 3dd8910 to f331b89 Compare November 28, 2016 23:10

1ec5 mentioned this pull request Nov 28, 2016

-[MGLStyle removeLayer:] relies on undefined behavior #7211

Closed

1ec5 added 6 commits November 28, 2016 15:45

[macos] Name custom style in toolbar

f611355

Display the custom style’s name in the Style toolbar button.

[macos] Added option to localize labels

83e134e

Added a setting to macosapp to change all symbol layers in the current style to use the preferred language if available. If the setting is off, change all symbol layers to use local languages.

1ec5 force-pushed the 1ec5-style-layers-6003 branch from f331b89 to 83e134e Compare November 28, 2016 23:45

jfirebaugh approved these changes Nov 28, 2016

View reviewed changes

1ec5 merged commit 83e134e into release-ios-v3.4.0 Nov 29, 2016

1ec5 deleted the 1ec5-style-layers-6003 branch November 29, 2016 00:04

This was referenced Nov 29, 2016

Add layers property to MGLStyle #6003

Closed

MGLMapView.style is not key-value compliant #6096

Closed

Menu item to toggle native/localized labels #6931

Closed

1ec5 removed the ✓ ready for review label Nov 29, 2016

This was referenced Dec 4, 2016

Check for duplicate layer IDs #7257

Merged

MapboxMap should have methods to list layers and sources #7285

Closed

[ios] Add inline examples in documentation #7337

Merged

This was referenced Dec 18, 2016

Reverse MGLStyle.layers #7474

Closed

MGLStyle.layers and .sources bridge to Swift as untyped collections #7459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGLStyle key-value coding compliance #6097

MGLStyle key-value coding compliance #6097

1ec5 commented Aug 19, 2016 •

edited

Loading

jfirebaugh Aug 22, 2016

1ec5 Oct 19, 2016

jfirebaugh Oct 19, 2016

jfirebaugh Oct 19, 2016

1ec5 Oct 19, 2016

1ec5 Nov 10, 2016

frederoni commented Nov 25, 2016

jfirebaugh Nov 28, 2016

1ec5 Nov 28, 2016

1ec5 Nov 28, 2016

jfirebaugh Nov 28, 2016

1ec5 Nov 28, 2016

jfirebaugh Nov 28, 2016

1ec5 Nov 28, 2016

		style::Layer* getLayer(const std::string& layerID);
		void setLayers(std::vector<style::Layer*>&);

MGLStyle key-value coding compliance #6097

MGLStyle key-value coding compliance #6097

Conversation

1ec5 commented Aug 19, 2016 • 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

frederoni commented Nov 25, 2016

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 commented Aug 19, 2016 •

edited

Loading