organicmaps/organicmaps-tmp

Archived

Fork 1

[gpx] Use string_view for bookmark file extensions #5284

Merged

cyber-toad merged 7 commits from gpx-sv into master

2023-06-14 12:16:40 +00:00

cyber-toad commented

2023-06-06 21:49:07 +00:00

(Migrated from github.com)

organicmaps/organicmaps#5166/files

https://git.omaps.dev/organicmaps/organicmaps/pulls/5166/files#r1194353821

biodranik (Migrated from github.com) approved these changes 2023-06-06 22:05:46 +00:00

biodranik (Migrated from github.com) left a comment

Awesome, thank you!

biodranik (Migrated from github.com) requested changes 2023-06-06 22:06:36 +00:00

biodranik (Migrated from github.com) left a comment

Uups, some compile errors )

vng (Migrated from github.com) requested changes 2023-06-07 13:45:20 +00:00

map/bookmark_helpers.cpp

vng (Migrated from github.com) commented

2023-06-07 13:45:13 +00:00

No need here. Instead of using one-time created string constants, now we create std::string(ext) object every time when calling this function.

No need here. Instead of using _one-time_ created string constants, now we create std::string(ext) object every time when calling this function.

vng (Migrated from github.com) reviewed 2023-06-07 13:46:52 +00:00

android/jni/app/organicmaps/Framework.cpp

vng (Migrated from github.com) commented

2023-06-07 13:46:52 +00:00

std::array<std::string_view, 4> exts = ...

cyber-toad (Migrated from github.com) reviewed 2023-06-07 18:39:44 +00:00

map/bookmark_helpers.cpp

cyber-toad (Migrated from github.com) commented

2023-06-07 18:39:44 +00:00

In this case what is a valid way to change type of extensions if we have

std::string GenerateUniqueFileName(const std::string & path, std::string name, std::string_view ext = kKmlExtension);

where kKmlExtension is a string_view? How it should be converted to a string to be concatenated inside GenerateUniqueFileName?

In this case what is a valid way to change type of extensions if we have `std::string GenerateUniqueFileName(const std::string & path, std::string name, std::string_view ext = kKmlExtension);` where kKmlExtension is a string_view? How it should be converted to a string to be concatenated inside GenerateUniqueFileName?

biodranik (Migrated from github.com) reviewed 2023-06-07 20:13:59 +00:00

map/bookmark_helpers.cpp

biodranik (Migrated from github.com) commented

2023-06-07 20:13:59 +00:00

Doesn't string.append() work?

vng (Migrated from github.com) approved these changes 2023-06-08 18:48:38 +00:00

vng (Migrated from github.com) reviewed 2023-06-08 18:51:53 +00:00

map/bookmark_helpers.cpp

vng (Migrated from github.com) commented

2023-06-08 18:51:50 +00:00

There is a bug here. We modify name var, but should not.

Looks like we are making very controversial changes for some very shadow profit ..

There is a bug here. We modify name var, but should not. Looks like we are making very controversial changes for some very _shadow_ profit ..

cyber-toad (Migrated from github.com) reviewed 2023-06-08 19:35:06 +00:00

map/bookmark_helpers.cpp

cyber-toad (Migrated from github.com) commented

2023-06-08 19:35:05 +00:00

Yep, this issue was caught by unit-test, I'm fixing it.

vng (Migrated from github.com) reviewed 2023-06-09 10:14:54 +00:00

android/jni/app/organicmaps/Framework.cpp

vng (Migrated from github.com) commented

2023-06-09 10:11:20 +00:00

looks like its not compiling

map/bookmark_helpers.cpp

vng (Migrated from github.com) commented

2023-06-09 10:14:18 +00:00

@biodranik I still think that we are doing strange changes for no reason.
If you insist:

  size_t counter = 1;
  std::string suffix, res;
  do
  {
    res = name;
    res = base::JoinPath(path, res.append(suffix).append(ext));
    if (!Platform::IsFileExistsByFullPath(res))
      break;
    suffix = strings::to_string(counter++);
  } while (true);

  return res;

@biodranik I still think that we are doing strange changes for no reason. If you insist: ``` size_t counter = 1; std::string suffix, res; do { res = name; res = base::JoinPath(path, res.append(suffix).append(ext)); if (!Platform::IsFileExistsByFullPath(res)) break; suffix = strings::to_string(counter++); } while (true); return res; ```

biodranik (Migrated from github.com) reviewed 2023-06-09 13:54:46 +00:00

map/bookmark_helpers.cpp

biodranik (Migrated from github.com) commented

2023-06-09 13:54:46 +00:00

I'm not insisting, I assumed that it can be easy, but looks like the reality is worse )

Anyway, if we are already here, it makes sense to fix it properly. I hope that @cyber-toad learned something new in the process )

I'm not insisting, I assumed that it can be easy, but looks like the reality is worse ) Anyway, if we are already here, it makes sense to fix it properly. I hope that @cyber-toad learned something new in the process )