Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

itfarrier
Copy link
Contributor

  • Refactor using a switch statement

Fixes: #8217

@itfarrier itfarrier force-pushed the 8217 branch 6 times, most recently from ccc1a0c to 4fc5120 Compare May 26, 2024 18:20
@itfarrier
Copy link
Contributor Author

@biodranik, hello and sorry for pushing, maybe you saw and forgot mentions in this thread. Could you please answer conversations above?

@biodranik
Copy link
Member

hello and sorry for pushing, maybe you saw and forgot mentions in this thread. Could you please answer conversations above?

Both my comments are not resolved, or am I missing something?

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from b6ec65e to 9dbb601 Compare May 27, 2024 15:51
@itfarrier
Copy link
Contributor Author

hello and sorry for pushing, maybe you saw and forgot mentions in this thread. Could you please answer conversations above?

Both my comments are not resolved, or am I missing something?

Hello, @biodranik, I resolved your threads. And my latest question was "What about using "(String(format: L("bookmarks_places"), bookmarksCount)), (String(format: L("tracks"), trackCount))" all the time? I mean no conditions."

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from d56c272 to 914ce76 Compare May 28, 2024 05:29
@itfarrier
Copy link
Contributor Author

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, hello and pls check.

@biodranik
Copy link
Member

Did you remove strings manually? There is a tools/unix/generate_localizations.sh script for it.

  1. The first commit should have your changes + cleanup in strings.txt
  2. Then you run the script
  3. Then create a second commit with automatically regenerated localization files and title [strings] Regenerated.

@biodranik biodranik added this to the Needs alpha/beta testing milestone May 28, 2024
@itfarrier itfarrier force-pushed the 8217 branch 3 times, most recently from ff2b1b2 to ed60a67 Compare May 28, 2024 16:52
@itfarrier
Copy link
Contributor Author

Did you remove strings manually? There is a tools/unix/generate_localizations.sh script for it.

  1. The first commit should have your changes + cleanup in strings.txt
  2. Then you run the script
  3. Then create a second commit with automatically regenerated localization files and title [strings] Regenerated.

@biodranik, done.

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience and contribution!

Дзякуй!

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@itfarrier
Copy link
Contributor Author

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?

@biodranik, hello. These are current situation in both platforms:

Android:

      final Resources resources = mSize.getResources();
      final int bookmarksCount = mEntity.getBookmarksCount();
      final int tracksCount = mEntity.getTracksCount();

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

      if (bookmarksCount > 0 && tracksCount > 0)
      {
        final String bookmarks = getQuantified(resources, R.plurals.places, bookmarksCount);
        final String tracks = getQuantified(resources, R.plurals.tracks, tracksCount);
        final String template = resources.getString(R.string.comma_separated_pair);
        return String.format(template, bookmarks, tracks);
      }

      if (bookmarksCount > 0)
        return getQuantified(resources, R.plurals.places, bookmarksCount);

      return getQuantified(resources, R.plurals.tracks, tracksCount);

iOS:

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

My proposal is (String(format: L("bookmarks_places"), bookmarksCount)), (String(format: L("tracks"), trackCount)) on both platforms for all cases. Please, give your opinion.

@biodranik
Copy link
Member

@itfarrier I like it!

@biodranik
Copy link
Member

Looks like it was a misunderstanding. The code should print:

0 bookmarks, 0 tracks
10 bookmarks, 5 tracks
5 bookmarks <--
6 tracks <--

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from 54961fc to e0976b8 Compare May 31, 2024 14:48
@itfarrier
Copy link
Contributor Author

Looks like it was a misunderstanding. The code should print:

0 bookmarks, 0 tracks 10 bookmarks, 5 tracks 5 bookmarks <-- 6 tracks <--

@biodranik, please, see.

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

выява

@biodranik
Copy link
Member

Note that Android correctly used the combining string comma_separated_pair, which may differ for different languages.

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from bfb04f1 to 7199568 Compare June 1, 2024 14:21
@itfarrier
Copy link
Contributor Author

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

выява

@biodranik, fixed. That was my bad, sorry.

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from 3bc6040 to dde9435 Compare June 4, 2024 08:34
@itfarrier
Copy link
Contributor Author

@biodranik, could you please review?

@biodranik
Copy link
Member

Thanks, on iOS tracks and bookmarks are not using singular/plural forms properly. Are strings properly regenerated?

image

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from cb6ff87 to 15dca90 Compare June 6, 2024 10:51
@itfarrier
Copy link
Contributor Author

@biodranik

Simulator Screenshot - iPhone 15 Pro Max - 2024-06-06 at 11 39 33
Simulator Screenshot - iPhone 15 Pro Max - 2024-06-06 at 11 39 21
Simulator Screenshot - iPhone 15 Pro Max - 2024-06-06 at 11 39 11
Simulator Screenshot - iPhone 15 Pro Max - 2024-06-06 at 11 38 52
Simulator Screenshot - iPhone 15 Pro Max - 2024-06-06 at 11 38 38
Simulator Screenshot - iPhone 15 Pro Max - 2024-06-06 at 11 38 28

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from 8d42649 to 405efcb Compare June 7, 2024 17:14
Comment on lines +5 to +17
let bookmarks = String(format: L("bookmarks_places"), bookmarksCount)
let tracks = String(format: L("tracks"), trackCount)

if (bookmarksCount == 0 && trackCount == 0) || (bookmarksCount > 0 && trackCount > 0) {
return "\(bookmarks), \(tracks)"
}

if (bookmarksCount > 0) {
return bookmarks
}

return tracks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let bookmarks = String(format: L("bookmarks_places"), bookmarksCount)
let tracks = String(format: L("tracks"), trackCount)
if (bookmarksCount == 0 && trackCount == 0) || (bookmarksCount > 0 && trackCount > 0) {
return "\(bookmarks), \(tracks)"
}
if (bookmarksCount > 0) {
return bookmarks
}
return tracks
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?

Copy link
Contributor

@kirylkaveryn kirylkaveryn Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>

return getQuantified(resources, R.plurals.objects, 0);

if (bookmarksCount > 0 && tracksCount > 0)
if (bookmarksCount == 0 && tracksCount == 0 || bookmarksCount > 0 && tracksCount > 0)
{
final String bookmarks = getQuantified(resources, R.plurals.places, bookmarksCount);
Copy link
Member

@biodranik biodranik Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@biodranik
Copy link
Member

You also need to add this change and then regenerate strings with tools/unix/generate_localizations.sh

   [comma_separated_pair]
     comment = A number of bookmarks and a number of tracks, separated by comma, like: 1 bookmark, 5 tracks
-    tags = android
+    tags = android,ios
     en = %@, %@

@itfarrier itfarrier force-pushed the 8217 branch 2 times, most recently from e12ea4b to 50a7947 Compare June 14, 2024 08:20
@biodranik biodranik mentioned this pull request Jun 22, 2024
@biodranik
Copy link
Member

@itfarrier do you need any help with the PR?

@itfarrier
Copy link
Contributor Author

@itfarrier do you need any help with the PR?

Hello. Not for now. I've just change the work and now too much loading on it, sorry.

itfarrier and others added 2 commits June 30, 2024 18:11
- Remove “objects” from strings.txt;
- Add condition flow for Android;
- Add condition flow for iOS.

Fixes: organicmaps#8217

Signed-off-by: Dzmitry Padabed <itfarrier@icloud.com>
Signed-off-by: Alexander Borsuk <me@alex.bio>
@biodranik
Copy link
Member

@itfarrier no problem, I've finished the PR. @vng PTAL

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your hard work!

@@ -1,3 +1,4 @@
6aa73face8b5eb8e026cfafa40d1983d4a0502c0
480fa6c2fcf53be296504ac6ba8e6b3d70f92b42
a6ede2b1466f0c9d8a443600ef337ba6b5832e58
1377b81bf1cac72bb6da192da7fed6696d5d5281
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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