From 791ddf90ce99a497de7919c085dae2d978f8a322 Mon Sep 17 00:00:00 2001 From: Sergey Magidovich Date: Sun, 6 Dec 2015 10:25:17 +0300 Subject: [PATCH] Code review. --- editor/editor.pro | 2 +- editor/editor_tests/editor_tests.pro | 2 +- editor/editor_tests/xml_feature_test.cpp | 18 ++-- editor/xml_feature.cpp | 101 ++++++++++++----------- editor/xml_feature.hpp | 21 +++-- omim.pro | 2 +- 6 files changed, 81 insertions(+), 65 deletions(-) diff --git a/editor/editor.pro b/editor/editor.pro index a7fde752db..f597714be0 100644 --- a/editor/editor.pro +++ b/editor/editor.pro @@ -10,7 +10,7 @@ include($$ROOT_DIR/common.pri) SOURCES += \ opening_hours_ui.cpp \ - ui2oh.cpp + ui2oh.cpp \ xml_feature.cpp \ HEADERS += \ diff --git a/editor/editor_tests/editor_tests.pro b/editor/editor_tests/editor_tests.pro index 29690dbf45..96da40e927 100644 --- a/editor/editor_tests/editor_tests.pro +++ b/editor/editor_tests/editor_tests.pro @@ -4,7 +4,7 @@ CONFIG -= app_bundle TEMPLATE = app ROOT_DIR = ../.. -DEPENDENCIES += editor base opening_hours pugixml +DEPENDENCIES += editor base coding geometry opening_hours pugixml include($$ROOT_DIR/common.pri) diff --git a/editor/editor_tests/xml_feature_test.cpp b/editor/editor_tests/xml_feature_test.cpp index 6f7903f462..592de5b514 100644 --- a/editor/editor_tests/xml_feature_test.cpp +++ b/editor/editor_tests/xml_feature_test.cpp @@ -2,13 +2,15 @@ #include "editor/xml_feature.hpp" +#include "geometry/mercator.hpp" + #include "base/timer.hpp" #include "std/sstream.hpp" #include "3party/pugixml/src/pugixml.hpp" -using namespace indexer; +using namespace editor; UNIT_TEST(XMLFeature_RawGetSet) { @@ -47,7 +49,7 @@ UNIT_TEST(XMLFeature_Setters) { XMLFeature feature; - feature.SetCenter(m2::PointD(64.2342340, 53.3124200)); + feature.SetCenter(MercatorBounds::FromLatLon(55.7978998, 37.4745280)); feature.SetModificationTime(my::StringToTimestamp("2015-11-27T21:13:32Z")); feature.SetName("Gorki Park"); @@ -62,7 +64,8 @@ UNIT_TEST(XMLFeature_Setters) auto const expectedString = R"( > x; - sstr.get(); - sstr.get(); - sstr >> y; - p = {x, y}; - return true; -} +auto constexpr kLatLonTolerance = 7; pugi::xml_node FindTag(pugi::xml_document const & document, string const & key) { return document.select_node(("//tag[@k='" + key + "']").data()).node(); } + +m2::PointD PointFromLatLon(pugi::xml_node const node) +{ + double lat, lon; + if (!strings::to_double(node.attribute("lat").value(), lat)) + MYTHROW(editor::XMLFeatureNoLatLonError, + ("Can't parse lat attribute: " + string(node.attribute("lat").value()))); + if (!strings::to_double(node.attribute("lon").value(), lon)) + MYTHROW(editor::XMLFeatureNoLatLonError, + ("Can't parse lon attribute: " + string(node.attribute("lon").value()))); + return MercatorBounds::FromLatLon(lat, lon); +} + +void ValidateNode(pugi::xml_node const & node) +{ + if (!node) + MYTHROW(editor::XMLFeatureNoNodeError, ("Document has no node")); + + try + { + PointFromLatLon(node); + } + catch(editor::XMLFeatureError) + { + throw; + } + + if (!node.attribute("timestamp")) + MYTHROW(editor::XMLFeatureNoTimestampError, ("Node has no timestamp attribute")); +} } // namespace - -namespace indexer +namespace editor { XMLFeature::XMLFeature() { @@ -48,26 +59,20 @@ XMLFeature::XMLFeature() XMLFeature::XMLFeature(string const & xml) { m_document.load(xml.data()); - - auto const node = GetRootNode(); - if (!node) - MYTHROW(XMLFeatureError, ("Document has no node")); - - auto attr = node.attribute("center"); - if (!attr) - MYTHROW(XMLFeatureError, ("Node has no center attribute")); - - m2::PointD center; - if (!FromString(attr.value(), center)) - MYTHROW(XMLFeatureError, ("Can't parse center attribute: " + string(attr.value()))); - - if (!(attr = node.attribute("timestamp"))) - MYTHROW(XMLFeatureError, ("Node has no timestamp attribute")); + ValidateNode(GetRootNode()); } XMLFeature::XMLFeature(pugi::xml_document const & xml) { m_document.reset(xml); + ValidateNode(GetRootNode()); +} + +XMLFeature::XMLFeature(pugi::xml_node const & xml) +{ + m_document.reset(); + m_document.append_copy(xml); + ValidateNode(GetRootNode()); } void XMLFeature::Save(ostream & ost) const @@ -77,24 +82,24 @@ void XMLFeature::Save(ostream & ost) const m2::PointD XMLFeature::GetCenter() const { - auto const node = m_document.child("node"); - m2::PointD center; - FromString(node.attribute("center").value(), center); - return center; + return PointFromLatLon(GetRootNode()); } -void XMLFeature::SetCenter(m2::PointD const & center) +void XMLFeature::SetCenter(m2::PointD const & mercatorCenter) { - SetAttribute("center", ToString(center)); + SetAttribute("lat", strings::to_string_dac(MercatorBounds::YToLat(mercatorCenter.y), + kLatLonTolerance)); + SetAttribute("lon", strings::to_string_dac(MercatorBounds::XToLon(mercatorCenter.x), + kLatLonTolerance)); } -string const XMLFeature::GetName(string const & lang) const +string XMLFeature::GetName(string const & lang) const { auto const suffix = lang == "default" || lang.empty() ? "" : ":" + lang; return GetTagValue("name" + suffix); } -string const XMLFeature::GetName(uint8_t const langCode) const +string XMLFeature::GetName(uint8_t const langCode) const { return GetName(StringUtf8Multilang::GetLangByCode(langCode)); } @@ -115,7 +120,7 @@ void XMLFeature::SetName(uint8_t const langCode, string const & name) SetName(StringUtf8Multilang::GetLangByCode(langCode), name); } -string const XMLFeature::GetHouse() const +string XMLFeature::GetHouse() const { return GetTagValue("addr:housenumber"); } @@ -190,4 +195,4 @@ pugi::xml_node XMLFeature::GetRootNode() const { return m_document.child("node"); } -} // namespace indexer +} // namespace editor diff --git a/editor/xml_feature.hpp b/editor/xml_feature.hpp index 067a61273c..ab13b96801 100644 --- a/editor/xml_feature.hpp +++ b/editor/xml_feature.hpp @@ -7,13 +7,18 @@ #include "std/map.hpp" #include "std/unique_ptr.hpp" +#include "coding/multilang_utf8_string.hpp" + #include "3party/pugixml/src/pugixml.hpp" -namespace indexer +namespace editor { DECLARE_EXCEPTION(XMLFeatureError, RootException); +DECLARE_EXCEPTION(XMLFeatureNoNodeError, XMLFeatureError); +DECLARE_EXCEPTION(XMLFeatureNoLatLonError, XMLFeatureError); +DECLARE_EXCEPTION(XMLFeatureNoTimestampError, XMLFeatureError); class XMLFeature { @@ -21,27 +26,28 @@ public: XMLFeature(); XMLFeature(string const & xml); XMLFeature(pugi::xml_document const & xml); + XMLFeature(pugi::xml_node const & xml); void Save(ostream & ost) const; m2::PointD GetCenter() const; - void SetCenter(m2::PointD const & center); + void SetCenter(m2::PointD const & mercatorCenter); string GetType() const; void SetType(string const & type); - string const GetName(string const & lang) const; - string const GetName(uint8_t const langCode = StringUtf8Multilang::DEFAULT_CODE) const; + string GetName(string const & lang) const; + string GetName(uint8_t const langCode = StringUtf8Multilang::DEFAULT_CODE) const; void SetName(string const & name); void SetName(string const & lang, string const & name); void SetName(uint8_t const langCode, string const & name); - string const GetHouse() const; + string GetHouse() const; void SetHouse(string const & house); time_t GetModificationTime() const; - void SetModificationTime(time_t const time); + void SetModificationTime(time_t const time = ::time(nullptr)); bool HasTag(string const & key) const; bool HasAttribute(string const & key) const; @@ -55,6 +61,7 @@ public: private: pugi::xml_node GetRootNode() const; + pugi::xml_document m_document; }; -} // namespace indexer +} // namespace editor diff --git a/omim.pro b/omim.pro index 5f4c9343de..aef71f2cc3 100644 --- a/omim.pro +++ b/omim.pro @@ -160,7 +160,7 @@ SUBDIRS = 3party base coding geometry editor indexer routing search SUBDIRS *= generator_tests editor_tests.subdir = editor/editor_tests - editor_tests.depends = 3party base editor geometry + editor_tests.depends = 3party base coding editor geometry SUBDIRS *= editor_tests SUBDIRS *= qt_tstfrm