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 string translation in Stats #10520

Merged
merged 4 commits into from
Sep 26, 2019
Merged

Conversation

planarvoid
Copy link
Contributor

@planarvoid planarvoid commented Sep 24, 2019

Fixes #8114

This fixes the issue with dates and other strings not being translated when the user changes the language within the App settings. This uses the injected ContextProvider instead of Context. The ContextProvider keeps a copy of the context which gets updated whenever the user selects a new language.

To test:

  • Have system language in English
  • Go to Me/App settings
  • Change language to something with different Month names (Czech)
  • Go to Stats
  • Check that all the strings are translated to your new language (except the strings which are missing translations in your selected language)

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@planarvoid planarvoid added this to the 13.4 milestone Sep 24, 2019
@planarvoid planarvoid requested a review from develric September 24, 2019 12:28
@planarvoid planarvoid self-assigned this Sep 24, 2019
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 24, 2019

You can test the changes on this Pull Request by downloading the APK here.

@malinajirka
Copy link
Contributor

This PR #7413 PR from Amanda might be useful for the review. I don't remember the details but I think Locale.getDefault() was causing some issues. 🤷‍♂

@planarvoid planarvoid changed the title Use default locale in LocaleManagerWrapper Fix string translation in Stats Sep 24, 2019
@planarvoid
Copy link
Contributor Author

@malinajirka I think we have to use the getSafeLocale just for strings, it's not necessary for Dates (as is this case). For the other changes I've followed the example of other parts of the app.


@Singleton
class ContextProvider
@Inject constructor(private var context: Context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Like this Singleton Provider approach 👍


fun getContext(): Context {
return context
}
Copy link
Contributor

@develric develric Sep 26, 2019

Choose a reason for hiding this comment

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

Really minor np: what about using expression body function here? Eventually keeping explicitly the return data type for readability. I'm in any case fine with as it is already (basically a matter of prefs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good 👍 anything that reduces the boilerplate code is a good change :-)

@develric
Copy link
Contributor

develric commented Sep 26, 2019

Hi @planarvoid, I followed the testing steps with Android API level 21 and 28 (with and without this patch) and it worked as expected. Good job! 👍

I think we have to use the getSafeLocale just for strings, it's not necessary for Dates (as is this case). For the other changes I've followed the example of other parts of the app.

This is also my guess also based on this comment. I'm not fully sure to get all the possible implications of the modification in ResourceProvider class in places like this and in general outside of stats code. TBH it's just a gut feeling and did not encounter any issue while testing, but I wanted to mention in case you fill it can be wort to have another couple of eyes on that part.

I left an extremely minor np in the code comments if you want to consider but you can leave as it is if you prefer.

During testing with develop branch without this patch I found an issue when selecting RTL languages (for example Hebrew). Basically the order of quick actions buttons and of the bottom nav bar items are not updated in RTL order with API > 24 unless you restart the app. Don't know if it was intentional but this PR seems to fix also that. I will open a dedicated issue to track this but wanted to mention in case you prefer to look into it in this PR explicitly, let me know 😊.

@planarvoid
Copy link
Contributor Author

I'm not fully sure to get all the possible implications of the modification in ResourceProvider class in places like this and in general outside of stats code.

I don't think there should be any problem with the ResourceProvider. What I've done is that I've replaced the injected Application context with the same context from the provider. The only difference is that it's now updated when we change the language.

Don't know if it was intentional but this PR seems to fix also that.

We use the ResourceProvider all over the place so I think it will fix more stuff than we think :-)

Copy link
Contributor

@develric develric 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 this @planarvoid ! LGTM! 👍

@develric develric merged commit 1cd3fdd into develop Sep 26, 2019
@develric develric deleted the fix/use_default_locale_in_stats branch September 27, 2019 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some strings use the previous language when it is changed in app on API > 24
3 participants