Core support for source-driven attribution #6431

1ec5 · 2016-09-22T15:30:19Z

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.

Working towards #2723. Blocks #5999.

/cc @tmpsantos

mention-bot · 2016-09-22T15:30:23Z

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

tmpsantos

Thanks a lot for this!

tmpsantos · 2016-09-22T15:59:42Z

src/mbgl/style/source_observer.hpp

@@ -17,6 +17,7 @@ class SourceObserver {
 virtual ~SourceObserver() = default;

 virtual void onSourceLoaded(Source&) {}
+ virtual void onSourceAttributionChanged(Source&, std::string) {}


const std::string&

tmpsantos · 2016-09-22T16:06:32Z

platform/qt/src/qmapboxgl.cpp

@@ -76,6 +76,7 @@ static_assert(mbgl::underlying_type(QMapboxGL::MapChangeDidFinishRenderingFrameF
 static_assert(mbgl::underlying_type(QMapboxGL::MapChangeWillStartRenderingMap) == mbgl::underlying_type(mbgl::MapChangeWillStartRenderingMap), "error");
 static_assert(mbgl::underlying_type(QMapboxGL::MapChangeDidFinishRenderingMap) == mbgl::underlying_type(mbgl::MapChangeDidFinishRenderingMap), "error");
 static_assert(mbgl::underlying_type(QMapboxGL::MapChangeDidFinishRenderingMapFullyRendered) == mbgl::underlying_type(mbgl::MapChangeDidFinishRenderingMapFullyRendered), "error");
+static_assert(mbgl::underlying_type(QMapboxGL::MapChangeSourceAttributionDidChange) == mbgl::underlying_type(mbgl::MapChangeSourceAttributionDidChange), "error");


This static_assert will fail because we did not add yet MapChangeDidFinishLoadingStyle on Qt. Can you add that one too?

I added it here, since MapChangeDidFinishRenderingMapFullyRendered was also defined there. Do I need to add it anywhere else?

Do I need to add it anywhere else?

That is not it, this code won't compile on Qt because we are missing the MapChangeDidFinishLoadingStyle signal on the API that must be declared before MapChangeSourceAttributionDidChange otherwise the enum won't fully match the counterpart which is exactly what the static_assert is asserting.

Ah, now I understand. Done in c1ef372. (Something similar is still missing from the iOS and macOS SDKs: #6412.)

tmpsantos · 2016-09-22T16:07:24Z

src/mbgl/style/style.cpp

+ style::TileSourceImpl* tileSource = static_cast<style::TileSourceImpl*>(source->baseImpl.get());
+ auto attribution = tileSource->getAttribution();
+ if (!attribution.empty()) {
+ result.push_back(attribution);


std::move(attribution)

tmpsantos · 2016-09-22T16:07:46Z

src/mbgl/style/style.cpp

@@ -465,6 +489,10 @@ void Style::onSourceLoaded(Source& source) {
 observer->onUpdate(Update::Repaint);
 }

+void Style::onSourceAttributionChanged(Source& source, std::string attribution) {


const std::string&

tmpsantos · 2016-09-22T16:08:18Z

test/src/mbgl/test/stub_style_observer.hpp

@@ -30,6 +30,10 @@ class StubStyleObserver : public style::Observer {
 if (sourceLoaded) sourceLoaded(source);
 }

+ void onSourceAttributionChanged(Source& source, std::string attribution) override {


const std::string&

jfirebaugh

Instead of adding Map::getAttributions() and Style::getAttributions(), can we add Source::getAttribution()? That leverages the existing runtime source API better.

1ec5 · 2016-09-22T17:13:13Z

Instead of adding Map::getAttributions() and Style::getAttributions(), can we add Source::getAttribution()? That leverages the existing runtime source API better.

That would make a lot of sense, though it means we’ll need more infrastructure for bubbling source-related changes up to the SDK level. The macOS implementation of MGLMapView, for instance, would need to register itself as a source observer as sources are added and removed in order to accomplish the dynamically updating attribution control in #5999. (Presumably the Qt SDK would have to do likewise, since an on-demand attribution control is only an option for mobile UIs.) How will this fit into the MapChange revamp proposed in #6383?

tmpsantos · 2016-09-22T18:41:29Z

platform/qt/include/qmapboxgl.hpp

@@ -111,7 +111,8 @@ class Q_DECL_EXPORT QMapboxGL : public QObject
 MapChangeDidFinishRenderingFrameFullyRendered,
 MapChangeWillStartRenderingMap,
 MapChangeDidFinishRenderingMap,
- MapChangeDidFinishRenderingMapFullyRendered
+ MapChangeDidFinishRenderingMapFullyRendered,
+ MapChangeSourceAttributionDidChange


This code will fail on the static_assert because we don't have MapChangeDidFinishLoadingStyle before MapChangeSourceAttributionDidChange.

This function can be called from different threads and was depending on a global `std::string protocol` which can do heap allocations. The first thread to use it would initialized it which is not exactly safe, so this patch moves it to a `char *` that is just a pointer to the data segment, so no races. Valgrind detected it on #6431. ``` ==9874== Conditional jump or move depends on uninitialised value(s) ==9874== 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) ==9874== by 0x79896E: mbgl::util::mapbox::isMapboxURL(std::string const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x6DC756: 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) ==9874== by 0x6DDA6A: std::_Function_handler<void (mbgl::Response), mbgl::style::TileSourceImpl::loadDescription(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) ==9874== by 0x50BE80: std::_Function_handler<void (), mbgl::StubFileSource::StubFileSource()::{lambda()#1}>::_M_invoke(std::_Any_data const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x70972A5: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x70A3621: QTimer::timerEvent(QTimerEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x7098053: QObject::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x60CCC8B: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.2.1) ==9874== by 0x60D1E55: QApplication::notify(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.2.1) ==9874== by 0x706FC2C: QCoreApplication::notifyInternal(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x70BC1AC: QTimerInfoList::activateTimers() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== ```

tmpsantos · 2016-09-26T09:49:21Z

@1ec5 seems like valgrind found an issue (not exactly related to this PR). I have a fix on #6455.

This function can be called from different threads and was depending on a global `std::string protocol` which can do heap allocations. The first thread to use it would initialized it which is not exactly safe, so this patch moves it to a `char *` that is just a pointer to the data segment, so no races. Valgrind detected it on #6431. ``` ==9874== Conditional jump or move depends on uninitialised value(s) ==9874== 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) ==9874== by 0x79896E: mbgl::util::mapbox::isMapboxURL(std::string const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x6DC756: 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) ==9874== by 0x6DDA6A: std::_Function_handler<void (mbgl::Response), mbgl::style::TileSourceImpl::loadDescription(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) ==9874== by 0x50BE80: std::_Function_handler<void (), mbgl::StubFileSource::StubFileSource()::{lambda()#1}>::_M_invoke(std::_Any_data const&) (in /home/travis/build/mapbox/mapbox-gl-native/build/qt-linux-x86_64/Release/mbgl-test) ==9874== by 0x70972A5: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x70A3621: QTimer::timerEvent(QTimerEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x7098053: QObject::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x60CCC8B: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.2.1) ==9874== by 0x60D1E55: QApplication::notify(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.2.1) ==9874== by 0x706FC2C: QCoreApplication::notifyInternal(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== by 0x70BC1AC: QTimerInfoList::activateTimers() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.2.1) ==9874== ```

tmpsantos · 2016-09-26T17:44:18Z

I have a fix on #6455.

Fix landed, bots should be 🍏 if you rebase.

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. Fixes #2723.

jfirebaugh · 2016-09-26T18:54:31Z

Hmm, I guess I should have disapproved the PR. I don't want to commit the core API to Map::getAttributions().

1ec5 added feature GL JS parity For feature parity with Mapbox GL JS Core The cross-platform C++ core, aka mbgl labels Sep 22, 2016

1ec5 self-assigned this Sep 22, 2016

1ec5 mentioned this pull request Sep 22, 2016

Source-driven attribution #5999

Merged

9 tasks

tmpsantos suggested changes Sep 22, 2016

View reviewed changes

1ec5 force-pushed the 1ec5-attribution-callback-core-5999 branch from 5a26cd4 to 147cd9d Compare September 22, 2016 16:50

jfirebaugh reviewed Sep 22, 2016

View reviewed changes

tmpsantos reviewed Sep 22, 2016

View reviewed changes

tmpsantos approved these changes Sep 22, 2016

View reviewed changes

tmpsantos mentioned this pull request Sep 26, 2016

[core] Fix initialization race on util::mapbox::isMapboxURL #6455

Merged

1ec5 added the Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL label Sep 26, 2016

[core] Source-driven attribution

6433cc7

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. Fixes #2723.

1ec5 force-pushed the 1ec5-attribution-callback-core-5999 branch from c1ef372 to 6433cc7 Compare September 26, 2016 18:09

1ec5 merged commit f307652 into master Sep 26, 2016

1ec5 deleted the 1ec5-attribution-callback-core-5999 branch September 26, 2016 18:37

1ec5 mentioned this pull request Sep 26, 2016

Clean up source-driven attribution #6465

Merged

1ec5 mentioned this pull request Nov 29, 2016

Attribution control on Qt #6000

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core support for source-driven attribution #6431

Core support for source-driven attribution #6431

1ec5 commented Sep 22, 2016

mention-bot commented Sep 22, 2016

tmpsantos left a comment

tmpsantos Sep 22, 2016

1ec5 Sep 22, 2016

tmpsantos Sep 22, 2016

1ec5 Sep 22, 2016

tmpsantos Sep 22, 2016

1ec5 Sep 22, 2016

tmpsantos Sep 22, 2016

1ec5 Sep 22, 2016

tmpsantos Sep 22, 2016

1ec5 Sep 22, 2016

tmpsantos Sep 22, 2016

1ec5 Sep 22, 2016

jfirebaugh left a comment

1ec5 commented Sep 22, 2016

tmpsantos Sep 22, 2016

tmpsantos commented Sep 26, 2016

tmpsantos commented Sep 26, 2016

jfirebaugh commented Sep 26, 2016

Core support for source-driven attribution #6431

Core support for source-driven attribution #6431

Conversation

1ec5 commented Sep 22, 2016

mention-bot commented Sep 22, 2016

tmpsantos 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

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

jfirebaugh left a comment

Choose a reason for hiding this comment

1ec5 commented Sep 22, 2016

Choose a reason for hiding this comment

tmpsantos commented Sep 26, 2016

tmpsantos commented Sep 26, 2016

jfirebaugh commented Sep 26, 2016