From c98f4f476bd590bdfd78409d428459e40347302a Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Wed, 24 Aug 2016 17:38:06 +0300 Subject: [PATCH] [core] Emit MapChangeDidFailLoadingMap when the style cannot be loaded or parsed Currently this signal is never emitted, which can cause the Still mode to starve in case of an invalid style or failed request. --- src/mbgl/map/map.cpp | 9 ++++++++- src/mbgl/style/observer.hpp | 1 + src/mbgl/style/parser.cpp | 15 ++++++++++----- src/mbgl/style/parser.hpp | 5 ++++- src/mbgl/style/style.cpp | 10 +++++++++- test/fixtures/style_parser/non-object.info.json | 4 ++-- test/map/map.cpp | 2 +- test/style/style_parser.cpp | 7 ++++++- 8 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index 2e7dc06e8..3a307735a 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -38,6 +38,7 @@ public: Impl(View&, FileSource&, MapMode, GLContextMode, ConstrainMode, ViewportMode); void onNeedsRepaint() override; + void onStyleError() override; void onResourceError(std::exception_ptr) override; void update(); @@ -316,6 +317,8 @@ void Map::setStyleURL(const std::string& url) { } else { Log::Error(Event::Setup, "loading style failed: %s", res.error->message.c_str()); } + impl->onStyleError(); + impl->onResourceError(std::make_exception_ptr(std::runtime_error(res.error->message))); } else if (res.notModified || res.noContent) { return; } else { @@ -343,8 +346,8 @@ void Map::setStyleJSON(const std::string& json) { } void Map::Impl::loadStyleJSON(const std::string& json) { - style->setJSON(json); style->setObserver(this); + style->setJSON(json); styleJSON = json; // force style cascade, causing all pending transitions to complete. @@ -908,6 +911,10 @@ void Map::Impl::onNeedsRepaint() { asyncUpdate.send(); } +void Map::Impl::onStyleError() { + view.notifyMapChange(MapChangeDidFailLoadingMap); +} + void Map::Impl::onResourceError(std::exception_ptr error) { if (mode == MapMode::Still && callback) { callback(error, {}); diff --git a/src/mbgl/style/observer.hpp b/src/mbgl/style/observer.hpp index 2c4811466..c4d31ae24 100644 --- a/src/mbgl/style/observer.hpp +++ b/src/mbgl/style/observer.hpp @@ -18,6 +18,7 @@ public: * and `onNeedsRepaint` will be called. */ void onNeedsRepaint() override {} + virtual void onStyleError() {} virtual void onResourceError(std::exception_ptr) {} }; diff --git a/src/mbgl/style/parser.cpp b/src/mbgl/style/parser.cpp index bc05b5484..e53fac266 100644 --- a/src/mbgl/style/parser.cpp +++ b/src/mbgl/style/parser.cpp @@ -14,24 +14,27 @@ #include #include +#include namespace mbgl { namespace style { Parser::~Parser() = default; -void Parser::parse(const std::string& json) { +StyleParseResult Parser::parse(const std::string& json) { rapidjson::GenericDocument, rapidjson::CrtAllocator> document; document.Parse<0>(json.c_str()); if (document.HasParseError()) { - Log::Error(Event::ParseStyle, "Error parsing style JSON at %i: %s", document.GetErrorOffset(), rapidjson::GetParseError_En(document.GetParseError())); - return; + std::stringstream message; + message << document.GetErrorOffset() << " - " + << rapidjson::GetParseError_En(document.GetParseError()); + + return std::make_exception_ptr(std::runtime_error(message.str())); } if (!document.IsObject()) { - Log::Error(Event::ParseStyle, "Style JSON must be an object"); - return; + return std::make_exception_ptr(std::runtime_error("style must be an object")); } if (document.HasMember("version")) { @@ -100,6 +103,8 @@ void Parser::parse(const std::string& json) { glyphURL = { glyphs.GetString(), glyphs.GetStringLength() }; } } + + return nullptr; } void Parser::parseSources(const JSValue& value) { diff --git a/src/mbgl/style/parser.hpp b/src/mbgl/style/parser.hpp index a6a71ed81..a05a0b316 100644 --- a/src/mbgl/style/parser.hpp +++ b/src/mbgl/style/parser.hpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -16,11 +17,13 @@ namespace mbgl { namespace style { +using StyleParseResult = std::exception_ptr; + class Parser { public: ~Parser(); - void parse(const std::string&); + StyleParseResult parse(const std::string&); std::string spriteURL; std::string glyphURL; diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index 1cf64002f..e04384a96 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -90,7 +90,15 @@ void Style::setJSON(const std::string& json) { classes.clear(); Parser parser; - parser.parse(json); + auto error = parser.parse(json); + + if (error) { + Log::Error(Event::ParseStyle, "Failed to parse style: %s", util::toString(error).c_str()); + observer->onStyleError(); + observer->onResourceError(error); + + return; + } for (auto& source : parser.sources) { addSource(std::move(source)); diff --git a/test/fixtures/style_parser/non-object.info.json b/test/fixtures/style_parser/non-object.info.json index d1b67e2ea..ac6ea00d9 100644 --- a/test/fixtures/style_parser/non-object.info.json +++ b/test/fixtures/style_parser/non-object.info.json @@ -1,7 +1,7 @@ { "default": { "log": [ - [1, "ERROR", "ParseStyle", "Style JSON must be an object"] + [1, "ERROR", "ParseStyle", "Failed to parse style: style must be an object"] ] } -} \ No newline at end of file +} diff --git a/test/map/map.cpp b/test/map/map.cpp index 2dc8aad92..7e0bbd023 100644 --- a/test/map/map.cpp +++ b/test/map/map.cpp @@ -68,7 +68,7 @@ TEST(Map, SetStyleInvalidJSON) { auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); EXPECT_EQ(1u, flo->count({ EventSeverity::Error, Event::ParseStyle, -1, - "Error parsing style JSON at 0: Invalid value." })); + "Failed to parse style: 0 - Invalid value." })); auto unchecked = flo->unchecked(); EXPECT_TRUE(unchecked.empty()) << unchecked; } diff --git a/test/style/style_parser.cpp b/test/style/style_parser.cpp index b7806c11c..3e1149c99 100644 --- a/test/style/style_parser.cpp +++ b/test/style/style_parser.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -32,7 +33,11 @@ TEST_P(StyleParserTest, ParseStyle) { Log::setObserver(std::unique_ptr(observer)); style::Parser parser; - parser.parse(util::read_file(base + ".style.json")); + auto error = parser.parse(util::read_file(base + ".style.json")); + + if (error) { + Log::Error(Event::ParseStyle, "Failed to parse style: %s", util::toString(error).c_str()); + } for (auto it = infoDoc.MemberBegin(), end = infoDoc.MemberEnd(); it != end; it++) { const std::string name { it->name.GetString(), it->name.GetStringLength() };