diff options
author | John Firebaugh <john.firebaugh@gmail.com> | 2016-07-21 16:37:18 -0700 |
---|---|---|
committer | John Firebaugh <john.firebaugh@gmail.com> | 2016-08-23 09:55:32 -0700 |
commit | 86cccc4d24804d40ea9d15d47ee1edb975bbf2e3 (patch) | |
tree | 7613b3a6546c4fb48c3f73069bbdde47e6a03e4d | |
parent | 5511e9a4e85c8eb8a67a8cfc56a2b1f665a8940d (diff) | |
download | qtlocation-mapboxgl-86cccc4d24804d40ea9d15d47ee1edb975bbf2e3.tar.gz |
[core] Don't allow style mutations to be overwritten by revalidation
* Once we get a fresh style, stop revalidating.
* If the style is mutated, stop revalidating and preserve the existing mutations.
-rw-r--r-- | include/mbgl/storage/response.hpp | 4 | ||||
-rw-r--r-- | src/mbgl/map/map.cpp | 20 | ||||
-rw-r--r-- | test/map/map.cpp | 64 | ||||
-rw-r--r-- | test/src/mbgl/test/fake_file_source.hpp | 60 |
4 files changed, 148 insertions, 0 deletions
diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp index 6c79f5e181..4dc3f36681 100644 --- a/include/mbgl/storage/response.hpp +++ b/include/mbgl/storage/response.hpp @@ -32,6 +32,10 @@ public: optional<Timestamp> modified; optional<Timestamp> expires; optional<std::string> etag; + + bool isFresh() const { + return !expires || *expires > util::now(); + } }; class Response::Error { diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index 15f7779f83..2e7dc06e8b 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -67,6 +67,7 @@ public: std::string styleURL; std::string styleJSON; + bool styleMutated = false; std::unique_ptr<AsyncRequest> styleRequest; @@ -293,10 +294,21 @@ void Map::setStyleURL(const std::string& url) { impl->styleRequest = nullptr; impl->styleURL = url; impl->styleJSON.clear(); + impl->styleMutated = false; impl->style = std::make_unique<Style>(impl->fileSource, impl->pixelRatio); impl->styleRequest = impl->fileSource.request(Resource::style(impl->styleURL), [this](Response res) { + // Once we get a fresh style, or the style is mutated, stop revalidating. + if (res.isFresh() || impl->styleMutated) { + impl->styleRequest.reset(); + } + + // Don't allow a loaded, mutated style to be overwritten with a new version. + if (impl->styleMutated && impl->style->loaded) { + return; + } + if (res.error) { if (res.error->reason == Response::Error::Reason::NotFound && util::mapbox::isMapboxURL(impl->styleURL)) { @@ -323,6 +335,8 @@ void Map::setStyleJSON(const std::string& json) { impl->styleURL.clear(); impl->styleJSON.clear(); + impl->styleMutated = false; + impl->style = std::make_unique<Style>(impl->fileSource, impl->pixelRatio); impl->loadStyleJSON(json); @@ -746,22 +760,27 @@ AnnotationIDs Map::queryPointAnnotations(const ScreenBox& box) { #pragma mark - Style API style::Source* Map::getSource(const std::string& sourceID) { + impl->styleMutated = true; return impl->style ? impl->style->getSource(sourceID) : nullptr; } void Map::addSource(std::unique_ptr<style::Source> source) { + impl->styleMutated = true; impl->style->addSource(std::move(source)); } void Map::removeSource(const std::string& sourceID) { + impl->styleMutated = true; impl->style->removeSource(sourceID); } style::Layer* Map::getLayer(const std::string& layerID) { + impl->styleMutated = true; return impl->style ? impl->style->getLayer(layerID) : nullptr; } void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& before) { + impl->styleMutated = true; impl->view.activate(); impl->style->addLayer(std::move(layer), before); @@ -772,6 +791,7 @@ void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& be } void Map::removeLayer(const std::string& id) { + impl->styleMutated = true; impl->view.activate(); impl->style->removeLayer(id); diff --git a/test/map/map.cpp b/test/map/map.cpp index 1f6e59f3ea..79c1993cb4 100644 --- a/test/map/map.cpp +++ b/test/map/map.cpp @@ -1,5 +1,6 @@ #include <mbgl/test/util.hpp> #include <mbgl/test/stub_file_source.hpp> +#include <mbgl/test/fake_file_source.hpp> #include <mbgl/test/fixture_log_observer.hpp> #include <mbgl/map/map.hpp> @@ -80,6 +81,69 @@ TEST(Map, DoubleStyleLoad) { map.setStyleJSON(""); } +TEST(Map, StyleFresh) { + // The map should not revalidate fresh styles. + + MapTest test; + FakeFileSource fileSource; + + Map map(test.view, fileSource, MapMode::Still); + map.setStyleURL("mapbox://styles/test"); + EXPECT_EQ(1u, fileSource.requests.size()); + + Response response; + response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/empty.json")); + response.expires = Timestamp::max(); + + fileSource.respond(Resource::Style, response); + EXPECT_EQ(0u, fileSource.requests.size()); +} + +TEST(Map, StyleExpired) { + // The map should allow expired styles to be revalidated, so long as no mutations are made. + + using namespace std::chrono_literals; + + MapTest test; + FakeFileSource fileSource; + + Map map(test.view, fileSource, MapMode::Still); + map.setStyleURL("mapbox://styles/test"); + EXPECT_EQ(1u, fileSource.requests.size()); + + Response response; + response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/empty.json")); + response.expires = util::now() - 1h; + + fileSource.respond(Resource::Style, response); + EXPECT_EQ(1u, fileSource.requests.size()); + + map.addLayer(std::make_unique<style::BackgroundLayer>("bg")); + EXPECT_EQ(1u, fileSource.requests.size()); + + fileSource.respond(Resource::Style, response); + EXPECT_EQ(0u, fileSource.requests.size()); + EXPECT_NE(nullptr, map.getLayer("bg")); +} + +TEST(Map, StyleEarlyMutation) { + // An early mutation should not prevent the initial style load. + + MapTest test; + FakeFileSource fileSource; + + Map map(test.view, fileSource, MapMode::Still); + map.setStyleURL("mapbox://styles/test"); + map.addLayer(std::make_unique<style::BackgroundLayer>("bg")); + + Response response; + response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/water.json")); + fileSource.respond(Resource::Style, response); + + EXPECT_EQ(0u, fileSource.requests.size()); + EXPECT_NE(nullptr, map.getLayer("water")); +} + TEST(Map, AddLayer) { MapTest test; diff --git a/test/src/mbgl/test/fake_file_source.hpp b/test/src/mbgl/test/fake_file_source.hpp new file mode 100644 index 0000000000..7ebb4cf0cb --- /dev/null +++ b/test/src/mbgl/test/fake_file_source.hpp @@ -0,0 +1,60 @@ +#pragma once + +#include <mbgl/storage/file_source.hpp> + +#include <list> + +namespace mbgl { + +/* + FakeFileSource is similar to StubFileSource, but it follows a post hoc, "push" model rather than + a pre hoc, "pull" model. You pass it to code that makes requests, it records what requests are + made, then you can examine them, make assertions about them, and respond as desired. + + This is particularly useful if you want to simulate multiple responses, e.g. as part of a resource + revalidation flow. StubFileSource allows only a single response. +*/ +class FakeFileSource : public FileSource { +public: + class FakeFileRequest : public AsyncRequest { + public: + Resource resource; + Callback callback; + + std::list<FakeFileRequest*>& list; + std::list<FakeFileRequest*>::iterator link; + + FakeFileRequest(Resource resource_, Callback callback_, std::list<FakeFileRequest*>& list_) + : resource(std::move(resource_)), + callback(std::move(callback_)), + list(list_), + link((list.push_back(this), std::prev(list.end()))) { + } + + ~FakeFileRequest() override { + list.erase(link); + } + }; + + std::unique_ptr<AsyncRequest> request(const Resource& resource, Callback callback) override { + return std::make_unique<FakeFileRequest>(resource, callback, requests); + } + + bool respond(Resource::Kind kind, const Response& response) { + auto it = std::find_if(requests.begin(), requests.end(), [&] (FakeFileRequest* request) { + return request->resource.kind == kind; + }); + + if (it != requests.end()) { + // Copy the callback, in case calling it deallocates the AsyncRequest. + Callback callback_ = (*it)->callback; + callback_(response); + } + + return it != requests.end(); + } + + std::list<FakeFileRequest*> requests; +}; + +} // namespace mbgl |