[ios] Change text "X objects" to "Y places, Z tracks" in Bookmarks and Tracks dialog #8245

kirylkaveryn · 2024-05-21T19:27:08Z

itfarrier commented

2024-05-21 19:27:08 +00:00

(Migrated from github.com)

Refactor using a switch statement

Fixes: #8217

- Refactor using a switch statement Fixes: #8217

biodranik (Migrated from github.com) reviewed 2024-05-21 20:34:17 +00:00

iphone/Maps/Categories/MWMCategory+PlacesCountTitle.swift

biodranik (Migrated from github.com) commented

2024-05-21 20:34:09 +00:00

Switch is less readable here, and it also has lookingly unnecessary case. When is this default used?

if (bookmarksCount)
{
  if (tracksCount)
    return bookmarks + tracks;
  return bookmarks;
}
return tracks;

Switch is less readable here, and it also has lookingly unnecessary case. When is this default used? ``` if (bookmarksCount) { if (tracksCount) return bookmarks + tracks; return bookmarks; } return tracks; ```

biodranik (Migrated from github.com) reviewed 2024-05-21 20:39:40 +00:00

iphone/Maps/Categories/MWMCategory+PlacesCountTitle.swift

					
				@ -11,1 +5,3 @@

				       return String(format: L("tracks"), trackCount)

				    let bookmarks = String(format: L("bookmarks_places"), bookmarksCount)

				    let tracks = String(format: L("tracks"), trackCount)

biodranik (Migrated from github.com) commented

2024-05-21 20:39:40 +00:00

I think that [objects] string can be removed from strings.txt as unused, right?

biodranik (Migrated from github.com) reviewed 2024-05-27 20:51:33 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! We usually split string regeneration commit into a separate one, to help with merge conflicts. Do you know how to amend commit and split it into two?

biodranik (Migrated from github.com) approved these changes 2024-05-28 23:15:05 +00:00

biodranik (Migrated from github.com) left a comment

Thanks for your patience and contribution!

Дзякуй!

Thanks for your patience and contribution! Дзякуй!

biodranik (Migrated from github.com) requested changes 2024-05-28 23:30:31 +00:00

biodranik (Migrated from github.com) left a comment

Ooops, sorry, I've somehow missed failed Android checks, there is one usage of the deleted string:

      if (mEntity.size() == 0)
        return getQuantified(resources, R.plurals.objects, 0);

We can make it better for the zero case (btw, it should also иу handled in the iOS code).

What about changing Android code to use bookmarks_empty_list_title translation and add the same translation to iOS when there are 0 items?

Ooops, sorry, I've somehow missed [failed Android checks](https://github.com/organicmaps/organicmaps/actions/runs/9273328462), there is one usage of the deleted string: ``` if (mEntity.size() == 0) return getQuantified(resources, R.plurals.objects, 0); ``` We can make it better for the zero case (btw, it should also иу handled in the iOS code). What about changing Android code to use `bookmarks_empty_list_title` translation and add the same translation to iOS when there are 0 items?

biodranik (Migrated from github.com) reviewed 2024-05-31 20:46:11 +00:00

biodranik (Migrated from github.com) left a comment

Looks like translations for 1 bookmark or 1 track are incorrect, can you please check it/fix it too?

Looks like translations for 1 bookmark or 1 track are incorrect, can you please check it/fix it too? ![выява](https://github.com/organicmaps/organicmaps/assets/170263/93308f59-c57d-4f90-bf7c-846961189df0)

android/app/src/main/java/app/organicmaps/bookmarks/Holders.java

biodranik (Migrated from github.com) commented

2024-05-31 20:42:04 +00:00

It would be great to avoid creating a string if it's not needed, by moving this code into the condition.

biodranik (Migrated from github.com) commented

2024-05-31 20:42:57 +00:00

It's better to avoid reformatting existing code.
It's better to follow the existing code style instead of mixing it.

1. It's better to avoid reformatting existing code. 2. It's better to follow the existing code style instead of mixing it.

biodranik (Migrated from github.com) commented

2024-05-31 20:43:25 +00:00

Braces are not needed for one-liners.

biodranik (Migrated from github.com) reviewed 2024-06-09 22:20:57 +00:00

android/app/src/main/java/app/organicmaps/bookmarks/Holders.java

biodranik (Migrated from github.com) commented

2024-06-09 22:20:39 +00:00

iOS uses bookmarks_places string, it would be better to use it on Android too, and delete the places string.

iOS uses `bookmarks_places` string, it would be better to use it on Android too, and delete the `places` string.

iphone/Maps/Categories/MWMCategory+PlacesCountTitle.swift

biodranik (Migrated from github.com) commented

2024-06-09 22:19:52 +00:00

    if (bookmarksCount == 0 && trackCount == 0 || bookmarksCount > 0 && trackCount > 0) {
      return String(format: L("comma_separated_pair"),
                    String(format: L("bookmarks_places"), bookmarksCount),
                    String(format: L("tracks"), trackCount))
    }

    if (bookmarksCount > 0) {
      return String(format: L("bookmarks_places"), bookmarksCount)
    }

    return String(format: L("tracks"), trackCount)

@kirylkaveryn for some reason, singular forms are not printed with this code on iOS 15 simulator. Do you have any ideas why it doesn't work?

```suggestion if (bookmarksCount == 0 && trackCount == 0 || bookmarksCount > 0 && trackCount > 0) { return String(format: L("comma_separated_pair"), String(format: L("bookmarks_places"), bookmarksCount), String(format: L("tracks"), trackCount)) } if (bookmarksCount > 0) { return String(format: L("bookmarks_places"), bookmarksCount) } return String(format: L("tracks"), trackCount) ``` @kirylkaveryn for some reason, singular forms are not printed with this code on iOS 15 simulator. Do you have any ideas why it doesn't work?

kirylkaveryn reviewed 2024-06-10 07:27:52 +00:00

iphone/Maps/Categories/MWMCategory+PlacesCountTitle.swift

kirylkaveryn commented

2024-06-10 07:27:52 +00:00

Hmm it seems like the plurals have only the other field and miss the one for EN.

I didn't check the other translations.

Hmm it seems like the plurals have only the `other` field and miss the `one` for EN. I didn't check the other translations. ![image](https://github.com/organicmaps/organicmaps/assets/79797627/1416b41f-8bfb-42da-b3d9-e628ddd409b5)

biodranik (Migrated from github.com) reviewed 2024-06-30 14:11:38 +00:00

iphone/Maps/Categories/MWMCategory+PlacesCountTitle.swift

biodranik (Migrated from github.com) commented

2024-06-30 14:11:38 +00:00

@kirylkaveryn thanks for the hint. Looks like our Twine tool does not generate plurals from a generic language (en) into sublanguage (en-GB):

diff iphone/Maps/LocalizedStrings/en.lproj/Localizable.stringsdict iphone/Maps/LocalizedStrings/en-GB.lproj/Localizable.stringsdict
6c6
<  * Language: en -->
---
>  * Language: en-GB -->
22,23d21
<       <key>one</key>
<       <string>%d bookmark</string>
39,40d36
<       <key>one</key>
<       <string>%d file was found. You can see it after conversion.</string>
56,57d51
<       <key>one</key>
<       <string>%d track</string>

@kirylkaveryn thanks for the hint. Looks like our Twine tool does not generate plurals from a generic language (en) into sublanguage (en-GB): ``` diff iphone/Maps/LocalizedStrings/en.lproj/Localizable.stringsdict iphone/Maps/LocalizedStrings/en-GB.lproj/Localizable.stringsdict 6c6 < * Language: en --> --- > * Language: en-GB --> 22,23d21 < <key>one</key> < <string>%d bookmark</string> 39,40d36 < <key>one</key> < <string>%d file was found. You can see it after conversion.</string> 56,57d51 < <key>one</key> < <string>%d track</string> ```

biodranik (Migrated from github.com) approved these changes 2024-06-30 19:20:00 +00:00

biodranik (Migrated from github.com) left a comment

Thanks for your hard work!

.git-blame-ignore-revs

					
				@ -1,3 +1,4 @@

				6aa73face8b5eb8e026cfafa40d1983d4a0502c0

				480fa6c2fcf53be296504ac6ba8e6b3d70f92b42

				a6ede2b1466f0c9d8a443600ef337ba6b5832e58

				1377b81bf1cac72bb6da192da7fed6696d5d5281

biodranik (Migrated from github.com) commented

2024-06-30 19:19:50 +00:00

This is to have a cleaner git history for data/strings.txt

vng (Migrated from github.com) reviewed 2024-07-01 01:03:13 +00:00

.git-blame-ignore-revs

					
				@ -1,3 +1,4 @@

				6aa73face8b5eb8e026cfafa40d1983d4a0502c0

				480fa6c2fcf53be296504ac6ba8e6b3d70f92b42

				a6ede2b1466f0c9d8a443600ef337ba6b5832e58

				1377b81bf1cac72bb6da192da7fed6696d5d5281

vng (Migrated from github.com) commented

2024-07-01 01:01:41 +00:00

I prefer to keep history. I see that other strings also was affected (accidently?)

android/app/src/main/java/app/organicmaps/bookmarks/Holders.java

vng (Migrated from github.com) commented

2024-07-01 00:56:51 +00:00

Please, put additional braces to clear show what you mean (yes, I know that || goes after &&, but anyway).

data/strings/strings.txt

vng (Migrated from github.com) commented

2024-07-01 00:57:38 +00:00

? Unprintable symbols here and below.

data/strings/strings.txt

vng (Migrated from github.com) commented

2024-07-01 00:57:44 +00:00

? Unprintable symbols here and below.

biodranik (Migrated from github.com) reviewed 2024-07-01 07:36:51 +00:00

data/strings/strings.txt

biodranik (Migrated from github.com) commented

2024-07-01 07:36:51 +00:00

~~Oh wow, good catch! @itfarrier which editor did you use to edit strings.txt?~~

Looks like it's our python script that cleans strings breaks the encoding. I'll take a look.

~Oh wow, good catch! @itfarrier which editor did you use to edit strings.txt?~ Looks like it's our python script that cleans strings breaks the encoding. I'll take a look.