Fixed non-working Wikimedia links with ? character in a title #8618

2024-07-02T19:09:06Z

alnzrv commented

2024-07-02 19:09:06 +00:00

(Migrated from github.com)

Should fix organicmaps/organicmaps#8505

Should fix https://git.omaps.dev/organicmaps/organicmaps/issues/8505

biodranik (Migrated from github.com) approved these changes 2024-07-02 19:27:53 +00:00

biodranik (Migrated from github.com) left a comment

Thanks!

@vng PTAL

Thanks! @vng PTAL

indexer/feature_meta.cpp

					
				@ -58,3 +46,4 @@

				  return "https://commons.wikimedia.org/wiki/" + v;

				}

				std::string Metadata::EncodeWikiURL(std::string v, int startIndex) {

biodranik (Migrated from github.com) commented

2024-07-02 19:25:53 +00:00

nit:

      v.insert(++i, "3F");  // ? => %3F

nit: ```suggestion v.insert(++i, "3F"); // ? => %3F ```

biodranik (Migrated from github.com) commented

2024-07-02 19:26:00 +00:00

std::string Metadata::ToWikimediaCommonsURL(std::string v)

```suggestion std::string Metadata::ToWikimediaCommonsURL(std::string v) ```

biodranik (Migrated from github.com) reviewed 2024-07-03 08:08:22 +00:00

generator/generator_tests/metadata_parser_test.cpp

biodranik (Migrated from github.com) commented

2024-07-03 08:08:22 +00:00

Are URLs with spaces opened properly on iOS and Android?

alnzrv (Migrated from github.com) reviewed 2024-07-03 09:53:41 +00:00

generator/generator_tests/metadata_parser_test.cpp

alnzrv (Migrated from github.com) commented

2024-07-03 09:53:41 +00:00

Yes, there is a redirect from ' ' to '':
https://commons.wikimedia.org/wiki/Category:What_If%3F(Bonn)

Yes, there is a redirect from ' ' to '_': https://commons.wikimedia.org/wiki/Category:What_If%3F_(Bonn)

biodranik (Migrated from github.com) reviewed 2024-07-03 09:59:09 +00:00

generator/generator_tests/metadata_parser_test.cpp

biodranik (Migrated from github.com) commented

2024-07-03 09:59:09 +00:00

Redirect means a slower server response. Does it make sense to replace spaces with underscores too? What other symbols should be escaped? Is there a full list of all category values to test?

biodranik (Migrated from github.com) reviewed 2024-07-04 20:57:20 +00:00

indexer/feature_meta.cpp

					
				@ -58,3 +46,4 @@

				  return "https://commons.wikimedia.org/wiki/" + v;

				}

				std::string Metadata::EncodeWikiURL(std::string v, int startIndex) {

biodranik (Migrated from github.com) commented

2024-07-04 20:57:20 +00:00

If this code is similar/the same as Metadata::ToWikiURL, please move it into a reusable static function (or into the anonymous namespace), and call it from both these methods.

If this code is similar/the same as `Metadata::ToWikiURL`, please move it into a reusable static function (or into the anonymous namespace), and call it from both these methods.

alnzrv (Migrated from github.com) reviewed 2024-07-04 21:45:31 +00:00

indexer/feature_meta.cpp

					
				@ -58,3 +46,4 @@

				  return "https://commons.wikimedia.org/wiki/" + v;

				}

				std::string Metadata::EncodeWikiURL(std::string v, int startIndex) {

alnzrv (Migrated from github.com) commented

2024-07-04 21:45:31 +00:00

These code fragments are similar except '%' encoding for Wikipedia title:

else if (c == '%')
  v.insert(i + 1, "25");  // % => %25

It was added here:
30d1880a6f (diff-2965b683212a4216733d4b452d86780e807d95340d8577ff497ddc090306f13bR24)

I have a feeling it's not needed for Wikipedia links too but I don't wanna test what happens if we remove it.

If '%' encoding is really required for Wikipedia links, I don't think moving space and '?' encoding into a separate function worth it.

These code fragments are similar except '%' encoding for Wikipedia title: ```c++ else if (c == '%') v.insert(i + 1, "25"); // % => %25 ``` It was added here: https://git.omaps.dev/organicmaps/organicmaps/commit/30d1880a6fe5f15add03a59cfa04411bdfda1d36#diff-2965b683212a4216733d4b452d86780e807d95340d8577ff497ddc090306f13bR24 I have a feeling it's not needed for Wikipedia links too but I don't wanna test what happens if we remove it. If '%' encoding is really required for Wikipedia links, I don't think moving space and '?' encoding into a separate function worth it.

biodranik (Migrated from github.com) reviewed 2024-08-05 04:58:03 +00:00

indexer/feature_meta.cpp

					
				@ -58,3 +46,4 @@

				  return "https://commons.wikimedia.org/wiki/" + v;

				}

				std::string Metadata::EncodeWikiURL(std::string v, int startIndex) {

biodranik (Migrated from github.com) commented

2024-08-05 04:58:03 +00:00

I'm not following the logic.

Why percent escaping is not needed?
Why the same code should not be reused but instead copy-pasted?

I'm not following the logic. 1. Why percent escaping is not needed? 2. Why the same code should not be reused but instead copy-pasted?

biodranik (Migrated from github.com) requested changes 2024-08-29 09:02:30 +00:00

biodranik (Migrated from github.com) left a comment

Any update here?

biodranik (Migrated from github.com) requested changes 2024-11-09 17:54:34 +00:00

biodranik (Migrated from github.com) left a comment

Can you please rebase?

indexer/feature_meta.cpp

biodranik (Migrated from github.com) commented

2024-11-09 17:51:24 +00:00

The usage pattern suggests to avoid unnecessary copying:

void Metadata::EncodeWikiURL(int startIndex, std::string & url)
{

The usage pattern suggests to avoid unnecessary copying: ```suggestion void Metadata::EncodeWikiURL(int startIndex, std::string & url) { ```

This repo is archived. You cannot comment on pull requests.

No reviewers

biodranik

No labels

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

External Map Datasets

F-Droid

Fonts

Frequently User Reported

No milestone

No project

No assignees

1 participant

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#8618

Rows
Columns