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

Fix locale of every String.format number formatter #4021

Merged
merged 1 commit into from Apr 9, 2020

Conversation

ebraminio
Copy link
Contributor

@ebraminio ebraminio commented Apr 8, 2020

Split from #4020, searched %d, %.1f, %.2f entire the project to see if locale String.format has correct locale, which includes fixes on some SQL query fixes on locales don't use 123 digits (۱۲۳), URL builders that should've used 123 in any locale, improvements on logs.

Rule of thumb, use Locale.getDefault() everywhere string goes directly to view and use Locale.US (or Locale.ENGLISH, the former was used by the project already) every other place like building a SQL query or URL (and maybe logs as they are in English anyway)

@@ -99,7 +100,7 @@ private void setupUi() {
final TextView txtvPlaybackSpeed = dialog.findViewById(R.id.txtvPlaybackSpeed);
float currentSpeed = getCurrentSpeed();

txtvPlaybackSpeed.setText(String.format("%.2fx", currentSpeed));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were using Locale.getDefault() by default but Android lint, correctly, asks to make requested locale explicit.

@ByteHamster
Copy link
Member

Woah, looks like the current production version is really broken in languages that do not use western Arabic numbers... Thanks a lot for your contribution!

Would you be interested in changing other occurrences of numbers, too? While trying your PR, I noticed that the following locations still use western Arabic numbers:

  • File size
  • Episode numbers in the sidebar
  • Episode numbers on the subscriptions screen
  • (Episode count in the queue infobar) I will change that when reworking String concatenation

@ebraminio
Copy link
Contributor Author

ebraminio commented Apr 8, 2020

Would you be interested in changing other occurrences of numbers, too? While trying your PR, I noticed that the following locations still use western Arabic numbers:

Sure! Did one of them bcb529e wanted to add another PR for it.

Woah, looks like the current production version is really broken in languages that do not use western Arabic numbers... Thanks a lot for your contribution!

It isn't that bad. AntennaPod was my main podcast player for several months without hitting these issues or I was ignoring them unknowingly anyway. You should've seen this particular issue on Google Play crash analytics however I believe, I use that for discovering my FOSS app issues which the app itself doesn't have internet access let alone other Google-y things as it is provided by Google Play for all the apps.

@ebraminio
Copy link
Contributor Author

ebraminio commented Apr 8, 2020

File size

This should be used https://developer.android.com/reference/kotlin/android/text/format/Formatter#formatShortFileSize(android.content.Context,%20kotlin.Long) which is totally localized.

Episode numbers in the sidebar

By bcb529e but guess we should inspect every String.valueOf.

Episode numbers on the subscriptions screen

Guess was covered by d938ed2 not sure however, probably is another String.valueOf use.

@ByteHamster
Copy link
Member

Sure! Did one of them bcb529e wanted to add another PR for it.

So can we merge this one or do you want to add those changes here?

@ebraminio
Copy link
Contributor Author

ebraminio commented Apr 9, 2020 via email

@ByteHamster ByteHamster merged commit d1a9190 into AntennaPod:develop Apr 9, 2020
@ByteHamster
Copy link
Member

Thanks a lot for your PR!

@ebraminio ebraminio deleted the sql branch April 9, 2020 16:06
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.

None yet

2 participants