Source-driven attribution #5999

1ec5 · 2016-08-13T18:34:09Z

The attribution view on macOS and the ℹ️ action sheet on iOS now display the correct attribution based on all the sources used in the current style, instead of the Mapbox and OpenStreetMap attribution that was previously hard-coded in each implementation of MGLMapView. This means DigitalGlobe is credited for Mapbox Satellite and Satellite Streets, and attribution for third-party sources added in Mapbox Studio or using the runtime styling API is also shown.

The attribution view on macOS updates immediately whenever the style changes and a new source is loaded. Developers can (should) now hook up an Improve This Map menu item to a new giveFeedback: action on MGLMapView, which opens in a Web browser any feedback URL it finds in a source’s attribution HTML code.

The ℹ️ action sheet on iOS fetches the current style’s attribution information on demand.

Implemented observer callbacks so the style knows when the source’s attribution changes and the map knows when the style’s attribution changes. Also implemented a getter for a tile source’s attribution. Split out as #6431.

More to come:

~~Attribution observer callbacks~~ (split out as Core support for source-driven attribution #6431)
~~Attribution accessors~~
Parse attribution HTML source code into structured data
Tests for parsing attribution strings
Dedupe attribution strings
Tests for deduping attribution strings
Update macOS attribution view dynamically (fixes the macOS side of Derive attribution from sources in current style #2723)
Use dynamic attribution in iOS attribution action sheet (fixes the iOS side of Derive attribution from sources in current style #2723)
Replace built-in “Improve This Map” link with customized/localized link from style (fixes Move Improve This Map to macOS SDK #5775)

Depends on #6431.

/cc @kkaefer @friedbunny

friedbunny · 2016-08-13T20:23:25Z

This is a great change. 👍

Should the feedback item be contingent on the current style being Mapbox-hosted?

I also keep coming back to the UI, but adding more entries to the action sheet is something I think we should try to limit or avoid. The items that exist there now are all very important (and are all presented as equally important), but adding an arbitrary number of new items dilutes existing ones.

If a map were to have many sources and/or many attributions, this list could quickly grow out of control. A custom view is one way around this, but we could also consider following the current telemetry opt-out alert style: add a generic “Map data providers” item to the action sheet, which presents the user with an alert with items that link to the different map data providers.

1ec5 · 2016-08-13T20:38:34Z

Should the feedback item be contingent on the current style being Mapbox-hosted?

This branch currently filters out the feedback link coming from TileJSON in favor of the built-in feedback item. On iOS, we could definitely drop the built-in item in favor of the TileJSON one, but we’ll need to customize the URL based on the current viewport and localize the item title (or at least title-case it).

On macOS, the built-in “Improve This Map” item belongs in the Help menu. As part of upstreaming the item from macosapp to the macOS SDK (#5775), we could enable and disable the menu item based on the link’s presence in TileJSON. However, I’m still unsure how the SDK would even automatically add a menu item in the first place.

adding an arbitrary number of new items dilutes existing ones

Agreed. However, in practice, the number of attribution links in a source is always kept to a minimum in order to fit horizontally on a standard webpage. For parity with GL JS, I’ve implemented deduping logic in the iOS/macOS SDK to avoid showing redundant items when, for instance, multiple Mapbox-copyrighted sources are being used. Among the default styles, Satellite and Satellite Streets are currently the worst case:

(On iOS, “Improve This Map” would swap places with the DigitalGlobe attribution if we replace the built-in feedback item with the one coming from TileJSON.)

A single “Map Data Providers” item would be more elegant, but it would divorce attribution even further from the map view. UI design changes will require considerable thought, so let’s hash that out in a separate PR.

1ec5 · 2016-08-14T04:40:11Z

macOS now supports rich formatting besides links, including inline images:

1ec5 · 2016-08-14T19:10:49Z

The Darwin unit tests are failing on iOS (but not macOS) due to network issues while running -[MGLOfflineStorageTests testAAALoadPacks]:

2016-08-14 12:08:30.022 xctest[78390:1809420] [ERROR] {}[Setup]: loading style failed: HTTP status code 401

I’m not sure why initializing an MGLOfflineStorage would trigger network requests, and none of this is related to the changes in this branch.

1ec5 · 2016-08-14T19:17:24Z

Tracking the iOS test failure in #6006.

tmpsantos · 2016-08-16T15:26:24Z

src/mbgl/style/style.cpp

@@ -240,6 +251,26 @@ Source* Style::getSource(const std::string& id) const {
    return it != sources.end() ? it->get() : nullptr;
 }

+std::vector<std::string> Style::getAttributions() const {
+    std::vector<std::string> result;
+    result.reserve(sources.size());


Not needed IMO.

To clarify, this will always allocated more memory than we need since we always have a annotation source that doesn't have attribution.

tmpsantos · 2016-08-16T15:29:05Z

👍

tmpsantos · 2016-08-16T15:31:01Z

include/mbgl/map/map.hpp

@@ -149,9 +149,11 @@ class Map : private util::noncopyable {
    void removeAnnotation(AnnotationID);

    // Sources
+    std::vector<const style::Source*> getSources() const;


We don't need this API yet, do we?

tmpsantos · 2016-08-16T15:38:06Z

src/mbgl/map/map.cpp

@@ -861,6 +870,10 @@ void Map::onLowMemory() {
    impl->view.invalidate();
 }

+void Map::Impl::onSourceAttributionChanged(style::Source&, std::string) {


Why the parameters if not in use?

I struggled with this one. Semantically, I think it makes plenty of sense for the string to go along with the callback, for consistency with the other callbacks. But we aren’t currently plumbing the string all the way through because the notifyMapChange() mechanism doesn’t support additional data beyond a single MapChange enum.

1ec5 · 2016-08-22T03:28:56Z

A Valgrind failure on Qt appears to have been uncovered by the addition of a simple unit test:

==10075== Conditional jump or move depends on uninitialised value(s)

==10075==    at 0x4C307C2: __memcmp_sse4_1 (in /home/travis/build/mapbox/mapbox-gl-native/mason_packages/linux-x86_64/valgrind/latest/lib/valgrind/vgpreload_memcheck-amd64-linux.so)

==10075==    by 0x75AB4E: mbgl::util::mapbox::isMapboxURL(std::string const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test)

==10075==    by 0x6A8259: mbgl::style::TileSourceImpl::parseTileJSON(std::string const&, std::string const&, mbgl::SourceType, unsigned short) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test)

==10075==    by 0x6A9F1D: std::_Function_handler<void (mbgl::Response), mbgl::style::TileSourceImpl::load(mbgl::FileSource&)::{lambda(mbgl::Response)#1}>::_M_invoke(std::_Any_data const&, mbgl::Response&&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test)

The test uses test as a tile URL, just like some of the other Source tests.

/cc @brunoabinader @tmpsantos

1ec5 · 2016-12-04T08:05:26Z

Feedback links are now derived from the current style’s sources instead of being hard-coded, as suggested in #5999 (comment). So in iOS applications displaying the Mapbox Satellite Streets style, the Improve This Map button in the ℹ️ sheet appears between the OpenStreetMap and DigitalGlobe attribution buttons. macOS applications can easily create a similar menu item by hooking it up to the first responder’s giveFeedback: action, which MGLMapView validates.

This PR is ready for review.

friedbunny

Thanks for the facetalk walkthrough!

Refactored MGLSource initialization. Implemented a new private class, MGLAttributionInfo, that parses an HTML attribution string from TileJSON and stores the resulting structured data. Added methods to MGLTileSet and MGLStyle to aggregate MGLAttributionInfos. On macOS, update the attribution view as soon as the source attribution changes. On iOS, fetch the current attribution information when displaying the action sheet. Removed hard-coded attribution strings.

Apply a default font and color to attribution HTML as it is imported into an attributed string. Pass the attributed string into MGLAttributionButton as is, without stripping formatting. Avoid overriding the font and color after importing the HTML, in case these attributes are explicitly specified rather than intrinsic to a hyperlink. Constrain the top of the attribution view to all the attribution buttons, in case one of them needs additional headspace.

Unlinked attribution strings are represented on macOS as buttons that have the default cursor and do nothing when clicked. On iOS, they are action sheet buttons that do nothing but dismiss the action sheet.

Auto Layout randomly finds itself unable to satisfy constraints when updating attribution, due to some spurious constraints between attribution buttons. Regenerate the entire attribution view every time the source attribution changes.

Also added a test to verify parity with the GL JS implementation. This implementation avoids sorting.

Also added parsing tests.

Included an emoji test to ensure that attribution strings are interpreted as UTF-8, to avoid mojibake. Included a test of removing the underline from a leading copyright symbol.

MGLAttributionInfo now detects feedback links in the attribution HTML code, and it is responsible for tailoring the feedback URL to the current viewport. Removed the hard-coded feedback action from the attribution sheet on iOS in favor of a source-derived feedback title and URL. Moved the feedback action from macosapp to MGLMapView; applications are now expected to hook an Improve This Map menu item to an MGLMapView action.

1ec5 · 2016-12-09T01:24:56Z

I was thinking about making MGLAttributionInfo public and turning MGLTileSet.attribution into an array of them, but I’ll address that separately in #7361.

* release-ios-v3.4.0: [ios] Fixed crash on launch in iosapp [ios] Fix iosapp runtime styling examples (#7395) [ios, macos] handle duplicate layer error [core] guard against duplicate layer ids [core] use raii to guard backend deactivation [ios, macos] Override references to property names in property requirements lists (#7391) [ios, macos] Load features into shape source if possible (#7339) [ios, macos] Expanded source documentation [ios, macos] Rename MGLGeoJSONSource to MGLShapeSource (#7334) [ios, macos] Silence -Wc++11-narrowing warning in open gl layer (#7355) [ios, macos] Source-driven attribution (#5999) [ios, macos] renamed raster-hue-rotate # Conflicts: # platform/darwin/src/MGLRasterSource.h # platform/darwin/src/MGLShapeSource.h # platform/darwin/src/MGLSource.h # platform/darwin/src/MGLVectorSource.h

* release-ios-v3.4.0: [ios] Fixed crash on launch in iosapp [ios] Fix iosapp runtime styling examples (#7395) [ios, macos] handle duplicate layer error [core] guard against duplicate layer ids [core] use raii to guard backend deactivation [ios, macos] Override references to property names in property requirements lists (#7391) [ios, macos] Load features into shape source if possible (#7339) [ios, macos] Expanded source documentation [ios, macos] Rename MGLGeoJSONSource to MGLShapeSource (#7334) [ios, macos] Silence -Wc++11-narrowing warning in open gl layer (#7355) [ios, macos] Source-driven attribution (#5999) [ios, macos] renamed raster-hue-rotate

* release-ios-v3.4.0: [ios] Fixed crash on launch in iosapp [ios] Fix iosapp runtime styling examples (#7395) [ios, macos] handle duplicate layer error [core] guard against duplicate layer ids [core] use raii to guard backend deactivation [ios, macos] Override references to property names in property requirements lists (#7391) [ios, macos] Load features into shape source if possible (#7339) [ios, macos] Expanded source documentation [ios, macos] Rename MGLGeoJSONSource to MGLShapeSource (#7334) [ios, macos] Silence -Wc++11-narrowing warning in open gl layer (#7355) [ios, macos] Source-driven attribution (#5999) [ios, macos] renamed raster-hue-rotate # Conflicts: # platform/ios/jazzy.yml

1ec5 added feature GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Core The cross-platform C++ core, aka mbgl labels Aug 13, 2016

1ec5 self-assigned this Aug 13, 2016

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

1ec5 mentioned this pull request Aug 13, 2016

Derive attribution from sources in current style #2723

Closed

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

1ec5 force-pushed the 1ec5-attribution-callback-2723 branch 2 times, most recently from a73a191 to 1ecee06 Compare August 16, 2016 08:46

tmpsantos mentioned this pull request Aug 16, 2016

Attribution control on Qt #6000

Closed

tmpsantos reviewed Aug 16, 2016
View reviewed changes

1ec5 force-pushed the 1ec5-attribution-callback-2723 branch from 1ecee06 to db19063 Compare August 16, 2016 15:34

tmpsantos reviewed Aug 16, 2016
View reviewed changes

1ec5 force-pushed the 1ec5-attribution-callback-2723 branch 2 times, most recently from 3b57a31 to df3e47b Compare August 22, 2016 03:01

1ec5 force-pushed the 1ec5-attribution-callback-2723 branch from 6584f17 to 23acd17 Compare December 8, 2016 10:16

1ec5 requested a review from friedbunny December 8, 2016 10:17

friedbunny approved these changes Dec 8, 2016

View reviewed changes

1ec5 modified the milestones: ios-v3.4.0, ios-3.4.1 Dec 8, 2016

1ec5 added 8 commits December 8, 2016 14:20

[ios, macos] Display unlinked attribution strings

567692b

Unlinked attribution strings are represented on macOS as buttons that have the default cursor and do nothing when clicked. On iOS, they are action sheet buttons that do nothing but dismiss the action sheet.

[macos] Fixed random Auto Layout exception

8e2852a

Auto Layout randomly finds itself unable to satisfy constraints when updating attribution, due to some spurious constraints between attribution buttons. Regenerate the entire attribution view every time the source attribution changes.

[ios, macos] Thoroughly dedupe attribution infos

9d8036b

Also added a test to verify parity with the GL JS implementation. This implementation avoids sorting.

[ios, macos] Trim whitespace from attribution strings

b85d48e

Also added parsing tests.

[ios, macos] Added attribution parsing tests for styles

1c3138e

Included an emoji test to ensure that attribution strings are interpreted as UTF-8, to avoid mojibake. Included a test of removing the underline from a leading copyright symbol.

1ec5 mentioned this pull request Dec 9, 2016

Fold MGLTileSet into MGLRasterSource and MGLVectorSource #7360

Closed

1ec5 force-pushed the 1ec5-attribution-callback-2723 branch from 23acd17 to 33c7470 Compare December 9, 2016 01:15

1ec5 mentioned this pull request Dec 9, 2016

Source attribution should be object-oriented #7361

Closed

1ec5 merged commit fe53af7 into release-ios-v3.4.0 Dec 9, 2016

1ec5 deleted the 1ec5-attribution-callback-2723 branch December 9, 2016 01:43

This was referenced Dec 9, 2016

Move Improve This Map to macOS SDK #5775

Closed

Simplified Chinese localization #7371

Closed

Attribution is incorrectly capitalized #7409

Closed

1ec5 removed the ✓ ready for review label Dec 21, 2016

1ec5 mentioned this pull request Jan 7, 2017

Exception zooming in using keyboard #7623

Closed

1ec5 mentioned this pull request Apr 4, 2017

[android] - derived source attribution #8630

Merged

1ec5 mentioned this pull request Jun 9, 2017

Make feedback button localizable #9215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source-driven attribution #5999

Source-driven attribution #5999

1ec5 commented Aug 13, 2016 •

edited

Loading

friedbunny commented Aug 13, 2016

1ec5 commented Aug 13, 2016 •

edited

Loading

1ec5 commented Aug 14, 2016

1ec5 commented Aug 14, 2016

1ec5 commented Aug 14, 2016

tmpsantos Aug 16, 2016

tmpsantos Aug 16, 2016

tmpsantos commented Aug 16, 2016

tmpsantos Aug 16, 2016

tmpsantos Aug 16, 2016

1ec5 Aug 16, 2016

1ec5 commented Aug 22, 2016 •

edited

Loading

1ec5 commented Dec 4, 2016 •

edited

Loading

friedbunny left a comment

1ec5 commented Dec 9, 2016

Source-driven attribution #5999

Source-driven attribution #5999

Conversation

1ec5 commented Aug 13, 2016 • edited Loading

friedbunny commented Aug 13, 2016

1ec5 commented Aug 13, 2016 • edited Loading

1ec5 commented Aug 14, 2016

1ec5 commented Aug 14, 2016

1ec5 commented Aug 14, 2016

tmpsantos Aug 16, 2016

Choose a reason for hiding this comment

tmpsantos Aug 16, 2016

Choose a reason for hiding this comment

tmpsantos commented Aug 16, 2016

tmpsantos Aug 16, 2016

Choose a reason for hiding this comment

tmpsantos Aug 16, 2016

Choose a reason for hiding this comment

1ec5 Aug 16, 2016

Choose a reason for hiding this comment

1ec5 commented Aug 22, 2016 • edited Loading

1ec5 commented Dec 4, 2016 • edited Loading

friedbunny left a comment

Choose a reason for hiding this comment

1ec5 commented Dec 9, 2016

1ec5 commented Aug 13, 2016 •

edited

Loading

1ec5 commented Aug 13, 2016 •

edited

Loading

1ec5 commented Aug 22, 2016 •

edited

Loading

1ec5 commented Dec 4, 2016 •

edited

Loading