organicmaps/organicmaps-tmp
Archived
13
Fork 1
Code Issues 2.7k Pull requests 182 Projects Releases 65 Wiki Activity Actions

[Android] Parse social pages links in POI editor #1545

Merged
root merged 6 commits from issue-1397-android-editor-process-social-link into master 2021-11-16 22:05:44 +00:00
Conversation 8 Commits 6 Files changed 12 +528 -524
strump commented 2021-11-12 16:12:06 +00:00
Member
Copy link

Moved Facebook, Instagram, Twitter, VK cleanup functions from "generator" to "indexer".
Added call to ValidateAndFormat_* functions on social links edit.
Fixed contact:line validation. Empty string is now correct value.

Closes #1397
Closes #1546

Updated 15.11.2021

Renamed contacts_processor to validate_and_format_contacts.

Moved functions ValidateFacebookPage, ValidateInstagramPage, ValidateTwitterPage, ValidateVkPage, ValidateLinePage to validate_and_format_contacts.hpp/.cpp

Moved social network tests to separate file validate_and_format_contacts_test.cpp. Also added contact:line tests there (no more issue #1546 😁 )

Updated XCode project to fix iOS build.

Moved Facebook, Instagram, Twitter, VK cleanup functions from "generator" to "indexer". Added call to `ValidateAndFormat_*` functions on social links edit. Fixed `contact:line` validation. Empty string is now correct value. Closes #1397 Closes #1546 ## Updated 15.11.2021 Renamed `contacts_processor` to `validate_and_format_contacts`. Moved functions `ValidateFacebookPage`, `ValidateInstagramPage`, `ValidateTwitterPage`, `ValidateVkPage`, `ValidateLinePage` to `validate_and_format_contacts.hpp/.cpp` Moved social network tests to separate file `validate_and_format_contacts_test.cpp`. Also added `contact:line` tests there (no more issue #1546 😁 ) Updated XCode project to fix iOS build.
strump commented 2021-11-12 16:23:03 +00:00
Author
Member
Copy link

After I moved some functions from "generator" to "indexer" there could be uncertainty in osm2meta.hpp:

case Metadata::FMD_WEBSITE: valid = ValidateAndFormat_url(v); break;  // <<< here we call method MetadataTagProcessorImpl::ValidateAndFormat_url
case Metadata::FMD_CONTACT_FACEBOOK: valid = osm::ValidateAndFormat_facebook(v); break; // <<< here we call static function
case Metadata::FMD_CONTACT_INSTAGRAM: valid = osm::ValidateAndFormat_instagram(v); break; // <<< here we call static function
case Metadata::FMD_CONTACT_TWITTER: valid = osm::ValidateAndFormat_twitter(v); break; // <<< here we call static function

Maybe we should rename ValidateAndFormat_* static functions for clarity?

After I moved some functions from "generator" to "indexer" there could be uncertainty in `osm2meta.hpp`: ```cpp case Metadata::FMD_WEBSITE: valid = ValidateAndFormat_url(v); break; // <<< here we call method MetadataTagProcessorImpl::ValidateAndFormat_url case Metadata::FMD_CONTACT_FACEBOOK: valid = osm::ValidateAndFormat_facebook(v); break; // <<< here we call static function case Metadata::FMD_CONTACT_INSTAGRAM: valid = osm::ValidateAndFormat_instagram(v); break; // <<< here we call static function case Metadata::FMD_CONTACT_TWITTER: valid = osm::ValidateAndFormat_twitter(v); break; // <<< here we call static function ``` Maybe we should rename `ValidateAndFormat_*` static functions for clarity?
biodranik (Migrated from github.com) approved these changes 2021-11-12 23:26:54 +00:00
generator/osm2meta.hpp
biodranik (Migrated from github.com) commented 2021-11-12 23:12:40 +00:00

We sort includes by alphabet inside one include block.

We sort includes by alphabet inside one include block.
indexer/CMakeLists.txt
biodranik (Migrated from github.com) commented 2021-11-12 23:14:55 +00:00

validate_and_format_contacts would be a better name.

validate_and_format_contacts would be a better name.
indexer/contacts_processor.cpp
biodranik (Migrated from github.com) commented 2021-11-12 23:16:04 +00:00

This else is redundant.

This else is redundant.
biodranik (Migrated from github.com) commented 2021-11-12 23:20:01 +00:00
  1. If a function returns an object, there is no benefit of using const & reference to it (compiler creates and keeps a full string anyway). Just use string const.
  2. nit: use right-to-left types everywhere, according to our style guide: "a reference to a constant string" is string const &
1. If a function returns an object, there is no benefit of using const & reference to it (compiler creates and keeps a full string anyway). Just use `string const`. 2. nit: use right-to-left types everywhere, according to our style guide: "a reference to a constant string" is `string const &`
biodranik (Migrated from github.com) commented 2021-11-12 23:20:33 +00:00

Redundant.

Redundant.
biodranik (Migrated from github.com) commented 2021-11-12 23:20:40 +00:00

Redundant.

Redundant.
biodranik (Migrated from github.com) commented 2021-11-12 23:20:49 +00:00

Redundant.

Redundant.
biodranik (Migrated from github.com) commented 2021-11-12 23:21:04 +00:00

Redundant.

Redundant.
biodranik (Migrated from github.com) commented 2021-11-12 23:22:10 +00:00

nit: easier to debug if the "if's" block (return here) is on the next line. And it is also in our style guide.

nit: easier to debug if the "if's" block (return here) is on the next line. And it is also in our style guide.
biodranik (Migrated from github.com) commented 2021-11-12 23:22:31 +00:00

Redundant.

Redundant.
indexer/editable_map_object.cpp
@ -509,3 +510,3 @@
m_metadata.Set(feature::Metadata::FMD_CONTACT_TWITTER, twitterPage);
m_metadata.Set(feature::Metadata::FMD_CONTACT_TWITTER, ValidateAndFormat_twitter(twitterPage));
}
biodranik (Migrated from github.com) commented 2021-11-12 23:26:23 +00:00

All these methods take string const &. Then you don't need to create a copy of the string in the function argument by using string facebookPage (yes, in C++ this declaration creates a copy of the passed parameter). Use string const & facebookPage here and in other functions.

All these methods take `string const &`. Then you don't need to create a copy of the string in the function argument by using `string facebookPage` (yes, in C++ this declaration creates a copy of the passed parameter). Use `string const & facebookPage` here and in other functions.
biodranik commented 2021-11-12 23:40:12 +00:00 (Migrated from github.com)
Copy link
  1. Curious (without looking at the implementation), why web URL is not validated by the same code used in the generator?
  2. There is a build error for iOS, assumingly because of newly added files. Here is the patch to apply (just rename file names if necessary):
diff --git a/xcode/indexer/indexer.xcodeproj/project.pbxproj b/xcode/indexer/indexer.xcodeproj/project.pbxproj
index 1c6cdfabc1..0399b64158 100644
--- a/xcode/indexer/indexer.xcodeproj/project.pbxproj
+++ b/xcode/indexer/indexer.xcodeproj/project.pbxproj
@@ -184,6 +184,8 @@
                FA67C84626BB356800B33DCA /* categories_cuisines.txt in Resources */ = {isa = PBXBuildFile; fileRef = 4052928E21496D2B00D821F1 /* categories_cuisines.txt */; };
                FA67C84926BB35A400B33DCA /* bounds.hpp in Headers */ = {isa = PBXBuildFile; fileRef = 40C3C090205BF9F400CED188 /* bounds.hpp */; };
                FA67C84F26BB36D700B33DCA /* categories_brands.txt in Resources */ = {isa = PBXBuildFile; fileRef = FA67C84E26BB36D600B33DCA /* categories_brands.txt */; };
+               FA7F9B84273F32680093EA08 /* contacts_processor.hpp in Headers */ = {isa = PBXBuildFile; fileRef = FA7F9B82273F32680093EA08 /* contacts_processor.hpp */; };
+               FA7F9B85273F32680093EA08 /* contacts_processor.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FA7F9B83273F32680093EA08 /* contacts_processor.cpp */; };
                FACB7C0B26B9176500810C9C /* brands_tests.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 406970A121AEF2F10024DDB2 /* brands_tests.cpp */; };
                FACB7C0C26B9176A00810C9C /* centers_table_test.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 3D452AF91EE6D9F5009EAB9B /* centers_table_test.cpp */; };
                FACB7C0D26B9176F00810C9C /* classificator_tests.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 40DF582C2174979200E4E0FC /* classificator_tests.cpp */; };
@@ -416,6 +418,8 @@
                F6DF5F301CD0FD9A00A87154 /* categories_index.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = categories_index.hpp; sourceTree = "<group>"; };
                F6F1DABD1F13D8B4006A69B7 /* ftraits.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = ftraits.hpp; sourceTree = "<group>"; };
                FA67C84E26BB36D600B33DCA /* categories_brands.txt */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = categories_brands.txt; path = ../../data/categories_brands.txt; sourceTree = "<group>"; };
+               FA7F9B82273F32680093EA08 /* contacts_processor.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = contacts_processor.hpp; sourceTree = "<group>"; };
+               FA7F9B83273F32680093EA08 /* contacts_processor.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = contacts_processor.cpp; sourceTree = "<group>"; };
                FACB7C1A26B919AD00810C9C /* libbase.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; path = libbase.a; sourceTree = BUILT_PRODUCTS_DIR; };
                FACB7C1B26B919AD00810C9C /* libcoding.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; path = libcoding.a; sourceTree = BUILT_PRODUCTS_DIR; };
                FACB7C1C26B919AD00810C9C /* libgeometry.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; path = libgeometry.a; sourceTree = BUILT_PRODUCTS_DIR; };
@@ -622,6 +626,8 @@
                                675340AD1A3F540F00A0A8C3 /* classificator_loader.hpp */,
                                675340AE1A3F540F00A0A8C3 /* classificator.cpp */,
                                675340AF1A3F540F00A0A8C3 /* classificator.hpp */,
+                               FA7F9B83273F32680093EA08 /* contacts_processor.cpp */,
+                               FA7F9B82273F32680093EA08 /* contacts_processor.hpp */,
                                34583BC11C88552100F94664 /* cuisines.cpp */,
                                34583BC21C88552100F94664 /* cuisines.hpp */,
                                40D62CEE23F2E8BE009A20F5 /* dat_section_header.hpp */,
@@ -797,6 +803,7 @@
                                6758AED21BB4413000C26E27 /* drules_selector_parser.hpp in Headers */,
                                6753413F1A3F540F00A0A8C3 /* scale_index.hpp in Headers */,
                                6753410E1A3F540F00A0A8C3 /* drawing_rules.hpp in Headers */,
+                               FA7F9B84273F32680093EA08 /* contacts_processor.hpp in Headers */,
                                409EE3E3237E9AA700EA31A4 /* postcodes.hpp in Headers */,
                                670C615C1AB0691900C38A8C /* features_offsets_table.hpp in Headers */,
                                6758AED41BB4413000C26E27 /* drules_selector.hpp in Headers */,
@@ -998,6 +1005,7 @@
                                675341121A3F540F00A0A8C3 /* feature_algo.cpp in Sources */,
                                675341211A3F540F00A0A8C3 /* feature_utils.cpp in Sources */,
                                4099F6491FC7142A002A7B05 /* fake_feature_ids.cpp in Sources */,
+                               FA7F9B85273F32680093EA08 /* contacts_processor.cpp in Sources */,
                                675341231A3F540F00A0A8C3 /* feature_visibility.cpp in Sources */,
                                56C74C221C749E4700B71B9F /* search_delimiters.cpp in Sources */,
                                347F337B1C454242009758CC /* rank_table.cpp in Sources */,
1. Curious (without looking at the implementation), why web URL is not validated by the same code used in the generator? 2. There is a build error for iOS, assumingly because of newly added files. Here is the patch to apply (just rename file names if necessary): ```patch diff --git a/xcode/indexer/indexer.xcodeproj/project.pbxproj b/xcode/indexer/indexer.xcodeproj/project.pbxproj index 1c6cdfabc1..0399b64158 100644 --- a/xcode/indexer/indexer.xcodeproj/project.pbxproj +++ b/xcode/indexer/indexer.xcodeproj/project.pbxproj @@ -184,6 +184,8 @@ FA67C84626BB356800B33DCA /* categories_cuisines.txt in Resources */ = {isa = PBXBuildFile; fileRef = 4052928E21496D2B00D821F1 /* categories_cuisines.txt */; }; FA67C84926BB35A400B33DCA /* bounds.hpp in Headers */ = {isa = PBXBuildFile; fileRef = 40C3C090205BF9F400CED188 /* bounds.hpp */; }; FA67C84F26BB36D700B33DCA /* categories_brands.txt in Resources */ = {isa = PBXBuildFile; fileRef = FA67C84E26BB36D600B33DCA /* categories_brands.txt */; }; + FA7F9B84273F32680093EA08 /* contacts_processor.hpp in Headers */ = {isa = PBXBuildFile; fileRef = FA7F9B82273F32680093EA08 /* contacts_processor.hpp */; }; + FA7F9B85273F32680093EA08 /* contacts_processor.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FA7F9B83273F32680093EA08 /* contacts_processor.cpp */; }; FACB7C0B26B9176500810C9C /* brands_tests.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 406970A121AEF2F10024DDB2 /* brands_tests.cpp */; }; FACB7C0C26B9176A00810C9C /* centers_table_test.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 3D452AF91EE6D9F5009EAB9B /* centers_table_test.cpp */; }; FACB7C0D26B9176F00810C9C /* classificator_tests.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 40DF582C2174979200E4E0FC /* classificator_tests.cpp */; }; @@ -416,6 +418,8 @@ F6DF5F301CD0FD9A00A87154 /* categories_index.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = categories_index.hpp; sourceTree = "<group>"; }; F6F1DABD1F13D8B4006A69B7 /* ftraits.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = ftraits.hpp; sourceTree = "<group>"; }; FA67C84E26BB36D600B33DCA /* categories_brands.txt */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = categories_brands.txt; path = ../../data/categories_brands.txt; sourceTree = "<group>"; }; + FA7F9B82273F32680093EA08 /* contacts_processor.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = contacts_processor.hpp; sourceTree = "<group>"; }; + FA7F9B83273F32680093EA08 /* contacts_processor.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = contacts_processor.cpp; sourceTree = "<group>"; }; FACB7C1A26B919AD00810C9C /* libbase.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; path = libbase.a; sourceTree = BUILT_PRODUCTS_DIR; }; FACB7C1B26B919AD00810C9C /* libcoding.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; path = libcoding.a; sourceTree = BUILT_PRODUCTS_DIR; }; FACB7C1C26B919AD00810C9C /* libgeometry.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; path = libgeometry.a; sourceTree = BUILT_PRODUCTS_DIR; }; @@ -622,6 +626,8 @@ 675340AD1A3F540F00A0A8C3 /* classificator_loader.hpp */, 675340AE1A3F540F00A0A8C3 /* classificator.cpp */, 675340AF1A3F540F00A0A8C3 /* classificator.hpp */, + FA7F9B83273F32680093EA08 /* contacts_processor.cpp */, + FA7F9B82273F32680093EA08 /* contacts_processor.hpp */, 34583BC11C88552100F94664 /* cuisines.cpp */, 34583BC21C88552100F94664 /* cuisines.hpp */, 40D62CEE23F2E8BE009A20F5 /* dat_section_header.hpp */, @@ -797,6 +803,7 @@ 6758AED21BB4413000C26E27 /* drules_selector_parser.hpp in Headers */, 6753413F1A3F540F00A0A8C3 /* scale_index.hpp in Headers */, 6753410E1A3F540F00A0A8C3 /* drawing_rules.hpp in Headers */, + FA7F9B84273F32680093EA08 /* contacts_processor.hpp in Headers */, 409EE3E3237E9AA700EA31A4 /* postcodes.hpp in Headers */, 670C615C1AB0691900C38A8C /* features_offsets_table.hpp in Headers */, 6758AED41BB4413000C26E27 /* drules_selector.hpp in Headers */, @@ -998,6 +1005,7 @@ 675341121A3F540F00A0A8C3 /* feature_algo.cpp in Sources */, 675341211A3F540F00A0A8C3 /* feature_utils.cpp in Sources */, 4099F6491FC7142A002A7B05 /* fake_feature_ids.cpp in Sources */, + FA7F9B85273F32680093EA08 /* contacts_processor.cpp in Sources */, 675341231A3F540F00A0A8C3 /* feature_visibility.cpp in Sources */, 56C74C221C749E4700B71B9F /* search_delimiters.cpp in Sources */, 347F337B1C454242009758CC /* rank_table.cpp in Sources */, ```
strump reviewed 2021-11-15 09:43:40 +00:00
generator/osm2meta.hpp
strump commented 2021-11-15 09:43:40 +00:00
Author
Member

renamed to validate_and_format_contacts.hpp and sorted includes alphabetially.

renamed to `validate_and_format_contacts.hpp` and sorted includes alphabetially.
strump reviewed 2021-11-15 09:44:08 +00:00
indexer/CMakeLists.txt
strump commented 2021-11-15 09:44:08 +00:00
Author
Member

renamed to validate_and_format_contacts

renamed to `validate_and_format_contacts`
strump reviewed 2021-11-15 09:44:17 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:44:16 +00:00
Author
Member

Fixed

Fixed
strump reviewed 2021-11-15 09:44:32 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:44:32 +00:00
Author
Member

Fixed

Fixed
strump reviewed 2021-11-15 09:44:42 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:44:42 +00:00
Author
Member

Done!

Done!
strump reviewed 2021-11-15 09:44:50 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:44:50 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-15 09:44:56 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:44:56 +00:00
Author
Member

Fixed

Fixed
strump reviewed 2021-11-15 09:45:03 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:45:03 +00:00
Author
Member

Fixed

Fixed
strump reviewed 2021-11-15 09:45:15 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:45:14 +00:00
Author
Member

Moved return to next line

Moved return to next line
strump reviewed 2021-11-15 09:45:22 +00:00
indexer/contacts_processor.cpp
strump commented 2021-11-15 09:45:21 +00:00
Author
Member

Fixed

Fixed
strump reviewed 2021-11-15 09:45:32 +00:00
indexer/editable_map_object.cpp
@ -509,3 +510,3 @@
m_metadata.Set(feature::Metadata::FMD_CONTACT_TWITTER, twitterPage);
m_metadata.Set(feature::Metadata::FMD_CONTACT_TWITTER, ValidateAndFormat_twitter(twitterPage));
}
strump commented 2021-11-15 09:45:32 +00:00
Author
Member

Changed argument type

Changed argument type
vng (Migrated from github.com) approved these changes 2021-11-15 11:05:05 +00:00
strump commented 2021-11-15 13:19:10 +00:00
Author
Member
Copy link

Moved regex's to static variables to reuse same regular expressions in validation and formatting functions. Now all regexes could be found at first lines of validate_and_format_contacts.cpp.
Looks like I'm done with this PR 🕺🏻

Moved regex's to static variables to reuse same regular expressions in validation and formatting functions. Now all regexes could be found at first lines of `validate_and_format_contacts.cpp`. Looks like I'm done with this PR 🕺🏻
biodranik (Migrated from github.com) requested changes 2021-11-15 16:30:36 +00:00
biodranik (Migrated from github.com) left a comment

Thank you very much! With every iteration your understanding of C++ beauty grows further and further ;-) I hope you enjoy it and don't hate me for the feedback :)

Нет предела совершенству!

Thank you very much! With every iteration your understanding of C++ beauty grows further and further ;-) I hope you enjoy it and don't hate me for the feedback :) Нет предела совершенству!
indexer/validate_and_format_contacts.cpp
biodranik (Migrated from github.com) commented 2021-11-15 15:41:09 +00:00

nit: redundant else :)

nit: redundant else :)
biodranik (Migrated from github.com) commented 2021-11-15 15:42:16 +00:00

nit: To make the code easier to read, and shift code below to the left by two spaces, and remove bracers, you can negate this if and immediately return {};

nit: To make the code easier to read, and shift code below to the left by two spaces, and remove bracers, you can negate this if and immediately `return {};`
biodranik (Migrated from github.com) commented 2021-11-15 15:43:54 +00:00

nit: The same: Negating if removes the indentation below.

nit: The same: Negating if removes the indentation below.
biodranik (Migrated from github.com) commented 2021-11-15 15:45:03 +00:00

nit: & is redundant.

nit: & is redundant.
biodranik (Migrated from github.com) commented 2021-11-15 15:45:23 +00:00

nit: & is redundant.

nit: & is redundant.
biodranik (Migrated from github.com) commented 2021-11-15 15:45:40 +00:00

nit: & is redundant.

nit: & is redundant.
biodranik (Migrated from github.com) commented 2021-11-15 15:45:55 +00:00

nit: & is redundant.

nit: & is redundant.
biodranik (Migrated from github.com) commented 2021-11-15 15:47:27 +00:00

nit: } // namespace osm

nit: `} // namespace osm`
biodranik (Migrated from github.com) commented 2021-11-15 15:48:10 +00:00

nit^2: . at the ends of sentences ;-)

nit^2: . at the ends of sentences ;-)
biodranik (Migrated from github.com) commented 2021-11-15 15:49:07 +00:00

nit^3: const position ;-)

nit^3: const position ;-)
biodranik (Migrated from github.com) commented 2021-11-15 15:49:20 +00:00

nit: & is redundant.

nit: & is redundant.
biodranik (Migrated from github.com) commented 2021-11-15 16:00:26 +00:00

nit: This part can be simpler and a bit faster by either adding an optional @ at the beginning of the regex, or using this approach:

if (regex_match(page.begin() + (page.front() == '@' ? 1 : 0), s_lineRegex))
  return true;
nit: This part can be simpler and a bit faster by either adding an optional @ at the beginning of the regex, or using this approach: ```C++ if (regex_match(page.begin() + (page.front() == '@' ? 1 : 0), s_lineRegex)) return true; ```
biodranik (Migrated from github.com) commented 2021-11-15 16:03:02 +00:00

nit: As you don't really need/use url object, the code can be shorter:

string const domain = strings::MakeLowerCase(url::Url{linePageUrl}.GetWebDomain());
nit: As you don't really need/use url object, the code can be shorter: ```C++ string const domain = strings::MakeLowerCase(url::Url{linePageUrl}.GetWebDomain()); ```
biodranik (Migrated from github.com) commented 2021-11-15 16:09:42 +00:00

nit: Negate the if, return false, and make the code shorter without bracers.

nit: Negate the if, return false, and make the code shorter without bracers.
biodranik (Migrated from github.com) commented 2021-11-15 16:11:07 +00:00 
  if (!EditableMapObject::ValidateWebsite(page))
    return regex_match(page, s_twitterRegex);  // Rules from here: https://stackoverflow.com/q/11361044
  string const domain = strings::MakeLowerCase(url::Url::FromString(page).GetWebDomain());
  return domain == "twitter.com" || strings::EndsWith(domain, ".twitter.com");
```suggestion if (!EditableMapObject::ValidateWebsite(page)) return regex_match(page, s_twitterRegex); // Rules from here: https://stackoverflow.com/q/11361044 string const domain = strings::MakeLowerCase(url::Url::FromString(page).GetWebDomain()); return domain == "twitter.com" || strings::EndsWith(domain, ".twitter.com"); ```
biodranik (Migrated from github.com) commented 2021-11-15 16:13:24 +00:00 
  if (!EditableMapObject::ValidateWebsite(page))
    return false;

  string const domain = strings::MakeLowerCase(url::Url::FromString(page).GetWebDomain());
  return strings::StartsWith(domain, "facebook.") || strings::StartsWith(domain, "fb.")
      || domain.find(".facebook.") != string::npos || domain.find(".fb.") != string::npos;
```suggestion if (!EditableMapObject::ValidateWebsite(page)) return false; string const domain = strings::MakeLowerCase(url::Url::FromString(page).GetWebDomain()); return strings::StartsWith(domain, "facebook.") || strings::StartsWith(domain, "fb.") || domain.find(".facebook.") != string::npos || domain.find(".fb.") != string::npos; ```
biodranik (Migrated from github.com) commented 2021-11-15 16:14:35 +00:00

nit^2: const :)

nit^2: const :)
biodranik (Migrated from github.com) commented 2021-11-15 16:16:12 +00:00 
    if (vkPageClean.front() == '_' && vkPageClean.back() == '_' || regex_match(vkPageClean, s_badVkRegex))
      return {};
```suggestion if (vkPageClean.front() == '_' && vkPageClean.back() == '_' || regex_match(vkPageClean, s_badVkRegex)) return {}; ```
biodranik (Migrated from github.com) commented 2021-11-15 16:18:10 +00:00

If webPath is empty, the code will crash below.

If webPath is empty, the code will crash below.
biodranik (Migrated from github.com) commented 2021-11-15 16:19:35 +00:00

nit: Negate the if to simplify indentation and remove lines with bracers.

nit: Negate the if to simplify indentation and remove lines with bracers.
biodranik (Migrated from github.com) commented 2021-11-15 16:26:53 +00:00

You can use this approach:

webPath.erase(webPath.find_last_not_of('/') + 1);
webPath.erase(0, str.find_first_not_of('@'));
You can use this approach: ```C++ webPath.erase(webPath.find_last_not_of('/') + 1); webPath.erase(0, str.find_first_not_of('@')); ```
biodranik (Migrated from github.com) commented 2021-11-15 16:27:31 +00:00

See how to properly fix this crash below.

See how to properly fix this crash below.
biodranik (Migrated from github.com) commented 2021-11-15 16:28:05 +00:00

See how to properly fix this crash below.

See how to properly fix this crash below.
biodranik (Migrated from github.com) commented 2021-11-15 16:28:59 +00:00 
    if (vkLogin.front() == '_' && vkLogin.back() == '_' || regex_match(vkLogin, s_badVkRegex))
      return false;
```suggestion if (vkLogin.front() == '_' && vkLogin.back() == '_' || regex_match(vkLogin, s_badVkRegex)) return false; ```
vng (Migrated from github.com) reviewed 2021-11-15 21:16:01 +00:00
indexer/validate_and_format_contacts.cpp
vng (Migrated from github.com) commented 2021-11-15 21:16:01 +00:00

Э, как-то сложно. Судя по логике, pop_back и pop_front будет просто и достаточно, ну и конечно надо проверить на empty предварительно.

Э, как-то сложно. Судя по логике, pop_back и pop_front будет просто и достаточно, ну и конечно надо проверить на empty предварительно.
biodranik (Migrated from github.com) reviewed 2021-11-15 22:13:02 +00:00
indexer/validate_and_format_contacts.cpp
biodranik (Migrated from github.com) commented 2021-11-15 22:13:01 +00:00

Тогда на empty придётся проверять дважды. Это самый простой способ, он ещё кстати обработает @@@@

Тогда на empty придётся проверять дважды. Это самый простой способ, он ещё кстати обработает @@@@
strump reviewed 2021-11-16 08:17:02 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:17:01 +00:00
Author
Member

Simplified if() statement. But also added parentheses (vkLogin.front() == '_' && vkLogin.back() == '_')

Simplified if() statement. But also added parentheses `(vkLogin.front() == '_' && vkLogin.back() == '_')`
strump reviewed 2021-11-16 08:17:31 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:17:31 +00:00
Author
Member

Get rid of .back() method using your advice

Get rid of `.back()` method using your advice
strump reviewed 2021-11-16 08:17:46 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:17:46 +00:00
Author
Member

Get rid of .back() method using your advice

Get rid of `.back()` method using your advice
strump reviewed 2021-11-16 08:18:14 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:18:14 +00:00
Author
Member

Negated if() statement

Negated if() statement
strump reviewed 2021-11-16 08:18:31 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:18:31 +00:00
Author
Member

Get rid of .back() method using your advice

Get rid of `.back()` method using your advice
strump reviewed 2021-11-16 08:18:56 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:18:56 +00:00
Author
Member

Simplified if() statement. But also added parentheses (vkLogin.front() == '_' && vkLogin.back() == '_')

Simplified if() statement. But also added parentheses `(vkLogin.front() == '_' && vkLogin.back() == '_')`
strump reviewed 2021-11-16 08:19:43 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:19:43 +00:00
Author
Member

Simplified this block using negated if (!EditableMapObject::ValidateWebsite(page))

Simplified this block using negated `if (!EditableMapObject::ValidateWebsite(page))`
strump reviewed 2021-11-16 08:20:00 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:20:00 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-16 08:21:04 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:21:03 +00:00
Author
Member

I personally don't like such one-liners because it's hard to read. Replaced with if (regex_match(stripAtSymbol(page), s_lineRegex))

I personally don't like such one-liners because it's hard to read. Replaced with `if (regex_match(stripAtSymbol(page), s_lineRegex))`
strump reviewed 2021-11-16 08:21:17 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:21:17 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-16 08:21:26 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:21:26 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-16 08:21:33 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:21:33 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-16 08:21:39 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:21:39 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-16 08:21:45 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:21:45 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-16 08:22:01 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:22:01 +00:00
Author
Member

Updated type declaration

Updated type declaration
strump reviewed 2021-11-16 08:22:23 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:22:23 +00:00
Author
Member

Done (+ const)

Done (+ const)
strump reviewed 2021-11-16 08:22:36 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:22:36 +00:00
Author
Member

Done (+ const)

Done (+ const)
strump reviewed 2021-11-16 08:22:50 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:22:50 +00:00
Author
Member

Nedated if() statement

Nedated if() statement
strump reviewed 2021-11-16 08:23:00 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:23:00 +00:00
Author
Member

Done

Done
strump reviewed 2021-11-16 08:23:08 +00:00
indexer/validate_and_format_contacts.cpp
strump commented 2021-11-16 08:23:08 +00:00
Author
Member

Fixed

Fixed
strump commented 2021-11-16 08:25:34 +00:00
Author
Member
Copy link

It appeared that Url::FromString method handles HTTP(S) protocol correctly. So I removed all checks for "http://" and "https://" in validate_and_format_contacts.cpp code and use only Url::FromString(...). Also fixed comments and const variable definitions.
Unit-tests didn't fail, so hope nothing broke 🤞

It appeared that `Url::FromString` method handles HTTP(S) protocol correctly. So I removed all checks for `"http://"` and `"https://"` in `validate_and_format_contacts.cpp` code and use only `Url::FromString(...)`. Also fixed comments and const variable definitions. Unit-tests didn't fail, so hope nothing broke 🤞
biodranik commented 2021-11-16 12:49:31 +00:00 (Migrated from github.com)
Copy link

Current implementation of the Url class has some bugs now. For example, this works:

Url u = Url::FromString("https://");
ASSERT(u.IsValid(), ());
//
u = Url::FromString("@&€:1;asf");
ASSERT(u.IsValid(€, ());
Current implementation of the Url class has some bugs now. For example, this works: ```c++ Url u = Url::FromString("https://"); ASSERT(u.IsValid(), ()); // u = Url::FromString("@&€:1;asf"); ASSERT(u.IsValid(€, ()); ```
strump commented 2021-11-16 14:29:56 +00:00
Author
Member
Copy link

@biodranik in validate_and_format_contact.cpp code I use Url::FromString(page) only after calling EditableMapObject::ValidateWebsite(page). We can add invalid urls to unit-test

@biodranik in `validate_and_format_contact.cpp` code I use `Url::FromString(page)` only after calling `EditableMapObject::ValidateWebsite(page)`. We can add invalid urls to unit-test
vng commented 2021-11-16 21:34:52 +00:00 (Migrated from github.com)
Copy link

I don't mind to merge. Url::FromString is not changed in this PR and it's called after url validation. We don't break any constrains comparing to the master branch. If we want some refactoring, let's make separate PR, otherwise we will stuck here for a long time.

I don't mind to merge. Url::FromString is not changed in this PR and it's called after url validation. We don't break any constrains comparing to the master branch. If we want some refactoring, let's make separate PR, otherwise we will stuck here for a long time.
biodranik (Migrated from github.com) approved these changes 2021-11-16 22:03:28 +00:00
biodranik commented 2021-11-16 22:05:25 +00:00 (Migrated from github.com)
Copy link

Thank you for your huge work and time! I hope you learned something new on the way :)

Thank you for your huge work and time! I hope you learned something new on the way :)
👍 1
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
vng
biodranik
Labels
Clear labels   
Accessibility

issues affecting users with specific needs

  
Accessibility

issues affecting users with specific needs

  
Address

Issues with addresses, post codes, city/town/village borders

  
Address

Issues with addresses, post codes, city/town/village borders

  
Android

Android development

  
Android

Android development

  
Android Auto

Android Auto

  
Android Auto

Android Auto

  
Android Automotive (AAOS)

   
Android Automotive (AAOS)

   
API

Short Links, deep links and url schemes, including ge0 and om://

  
API

Short Links, deep links and url schemes, including ge0 and om://

  
AppGallery

Huawei AppGallery

  
AppGallery

Huawei AppGallery

  
AppStore

Apple AppStore

  
AppStore

Apple AppStore

  
Battery and Performance

Performance and battery consumption-related issues and optimizations

  
Battery and Performance

Performance and battery consumption-related issues and optimizations

  
Blocker

An issue blocking the release

  
Blocker

An issue blocking the release

  
Bookmarks and Tracks

Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export

  
Bookmarks and Tracks

Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export

  
Borders

Country borders and polygons

  
Borders

Country borders and polygons

  
Bug

Something isn't working

  
Bug

Something isn't working

  
Build

Compilation issue

  
Build

Compilation issue

  
CarPlay

CarPlay Integration

  
CarPlay

CarPlay Integration

  
Classificator

Classify object by types

  
Classificator

Classify object by types

  
Community

Community relations

  
Community

Community relations

  
Core

Cross-platform C++ libraries with a core functionality

  
Core

Cross-platform C++ libraries with a core functionality

  
CrashReports

Crash Reporting

  
CrashReports

Crash Reporting

  
Cycling

Bike, cycle, biking and related

  
Cycling

Bike, cycle, biking and related

  
Desktop

Desktop application

  
Desktop

Desktop application

  
DevEx

Developer Experience

  
DevEx

Developer Experience

  
DevOps

Automation and pipelines

  
DevOps

Automation and pipelines

  
dev_sandbox

A desktop app to debug rendering

  
dev_sandbox

A desktop app to debug rendering

  
Directions

Issues with next turns, labels, streets, voice instructions

  
Directions

Issues with next turns, labels, streets, voice instructions

  
Documentation

Improvements or additions to documentation

  
Documentation

Improvements or additions to documentation

  
Downloader

Map dowloader

  
Downloader

Map dowloader

  
Drape

Drape OpenGL, Vulkan and Metal graphics rendering engine

  
Drape

Drape OpenGL, Vulkan and Metal graphics rendering engine

  
Driving

Drive a car related issues

  
Driving

Drive a car related issues

  
Duplicate

This issue or pull request already exists

  
Duplicate

This issue or pull request already exists

  
Editor

OSM Editor

  
Editor

OSM Editor

  
Elevation

Isolines and altitude

  
Elevation

Isolines and altitude

  
Enhancement

New feature or request, an improvement of some existing feature

  
Enhancement

New feature or request, an improvement of some existing feature

  
Epic

A large body work

  
Epic

A large body work

  
External Map Datasets

TIGER, Ordnance Survey, ASTER, SRTM, etc.

  
External Map Datasets

TIGER, Ordnance Survey, ASTER, SRTM, etc.

  
F-Droid

F-Droid build and distribution

  
F-Droid

F-Droid build and distribution

  
Fonts

Font and text rendering

  
Fonts

Font and text rendering

  
Frequently User Reported

Most annoying UX and other issues for users reported by many people

  
Frequently User Reported

Most annoying UX and other issues for users reported by many people

  
Fund

   
Fund

   
Generator

Related to generator tool.

  
Generator

Related to generator tool.

  
Good first issue

Good for newcomers

  
Good first issue

Good for newcomers

  
Google Play

Google Play

  
Google Play

Google Play

  
GPS

Location and positioning issues

  
GPS

Location and positioning issues

  
GSoC

Ideas for Google Summer of Code

  
GSoC

Ideas for Google Summer of Code

  
iCloud

   
iCloud

   
Icons

Map and app icons

  
Icons

Map and app icons

  
iOS

iOS development

  
iOS

iOS development

  
Legal

Legal aspects

  
Legal

Legal aspects

  
Linux Desktop

Ubuntu, Gentoo, Red Hat, Debian, Cent OS, Fedora, etc.

  
Linux Desktop

Ubuntu, Gentoo, Red Hat, Debian, Cent OS, Fedora, etc.

  
Linux packaging

   
Linux packaging

   
Linux Phone

LibreM, PinePhone, Wayland and other mobile Linux versions

  
Linux Phone

LibreM, PinePhone, Wayland and other mobile Linux versions

  
Mac OS

Desktop issues related to Mac OS X, OM running on M1, M2, M3 Apple silicon, or Qt version

  
Mac OS

Desktop issues related to Mac OS X, OM running on M1, M2, M3 Apple silicon, or Qt version

  
Map Data

OpenStreetMap data related issues

  
Map Data

OpenStreetMap data related issues

  
Metro

Subway, Tube, light-rail public transport issues

  
Metro

Subway, Tube, light-rail public transport issues

  
Navigation

Issues related to the active Navigator / Navigation mode

  
Navigation

Issues related to the active Navigator / Navigation mode

  
Need Feedback

Further information is requested

  
Need Feedback

Further information is requested

  
Night Mode

Night or dark mode feature

  
Night Mode

Night or dark mode feature

  
NLnet 2024-06-281

Tasks related to NLnet 2024-06-281 project.

  
NLnet 2024-06-281

Tasks related to NLnet 2024-06-281 project.

  
No Feature Parity

A platform needs this to be implemented to have feature parity with other platforms

  
No Feature Parity

A platform needs this to be implemented to have feature parity with other platforms

  
Opening Hours

Business Hours, off time, holidays, seasonal

  
Opening Hours

Business Hours, off time, holidays, seasonal

  
Outdoors

Hiking, mtb, 4x4...

  
Outdoors

Hiking, mtb, 4x4...

  
POI Info

Feature metadata, OSM tags that are displayed in Place Page

  
POI Info

Feature metadata, OSM tags that are displayed in Place Page

  
Privacy

Privacy

  
Privacy

Privacy

  
Public Transport

Bus, trolleybus, train, tram, metro, subway, light rail

  
Public Transport

Bus, trolleybus, train, tram, metro, subway, light rail

  
Raw Idea

An idea that needs to be elaborated

  
Raw Idea

An idea that needs to be elaborated

  
Refactoring

   
Refactoring

   
Regional

This issue is different depending on the country or region

  
Regional

This issue is different depending on the country or region

  
Regression

A regression bug

  
Regression

A regression bug

  
Releases

Meta-tickets for tracking the release procedure

  
Releases

Meta-tickets for tracking the release procedure

  
RoboTest

Robo / Monkey Auto Tests

  
RoboTest

Robo / Monkey Auto Tests

  
Route Planning

Preview and plan your track

  
Route Planning

Preview and plan your track

  
Routing

Route building issues, e.g. valid route, valid ETA

  
Routing

Route building issues, e.g. valid route, valid ETA

  
Ruler

Helicopter routing, "as a crow flies"

  
Ruler

Helicopter routing, "as a crow flies"

  
Search

Search

  
Search

Search

  
Security

Security improvements

  
Security

Security improvements

  
Styles

Map drawing styles

  
Styles

Map drawing styles

  
Tests

   
Tests

   
Track Recording

Track Recording

  
Track Recording

Track Recording

  
Translations

Localization and translations issues

  
Translations

Localization and translations issues

  
TTS

Text-to-Speech

  
TTS

Text-to-Speech

  
UI

User interface issues

  
UI

User interface issues

  
UX

User eXperience, an issue with usability

  
UX

User eXperience, an issue with usability

  
Walk Navigation

Foot pedestrian mode

  
Walk Navigation

Foot pedestrian mode

  
Watches

Android Watches or Apple Watches support

  
Watches

Android Watches or Apple Watches support

  
Web

https://organicmaps.app/

  
Web

https://organicmaps.app/

  
Wikipedia

Issues related to Wiki articles embedded in OM data files.

  
Wikipedia

Issues related to Wiki articles embedded in OM data files.

  
Windows

Windows

  
Windows

Windows

  
Won't fix

Unfeasible or impossible to resolve

  
Won't fix

Unfeasible or impossible to resolve

  
World Map

Issues about improving the base map of the whole world that is bundled with the app

  
World Map

Issues about improving the base map of the whole world that is bundled with the app

No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#1545
No description provided.
Delete branch "issue-1397-android-editor-process-social-link"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?