diff options
author | Alex Zolotarev <alex@maps.me> | 2015-11-06 10:21:41 +0300 |
---|---|---|
committer | Alex Zolotarev <alex@maps.me> | 2015-11-10 19:16:05 +0300 |
commit | 30d1880a6fe5f15add03a59cfa04411bdfda1d36 (patch) | |
tree | a1fd0c0867f7ecdade742fb5599187fb8acd0781 | |
parent | 7685f5327b54902e09d9caf12ad288439510af45 (diff) |
Fixed wikipedia links mwm storage and displaying in Android.
Previous solution was a really bad engineering decision (more details in https://github.com/mapsme/omim/pull/421 )
-rw-r--r-- | android/jni/com/mapswithme/maps/Framework.cpp | 24 | ||||
-rw-r--r-- | android/res/layout/place_page_wiki.xml | 2 | ||||
-rw-r--r-- | android/src/com/mapswithme/maps/bookmarks/data/Metadata.java | 1 | ||||
-rw-r--r-- | android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java | 15 | ||||
-rw-r--r-- | generator/generator.pro | 2 | ||||
-rw-r--r-- | generator/generator_tests/metadata_test.cpp | 87 | ||||
-rw-r--r-- | generator/osm2meta.cpp | 50 | ||||
-rw-r--r-- | generator/osm2meta.hpp | 48 | ||||
-rw-r--r-- | indexer/feature_meta.cpp | 85 | ||||
-rw-r--r-- | indexer/feature_meta.hpp | 3 |
10 files changed, 150 insertions, 167 deletions
diff --git a/android/jni/com/mapswithme/maps/Framework.cpp b/android/jni/com/mapswithme/maps/Framework.cpp index f0213d5f79..58e9dbff60 100644 --- a/android/jni/com/mapswithme/maps/Framework.cpp +++ b/android/jni/com/mapswithme/maps/Framework.cpp @@ -739,15 +739,21 @@ namespace android // Fills mapobject's metadata from UserMark void Framework::InjectMetadata(JNIEnv * env, jclass const clazz, jobject const mapObject, UserMark const * userMark) { - feature::Metadata metadata; + using feature::Metadata; + + Metadata metadata; frm()->FindClosestPOIMetadata(userMark->GetOrg(), metadata); static jmethodID const addId = env->GetMethodID(clazz, "addMetadata", "(ILjava/lang/String;)V"); ASSERT ( addId, () ); - for (feature::Metadata::EType t : metadata.GetPresentTypes()) + for (auto const t : metadata.GetPresentTypes()) { - jstring metaString = jni::ToJavaString(env, metadata.Get(t)); + // TODO: It is not a good idea to pass raw strings to UI. Calling separate getters should be a better way. + // Upcoming change: how to pass opening hours (parsed) into Editor's UI? How to get edited changes back? + jstring metaString = t == Metadata::FMD_WIKIPEDIA ? + jni::ToJavaString(env, metadata.GetWikiURL()) : + jni::ToJavaString(env, metadata.Get(t)); env->CallVoidMethod(mapObject, addId, t, metaString); // TODO use unique_ptrs for autoallocation of local refs env->DeleteLocalRef(metaString); @@ -775,7 +781,9 @@ namespace { pair<jintArray, jobjectArray> NativeMetadataToJavaMetadata(JNIEnv * env, feature::Metadata const & metadata) { - vector<feature::Metadata::EType> const metaTypes = metadata.GetPresentTypes(); + using feature::Metadata; + + vector<Metadata::EType> const metaTypes = metadata.GetPresentTypes(); // FIXME arrays, allocated through New<Type>Array should be deleted manually in the method. // refactor that to delete refs locally or pass arrays from outside context const jintArray j_metaTypes = env->NewIntArray(metadata.Size()); @@ -784,8 +792,12 @@ pair<jintArray, jobjectArray> NativeMetadataToJavaMetadata(JNIEnv * env, feature for (size_t i = 0; i < metaTypes.size(); i++) { - arr[i] = metaTypes[i]; - jstring metaString = jni::ToJavaString(env, metadata.Get(metaTypes[i])); + auto const type = metaTypes[i]; + arr[i] = type; + // TODO: Refactor code to use separate getters for each metadata. + jstring metaString = type == Metadata::FMD_WIKIPEDIA ? + jni::ToJavaString(env, metadata.GetWikiURL()) : + jni::ToJavaString(env, metadata.Get(type)); env->SetObjectArrayElement(j_metaValues, i, metaString); env->DeleteLocalRef(metaString); } diff --git a/android/res/layout/place_page_wiki.xml b/android/res/layout/place_page_wiki.xml index 1e355175e4..22cbba06d2 100644 --- a/android/res/layout/place_page_wiki.xml +++ b/android/res/layout/place_page_wiki.xml @@ -28,5 +28,5 @@ android:layout_height="wrap_content" android:textColor="@color/text_place_page_blue" android:textSize="@dimen/text_size_body_1" - tools:text="https://en.wikipedia.org/"/> + android:text="@string/read_in_wikipedia"/> </LinearLayout> diff --git a/android/src/com/mapswithme/maps/bookmarks/data/Metadata.java b/android/src/com/mapswithme/maps/bookmarks/data/Metadata.java index 6bbbf35b5d..99892617ae 100644 --- a/android/src/com/mapswithme/maps/bookmarks/data/Metadata.java +++ b/android/src/com/mapswithme/maps/bookmarks/data/Metadata.java @@ -26,6 +26,7 @@ public class Metadata implements Parcelable FMD_TURN_LANES_BACKWARD(13), FMD_EMAIL(14), FMD_POSTCODE(15), + // TODO: It is hacked in jni and returns full Wikipedia url. Should use separate getter instead. FMD_WIKIPEDIA(16), FMD_MAXSPEED(17), FMD_FLATS(18); diff --git a/android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java b/android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java index ae8a3c1470..bb3411f3c8 100644 --- a/android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java +++ b/android/src/com/mapswithme/maps/widget/placepage/PlacePageView.java @@ -63,8 +63,6 @@ import com.mapswithme.util.sharing.ShareOption; import com.mapswithme.util.statistics.AlohaHelper; import com.mapswithme.util.statistics.Statistics; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; import java.util.ArrayList; import java.util.List; @@ -427,13 +425,7 @@ public class PlacePageView extends RelativeLayout implements View.OnClickListene refreshMetadataOrHide(mMapObject.getMetadata(Metadata.MetadataType.FMD_EMAIL), mEmail, mTvEmail); refreshMetadataOrHide(mMapObject.getMetadata(Metadata.MetadataType.FMD_OPERATOR), mOperator, mTvOperator); refreshMetadataOrHide(translateCuisine(mMapObject.getMetadata(Metadata.MetadataType.FMD_CUISINE)), mCuisine, mTvCuisine); - try - { - final String wikipedia = mMapObject.getMetadata(Metadata.MetadataType.FMD_WIKIPEDIA); - refreshMetadataOrHide(TextUtils.isEmpty(wikipedia) ? null : URLDecoder.decode(wikipedia, "UTF-8"), mWiki, mTvWiki); - } catch (UnsupportedEncodingException e) - { - } + refreshMetadataOrHide(mMapObject.getMetadata(Metadata.MetadataType.FMD_WIKIPEDIA), mWiki, mTvWiki); refreshMetadataOrHide(mMapObject.getMetadata(Metadata.MetadataType.FMD_INTERNET), mWifi, null); refreshMetadataOrHide(mMapObject.getMetadata(Metadata.MetadataType.FMD_FLATS), mEntrance, mTvEntrance); // TODO throw away parsing hack when data will be parsed correctly in core @@ -698,9 +690,8 @@ public class PlacePageView extends RelativeLayout implements View.OnClickListene followUrl(mTvWebsite.getText().toString()); break; case R.id.ll__place_wiki: - final String[] wikiParts = mTvWiki.getText().toString().split(":"); - if (wikiParts.length == 2) - followUrl("https://" + wikiParts[0] + ".wikipedia.org/wiki/" + wikiParts[1]); + // TODO: Refactor and use separate getters for Wiki and all other PP meta info too. + followUrl(mMapObject.getMetadata(Metadata.MetadataType.FMD_WIKIPEDIA)); break; case R.id.tv__bookmark_group: saveBookmarkNameIfUpdated(null); diff --git a/generator/generator.pro b/generator/generator.pro index 254d7df794..a5f12ece16 100644 --- a/generator/generator.pro +++ b/generator/generator.pro @@ -23,6 +23,7 @@ SOURCES += \ feature_generator.cpp \ feature_merger.cpp \ feature_sorter.cpp \ + osm2meta.cpp \ osm2type.cpp \ osm_element.cpp \ osm_id.cpp \ @@ -50,7 +51,6 @@ HEADERS += \ generate_info.hpp \ osm2meta.hpp \ osm2type.hpp \ - osm2meta.hpp \ osm_element.hpp \ osm_id.hpp \ osm_o5m_source.hpp \ diff --git a/generator/generator_tests/metadata_test.cpp b/generator/generator_tests/metadata_test.cpp index 364505b6c7..1666752024 100644 --- a/generator/generator_tests/metadata_test.cpp +++ b/generator/generator_tests/metadata_test.cpp @@ -131,44 +131,59 @@ UNIT_TEST(Metadata_ValidateAndFormat_ele) UNIT_TEST(Metadata_ValidateAndFormat_wikipedia) { + using feature::Metadata; + + char const * kWikiKey = "wikipedia"; + FeatureParams params; MetadataTagProcessor p(params); - string const lanaWoodUrlEncoded = "%D0%9B%D0%B0%D0%BD%D0%B0_%D0%92%D1%83%D0%B4"; - - p("wikipedia", "ru:Лана Вуд"); - TEST_EQUAL(params.GetMetadata().Get(feature::Metadata::FMD_WIKIPEDIA), "ru:" + lanaWoodUrlEncoded, ("ru:")); - TEST_EQUAL(params.GetMetadata().GetWikiTitle(), "ru:Лана Вуд", ("ru:")); - TEST_EQUAL(params.GetMetadata().GetWikiURL(), "https://ru.wikipedia.org/wiki/" + lanaWoodUrlEncoded, ("ru:")); - params.GetMetadata().Drop(feature::Metadata::FMD_WIKIPEDIA); - - p("wikipedia", "https://ru.wikipedia.org/wiki/" + lanaWoodUrlEncoded); - TEST_EQUAL(params.GetMetadata().Get(feature::Metadata::FMD_WIKIPEDIA), "ru:" + lanaWoodUrlEncoded, ("https:")); - TEST_EQUAL(params.GetMetadata().GetWikiTitle(), "ru:Лана Вуд", ("https:")); - TEST_EQUAL(params.GetMetadata().GetWikiURL(), "https://ru.wikipedia.org/wiki/" + lanaWoodUrlEncoded, ("https:")); - params.GetMetadata().Drop(feature::Metadata::FMD_WIKIPEDIA); - - p("wikipedia", "Test"); - TEST(params.GetMetadata().Empty(), ("Test")); - - p("wikipedia", "https://en.wikipedia.org/wiki/"); - TEST(params.GetMetadata().Empty(), ("Null wiki")); - - p("wikipedia", "http://.wikipedia.org/wiki/Whatever"); - TEST(params.GetMetadata().Empty(), ("Null lang", params.GetMetadata().Get(feature::Metadata::FMD_WIKIPEDIA))); - - // We ignore incorrect prefixes - p("wikipedia", "ht.tps://en.wikipedia.org/wiki/Whuh"); - TEST_EQUAL(params.GetMetadata().Get(feature::Metadata::FMD_WIKIPEDIA), "en:Whuh", ("ht.tp:")); - params.GetMetadata().Drop(feature::Metadata::FMD_WIKIPEDIA); - - p("wikipedia", "http://ru.google.com/wiki/wutlol"); - TEST(params.GetMetadata().Empty(), ("Google")); - - // URL Decoding Test - string const badWikiTitle = "%%A"; - p("wikipedia", "https://bad.wikipedia.org/wiki/" + badWikiTitle); - TEST_EQUAL(params.GetMetadata().GetWikiTitle(), "bad:" + badWikiTitle, ("bad title")); - params.GetMetadata().Drop(feature::Metadata::FMD_WIKIPEDIA); + + p(kWikiKey, "en:Bad %20Data"); + TEST_EQUAL(params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA), "en:Bad %20Data", ()); + TEST_EQUAL(params.GetMetadata().GetWikiURL(), "https://en.m.wikipedia.org/wiki/Bad_%2520Data", ()); + params.GetMetadata().Drop(Metadata::FMD_WIKIPEDIA); + + p(kWikiKey, "ru:Тест_with % sign"); + TEST_EQUAL(params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA), "ru:Тест with % sign", ()); + TEST_EQUAL(params.GetMetadata().GetWikiURL(), "https://ru.m.wikipedia.org/wiki/Тест_with_%25_sign", ()); + params.GetMetadata().Drop(Metadata::FMD_WIKIPEDIA); + + p(kWikiKey, "https://be-tarask.wikipedia.org/wiki/Вялікае_Княства_Літоўскае"); + TEST_EQUAL(params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA), "be-tarask:Вялікае Княства Літоўскае", ()); + TEST_EQUAL(params.GetMetadata().GetWikiURL(), "https://be-tarask.m.wikipedia.org/wiki/Вялікае_Княства_Літоўскае", ()); + params.GetMetadata().Drop(Metadata::FMD_WIKIPEDIA); + + // Final link points to https and mobile version. + p(kWikiKey, "http://en.wikipedia.org/wiki/A"); + TEST_EQUAL(params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA), "en:A", ()); + TEST_EQUAL(params.GetMetadata().GetWikiURL(), "https://en.m.wikipedia.org/wiki/A", ()); + params.GetMetadata().Drop(Metadata::FMD_WIKIPEDIA); + + p(kWikiKey, "invalid_input_without_language_and_colon"); + TEST(params.GetMetadata().Empty(), (params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA))); + + p(kWikiKey, "https://en.wikipedia.org/wiki/"); + TEST(params.GetMetadata().Empty(), (params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA))); + + p(kWikiKey, "http://wikipedia.org/wiki/Article"); + TEST(params.GetMetadata().Empty(), (params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA))); + + p(kWikiKey, "http://somesite.org"); + TEST(params.GetMetadata().Empty(), (params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA))); + + p(kWikiKey, "http://www.spamsitewithaslash.com/"); + TEST(params.GetMetadata().Empty(), (params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA))); + + p(kWikiKey, "http://.wikipedia.org/wiki/Article"); + TEST(params.GetMetadata().Empty(), (params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA))); + + // Ignore incorrect prefixes. + p(kWikiKey, "ht.tps://en.wikipedia.org/wiki/Whuh"); + TEST_EQUAL(params.GetMetadata().Get(Metadata::FMD_WIKIPEDIA), "en:Whuh", ()); + params.GetMetadata().Drop(Metadata::FMD_WIKIPEDIA); + + p(kWikiKey, "http://ru.google.com/wiki/wutlol"); + TEST(params.GetMetadata().Empty(), ("Not a wikipedia site.")); } UNIT_TEST(Metadata_ReadWrite_Intermediate) diff --git a/generator/osm2meta.cpp b/generator/osm2meta.cpp index c6afb392f2..985c20a5c3 100644 --- a/generator/osm2meta.cpp +++ b/generator/osm2meta.cpp @@ -1 +1,51 @@ #include "generator/osm2meta.hpp" + +#include "coding/url_encode.hpp" + +#include "base/logging.hpp" +#include "base/string_utils.hpp" + +string MetadataTagProcessor::ValidateAndFormat_wikipedia(string v) const +{ + strings::Trim(v); + // Normalize by converting full URL to "lang:title" if necessary + // (some OSMers do not follow standard for wikipedia tag). + static string const base = ".wikipedia.org/wiki/"; + auto const baseIndex = v.find(base); + if (baseIndex != string::npos) + { + auto const baseSize = base.size(); + // Do not allow urls without article name after /wiki/. + if (v.size() > baseIndex + baseSize) + { + auto const slashIndex = v.rfind('/', baseIndex); + if (slashIndex != string::npos && slashIndex + 1 != baseIndex) + { + // Normalize article title according to OSM standards. + string title = UrlDecode(v.substr(baseIndex + baseSize)); + replace(title.begin(), title.end(), '_', ' '); + return v.substr(slashIndex + 1, baseIndex - slashIndex - 1) + ":" + title; + } + } + LOG(LWARNING, ("Invalid Wikipedia tag value:", v)); + return string(); + } + // Standard case: "lang:Article Name With Spaces". + // Language and article are at least 2 chars each. + auto const colonIndex = v.find(':'); + if (colonIndex == string::npos || colonIndex < 2 || colonIndex + 2 > v.size()) + { + LOG(LWARNING, ("Invalid Wikipedia tag value:", v)); + return string(); + } + // Check if it's not a random/invalid link. + if (v.find('/') != string::npos) + { + LOG(LWARNING, ("Invalid Wikipedia tag value:", v)); + return string(); + } + // Normalize to OSM standards. + string normalized(v); + replace(normalized.begin() + colonIndex, normalized.end(), '_', ' '); + return normalized; +} diff --git a/generator/osm2meta.hpp b/generator/osm2meta.hpp index 4b30411912..79efd40fb9 100644 --- a/generator/osm2meta.hpp +++ b/generator/osm2meta.hpp @@ -213,51 +213,5 @@ protected: { return v; } - - // Special URL encoding for wikipedia: - // Replaces special characters with %HH codes - // And spaces with underscores. - string WikiUrlEncode(string const & value) const - { - ostringstream escaped; - escaped.fill('0'); - escaped << hex; - - for (auto const & c : value) - { - if (isalnum(c) || c == '-' || c == '_' || c == '.' || c == '~') - escaped << c; - else if (c == ' ') - escaped << '_'; - else - escaped << '%' << std::uppercase << setw(2) << static_cast<int>(static_cast<unsigned char>(c)); - } - - return escaped.str(); - } - - string ValidateAndFormat_wikipedia(string const & v) const - { - // Find prefix before ':', shortest case: "lg:aa". - string::size_type i = v.find(':'); - if (i == string::npos || i < 2 || i + 2 > v.length()) - return string(); - - // URL encode lang:title (lang is at most 3 chars), so URL can be reconstructed faster. - if (i <= 3 || v.substr(0, i) == "be-x-old") - return v.substr(0, i + 1) + WikiUrlEncode(v.substr(i + 1)); - - static string::size_type const minUrlPartLength = string("//be.wikipedia.org/wiki/AB").length(); - if (v[i+1] == '/' && i + minUrlPartLength < v.length()) - { - // Convert URL to "lang:title". - i += 3; // skip "://" - string::size_type const j = v.find('.', i + 1); - static string const wikiUrlPart = ".wikipedia.org/wiki/"; - if (j != string::npos && v.substr(j, wikiUrlPart.length()) == wikiUrlPart) - return v.substr(i, j - i) + ":" + v.substr(j + wikiUrlPart.length()); - } - return string(); - } - + string ValidateAndFormat_wikipedia(string v) const; }; diff --git a/indexer/feature_meta.cpp b/indexer/feature_meta.cpp index f709aa0391..cc41944241 100644 --- a/indexer/feature_meta.cpp +++ b/indexer/feature_meta.cpp @@ -1,72 +1,33 @@ #include "indexer/feature_meta.hpp" -namespace -{ -char HexToDec(char ch) -{ - if (ch >= '0' && ch <= '9') - return ch - '0'; - if (ch >= 'a') - ch -= 'a' - 'A'; - if (ch >= 'A' && ch <= 'F') - return ch - 'A' + 10; - return -1; -} - -string UriDecode(string const & sSrc) -{ - // This code is based on - // http://www.codeguru.com/cpp/cpp/string/conversions/article.php/c12759 - // - // Note from RFC1630: "Sequences which start with a percent - // sign but are not followed by two hexadecimal characters - // (0-9, A-F) are reserved for future extension" - - string result(sSrc.length(), 0); - auto itResult = result.begin(); - - for (auto it = sSrc.begin(); it != sSrc.end(); ++it) - { - if (*it == '%') - { - if (distance(it, sSrc.end()) > 2) - { - char dec1 = HexToDec(*(it + 1)); - char dec2 = HexToDec(*(it + 2)); - if (-1 != dec1 && -1 != dec2) - { - *itResult++ = (dec1 << 4) + dec2; - it += 2; - continue; - } - } - } - - if (*it == '_') - *itResult++ = ' '; - else - *itResult++ = *it; - } - - result.resize(distance(result.begin(), itResult)); - return result; -} -} // namespace +#include "std/algorithm.hpp" namespace feature { + string Metadata::GetWikiURL() const { - string value = this->Get(FMD_WIKIPEDIA); - string::size_type i = value.find(':'); - if (i == string::npos) - return string(); - return "https://" + value.substr(0, i) + ".wikipedia.org/wiki/" + value.substr(i + 1); + string v = this->Get(FMD_WIKIPEDIA); + if (v.empty()) + return v; + + auto const colon = v.find(':'); + if (colon == string::npos) + return v; + + // Spaces and % sign should be replaced in urls. + replace(v.begin() + colon, v.end(), ' ', '_'); + string::size_type percent, pos = colon; + string const escapedPercent("%25"); + while ((percent = v.find('%', pos)) != string::npos) + { + v.replace(percent, 1, escapedPercent); + pos = percent + escapedPercent.size(); + } + // Trying to avoid redirects by constructing the right link. + // TODO: Wikipedia article could be opened it a user's language, but need + // generator level support to check for available article languages first. + return "https://" + v.substr(0, colon) + ".m.wikipedia.org/wiki/" + v.substr(colon + 1); } -string Metadata::GetWikiTitle() const -{ - string value = this->Get(FMD_WIKIPEDIA); - return UriDecode(value); -} } // namespace feature diff --git a/indexer/feature_meta.hpp b/indexer/feature_meta.hpp index cc588048d7..363f899ded 100644 --- a/indexer/feature_meta.hpp +++ b/indexer/feature_meta.hpp @@ -54,7 +54,7 @@ namespace feature string Get(EType type) const { - auto it = m_metadata.find(type); + auto const it = m_metadata.find(type); return (it == m_metadata.end()) ? string() : it->second; } @@ -78,7 +78,6 @@ namespace feature inline size_t Size() const { return m_metadata.size(); } string GetWikiURL() const; - string GetWikiTitle() const; template <class ArchiveT> void SerializeToMWM(ArchiveT & ar) const { |