organicmaps/organicmaps
35
Fork 24
Code Issues 2.7k Pull requests 197 Projects Releases 65 Wiki Activity Actions

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

Merged
root merged 6 commits from github/fork/strump/issue-1397-android-editor-process-social-link into master 2021-11-26 07:58:39 +00:00
Conversation 80 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:18:15 +00:00
Author
Member
Copy link

mentioned in issue #1546

mentioned in issue #1546
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 commented 2021-11-12 23:12:40 +00:00 (Migrated from github.com)
Copy link

We sort includes by alphabet inside one include block.

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

validate_and_format_contacts would be a better name.

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

This else is redundant.

This else is redundant.
biodranik commented 2021-11-12 23:20:01 +00:00 (Migrated from github.com)
Copy link
  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 commented 2021-11-12 23:20:33 +00:00 (Migrated from github.com)
Copy link

Redundant.

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

Redundant.

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

Redundant.

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

Redundant.

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

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 commented 2021-11-12 23:22:31 +00:00 (Migrated from github.com)
Copy link

Redundant.

Redundant.
biodranik commented 2021-11-12 23:26:23 +00:00 (Migrated from github.com)
Copy link

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:26:54 +00:00 (Migrated from github.com)
Copy link

approved this merge request

approved this merge request
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 commented 2021-11-15 09:43:40 +00:00
Author
Member
Copy link

renamed to validate_and_format_contacts.hpp and sorted includes alphabetially.

renamed to `validate_and_format_contacts.hpp` and sorted includes alphabetially.
strump commented 2021-11-15 09:44:08 +00:00
Author
Member
Copy link

renamed to validate_and_format_contacts

renamed to `validate_and_format_contacts`
strump commented 2021-11-15 09:44:16 +00:00
Author
Member
Copy link

Fixed

Fixed
strump commented 2021-11-15 09:44:32 +00:00
Author
Member
Copy link

Fixed

Fixed
strump commented 2021-11-15 09:44:42 +00:00
Author
Member
Copy link

Done!

Done!
strump commented 2021-11-15 09:44:50 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-15 09:44:56 +00:00
Author
Member
Copy link

Fixed

Fixed
strump commented 2021-11-15 09:45:03 +00:00
Author
Member
Copy link

Fixed

Fixed
strump commented 2021-11-15 09:45:14 +00:00
Author
Member
Copy link

Moved return to next line

Moved return to next line
strump commented 2021-11-15 09:45:21 +00:00
Author
Member
Copy link

Fixed

Fixed
strump commented 2021-11-15 09:45:32 +00:00
Author
Member
Copy link

Changed argument type

Changed argument type
vng commented 2021-11-15 11:05:05 +00:00 (Migrated from github.com)
Copy link

approved this merge request

approved this merge request
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 commented 2021-11-15 15:41:09 +00:00 (Migrated from github.com)
Copy link

nit: redundant else :)

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

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 commented 2021-11-15 15:43:54 +00:00 (Migrated from github.com)
Copy link

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

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

nit: & is redundant.

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

nit: & is redundant.

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

nit: & is redundant.

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

nit: & is redundant.

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

nit: } // namespace osm

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

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

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

nit^3: const position ;-)

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

nit: & is redundant.

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

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 commented 2021-11-15 16:03:02 +00:00 (Migrated from github.com)
Copy link

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 commented 2021-11-15 16:09:42 +00:00 (Migrated from github.com)
Copy link

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 commented 2021-11-15 16:11:07 +00:00 (Migrated from github.com)
Copy link
  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:-0+0 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 commented 2021-11-15 16:13:24 +00:00 (Migrated from github.com)
Copy link
  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:-0+0 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 commented 2021-11-15 16:14:35 +00:00 (Migrated from github.com)
Copy link

nit^2: const :)

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

If webPath is empty, the code will crash below.

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

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 commented 2021-11-15 16:26:53 +00:00 (Migrated from github.com)
Copy link

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 commented 2021-11-15 16:27:31 +00:00 (Migrated from github.com)
Copy link

See how to properly fix this crash below.

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

See how to properly fix this crash below.

See how to properly fix this crash below.
biodranik commented 2021-11-15 16:28:59 +00:00 (Migrated from github.com)
Copy link
    if (vkLogin.front() == '_' && vkLogin.back() == '_' || regex_match(vkLogin, s_badVkRegex))
      return false;
```suggestion:-0+0 if (vkLogin.front() == '_' && vkLogin.back() == '_' || regex_match(vkLogin, s_badVkRegex)) return false; ```
biodranik commented 2021-11-15 16:30:36 +00:00 (Migrated from github.com)
Copy link

Review: Changes requested

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 :)

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

**Review:** Changes requested 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 :) Нет предела совершенству!
vng commented 2021-11-15 21:16:01 +00:00 (Migrated from github.com)
Copy link

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

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

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

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

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

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

Get rid of .back() method using your advice

Get rid of `.back()` method using your advice
strump commented 2021-11-16 08:17:46 +00:00
Author
Member
Copy link

Get rid of .back() method using your advice

Get rid of `.back()` method using your advice
strump commented 2021-11-16 08:18:14 +00:00
Author
Member
Copy link

Negated if() statement

Negated if() statement
strump commented 2021-11-16 08:18:31 +00:00
Author
Member
Copy link

Get rid of .back() method using your advice

Get rid of `.back()` method using your advice
strump commented 2021-11-16 08:18:56 +00:00
Author
Member
Copy link

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

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

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

Simplified this block using negated `if (!EditableMapObject::ValidateWebsite(page))`
strump commented 2021-11-16 08:20:00 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-16 08:21:03 +00:00
Author
Member
Copy link

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 commented 2021-11-16 08:21:17 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-16 08:21:26 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-16 08:21:33 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-16 08:21:39 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-16 08:21:45 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-16 08:22:01 +00:00
Author
Member
Copy link

Updated type declaration

Updated type declaration
strump commented 2021-11-16 08:22:23 +00:00
Author
Member
Copy link

Done (+ const)

Done (+ const)
strump commented 2021-11-16 08:22:36 +00:00
Author
Member
Copy link

Done (+ const)

Done (+ const)
strump commented 2021-11-16 08:22:50 +00:00
Author
Member
Copy link

Nedated if() statement

Nedated if() statement
strump commented 2021-11-16 08:23:00 +00:00
Author
Member
Copy link

Done

Done
strump commented 2021-11-16 08:23:08 +00:00
Author
Member
Copy link

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 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 :)
biodranik (Migrated from github.com) approved these changes 2025-03-22 18:06:31 +00:00
vng (Migrated from github.com) approved these changes 2025-03-22 18:06:31 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
biodranik
vng
Labels
Clear labels   
Accessibility

   
Address

   
Android

   
Android Auto

   
Android Automotive (AAOS)

   
API

   
AppGallery

   
AppStore

   
Battery and Performance

   
Blocker

   
Bookmarks and Tracks

   
Borders

   
Bug

   
Build

   
CarPlay

   
Classificator

   
Community

   
Core

   
CrashReports

   
Cycling

   
Desktop

   
DevEx

   
DevOps

   
dev_sandbox

   
Directions

   
Documentation

   
Downloader

   
Drape

   
Driving

   
Duplicate

   
Editor

   
Elevation

   
Enhancement

   
Epic

   
External Map Datasets

   
F-Droid

   
Fonts

   
Frequently User Reported

   
Fund

   
Generator

   
Good first issue

   
Google Play

   
GPS

   
GSoC

   
iCloud

   
Icons

   
iOS

   
Legal

   
Linux Desktop

   
Linux packaging

   
Linux Phone

   
Mac OS

   
Map Data

   
Metro

   
Navigation

   
Need Feedback

   
Night Mode

   
NLnet 2024-06-281

   
No Feature Parity

   
Opening Hours

   
Outdoors

   
POI Info

   
Privacy

   
Public Transport

   
Raw Idea

   
Refactoring

   
Regional

   
Regression

   
Releases

   
RoboTest

   
Route Planning

   
Routing

   
Ruler

   
Search

   
Security

   
Styles

   
Tests

   
Track Recording

   
Translations

   
TTS

   
UI

   
UX

   
Walk Navigation

   
Watches

   
Web

   
Wikipedia

   
Windows

   
Won't fix

   
World Map
No labels
Accessibility
Address
Android
Android Auto
Android Automotive (AAOS)
API
AppGallery
AppStore
Battery and Performance
Blocker
Bookmarks and Tracks
Borders
Bug
Build
CarPlay
Classificator
Community
Core
CrashReports
Cycling
Desktop
DevEx
DevOps
dev_sandbox
Directions
Documentation
Downloader
Drape
Driving
Duplicate
Editor
Elevation
Enhancement
Epic
External Map Datasets
F-Droid
Fonts
Frequently User Reported
Fund
Generator
Good first issue
Google Play
GPS
GSoC
iCloud
Icons
iOS
Legal
Linux Desktop
Linux packaging
Linux Phone
Mac OS
Map Data
Metro
Navigation
Need Feedback
Night Mode
NLnet 2024-06-281
No Feature Parity
Opening Hours
Outdoors
POI Info
Privacy
Public Transport
Raw Idea
Refactoring
Regional
Regression
Releases
RoboTest
Route Planning
Routing
Ruler
Search
Security
Styles
Tests
Track Recording
Translations
TTS
UI
UX
Walk Navigation
Watches
Web
Wikipedia
Windows
Won't fix
World Map
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
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#1545
No description provided.
Delete branch "github/fork/strump/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?