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 custom app language selection issues across various android versions #7413

Merged
merged 16 commits into from
Mar 9, 2018

Conversation

AmandaRiu
Copy link
Contributor

@AmandaRiu AmandaRiu commented Mar 7, 2018

Fixes #7267, #7265

Updating the language of the App in real time is handled differently across different versions of Android. This PR centralizes language updates to a few new classes:

  • BaseActivity
  • BaseListActivity
  • LocaleManager

Any Activity that should be sensitive to Locale changes should extend either BaseActivity or BaseListActivity.

LocaleManager contains a set of helper methods/utilities for properly persisting, fetching and applying locale settings appropriate to the active android version.

AppSettingsFragment now calls System.exit(0) to force an instance restart of the app so the language of the app is updated instantly, and to give a consistent experience across all versions of Android.

Once localization was updated to be carried with the active context, I ran into an issue where devices running API 24 & 25 would suddenly crash with a fatal "division by zero" exception while attempting to parse and format a string containing locale-specific grouping separators. For example, the following string: %,d people like this would cause a fatal crash while attempting to format it using String.format(baseString, val). Turns out this is a known Java bug for Android 7.x.

The only way around this bug is to pass in the locale to String.format(), BUT, Locale.getDefault() will not work when the language property of the Locale object contains any regional information. Specifically, it will only work if language=en and will not work if language=en_US. Created LocaleManager.getSafeLocale(context) to obtain a properly formatted locale object for the workaround and updated any places where this may be a problem by searching for %,d in strings.xml.

Fix tested on emulators running the following versions: 17, 19, 22, 23, 24, 25, 26, 27

Screenshots

API 19

1_screenshot_1520393588 1_screenshot_1520393437

API 26

1_screenshot_1520394446 1_screenshot_1520394454 1_screenshot_1520394451 1_screenshot_1520394458 1_screenshot_1520394465

To test:

  1. Change language to Hebrew in App Settings.
  2. On devices running 17+, verify RTL layout change.
  3. Verify the language is updated througout the app. Pay special attention to:
    - Labels are updated in the filter bar of the notification tab.
    - Tag options dropdown are updated in the Reader.
  4. Change device orientation.
  5. Verify the language change persists.
  6. Kill app and reopen.
  7. Verify the language change persists.
  8. Change the language back to English.
  9. repeat steps 2 - 4.

AmandaRiu and others added 3 commits March 6, 2018 21:50
* Retain custom language selection across application kill/restarts on devices running api 24 and above.
* Instantly apply the language selection and update all views on devices running api 23 and less.
* Change to the app language now consistent across all versions of android.
* Update the title of MediaBrowserActivity to use the localized string.

Tickets: #7267, #7265
@oguzkocer
Copy link
Contributor

We have been discussing this issue on Slack and I'll just list the alternative solutions we can go about it in the order of my own preference:

  1. Drop the language option all together. @nbradbury reminded us that we didn't do that before because we are supporting some languages that doesn't exist in the Android. I'd check the usage rate of these languages and consider if it's worth having the feature.
  2. Drop the support for changing language for api levels that don't support it without all this workaround. At least in the latest couple versions it's working as expected. Again we need to check the usage rates, maybe these "extra" languages are only used by recent apis (pretty unlikely but worth a check).
  3. Instead of introducing a base activity, I'd add it to each activity. I am almost 50-50 between this and the 4th option, but I really don't like base classes especially for something like activities. I feel like it's more likely for that base activity to become a mess than for us to forget about including the attachBaseContext in each activity. I believe this could easily be solved with the linter and there might even be an option to change the new activity template to have it by default. For me it's not too much different than passing the site object everywhere.
  4. Keep this solution
  5. Side note for 3 and 4, I am really worried about killing the process. Is there anything else we can do? Even if it's just showing a dialog to the user that they need to restart the app seems better to me.

I am going to defer this issue to @maxme since he probably knows about this issue more than any of us.

/cc @mzorz

…PI levels

* Remove need for forcing an app restart using System.exit(0) and force WPMainActivity to recreate itself.
* Contrary to the docs, API 17 & 19 require locale updates to happen using both the old way.

Tickets: #7267, #7265
… method across all activities.

* This method is required for app-level localization to work in later versions of Android.

Tickets: #7267, #7265
@AmandaRiu
Copy link
Contributor Author

AmandaRiu commented Mar 8, 2018

While reviewing the code changes in this PR with @oguzkocer, it was decided that using System.exit(0) may be dangerous since it would kill the app instantly and this may cause issues with uploads, notifications, and FluxC jobs. That ended up being a really good call because once I figured out how to remove that and get everything working, the new fix ended up being cleaner and less "hackish" feeling. Much better. Peer reviews FTW! 🎉

The How

  • Even though the new context.createConfigurationContext(...) method was added in API 17, it doesn't fully replace the old way of changing things until Lollipop. If you update one and not the other you either don't see the language update at all, or RTL definitions are not applied. This method now covers those scenarios:
private static Context updateResources(Context context, String language) {
    Locale locale = new Locale(language);
    Locale.setDefault(locale);

    Resources res = context.getResources();
    Configuration config = new Configuration(res.getConfiguration());

    // NOTE: Earlier versions of Android require both of these to be set, otherwise
    // RTL may not be implemented properly.
    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN_MR1) {
        config.setLocale(locale);
        context = context.createConfigurationContext(config);
    }
    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N_MR1) {
        config.locale = locale;
        res.updateConfiguration(config, res.getDisplayMetrics());
    }
    return context;
}
  • WordPressUtils -> LanguageUtils.getCurrentDeviceLanguage(...) is fetching the locale using this logic:
public static Locale getCurrentDeviceLanguage(Context context) {
    // better use getConfiguration as it has the latest locale configuration change.
    // Otherwise Locale.getDefault().getLanguage() gets
    // the config upon application launch.
    Locale deviceLocale = context != null ? context.getResources().getConfiguration().locale : Locale.getDefault();
    return deviceLocale;
}

For Android 25 and above, this no longer works since the locale is updated differently. Looking at the code comment, the developer was worried that Locale.getDefault() would return an outdated locale, but this was only because the default locale was not being updated when the app language was changed (Locale.setDefault(locale)). This has been added so Locale.getDefault() will always return the current locale settings no matter the version of Android. So this method should be changed to:

public static Locale getCurrentDeviceLanguage(Context context) {
    return Locale.getDefault();
}

Technically we no longer need the context argument, but since this is a library used by many, maybe leave it and mark it as deprecated. Then have it point to a new method that does not have the context argument?

I haven't added this code change yet because I'm really new to this repo and would like to get some direction before changing it.

Until it is changed, however, the tag options drop down on the Notification view (and likely other places) will not reflect the app language change until the app is restarted for devices running API 25 and above.

Other Changes Included

@oguzkocer requested that I remove BaseActivity and BaseListActivity because base classes have the potential for abuse and often turn into a nightmare. I understand and respect this position, BUT I must express that I have been brainwashed by the DRY principle so thoroughly that updating 54 classes with these exact 4 lines of code was just soul shattering 💔

@Override
    protected void attachBaseContext(Context newBase) {
        super.attachBaseContext(LocaleManager.setLocale(newBase));
    }

Ok, maybe I am being a tad bit dramatic. In any event, I committed all those changes separately from the main fix just in case someone decides they liked it better the other way 😉

Now for some noteworthy funkiness

code word: Language Toggling
On Android API 17 & 19, swapping between the same two languages more than once will still work for 95% of the UI, but the filter bar in the Notifications tab will stop updating. I believe this is because the buttons in that bar have static labels with assigned string resources, but I'm not 100% sure why it would work when switching from language A to language B, but not update when switching back to language A. The only thing I can think of is maybe the OS doesn't see it as a language change because it has it cached somewhere. If you go and switch to language C, it will update again just fine. I thought about how I could fix this by maybe creating an event to pass over the EventBus that would force those labels to be redrawn using the current locale, but that seemed like a lot of extra complexity to solve an issue that only happens on really old versions of android and only if the users follows that exact pattern.

Emulator Test Results by API

  • 16 : 💯 - RTL does not work - functionality not added until 17
  • 17, 19 : 👌 - language toggling
  • 22, 23, 24 : 💯
  • 25, 26, 27 : 💯 - requires the recommended WordPressUtils fix
  • P : 👌 - RTL changes not applied until app restart. Not sure what changed.

cc: @maxme @oguzkocer

@hypest
Copy link
Contributor

hypest commented Mar 8, 2018

By @oguzkocer :

Instead of introducing a base activity, I'd add it to each activity... I believe this could easily be solved with the linter

Regardless on which route we'll take on this particular PR, I would like us to explore that ^ idea. I'd love having the linter help us do the right thing while coding and implementing conventions this way sounds awesome!

@daniloercoli
Copy link
Contributor

My preference here is to go with option 1. Drop the language option all together..

I suggest to check usage in Tracks (if this is possibile) and if we don't want to kill the feature because it's still used by some of our users, we can just show a dialog that explains to restart the app to apply the changes (if this makes sense). Or limit the feature and patches to latest Android versions. Also mark this feature deprecated and remove it in one year or so.

@oguzkocer
Copy link
Contributor

@AmandaRiu I think there was a slight miscommunication because what I was trying to say was I personally prefer not to add a base activity, however I am not completely against the idea. As we also discussed on Slack yesterday, I am actually OK with it if all we use it for will be the language issue. I am just worried that once that base activity is there we'll start adding more stuff which will complicate our lives in the long run.

@AmandaRiu
Copy link
Contributor Author

@oguzkocer Yeah I know and I half agree with you which is why I implemented it 😸 I figured I'd put it in and leave it up to @maxme

@maxme
Copy link
Contributor

maxme commented Mar 9, 2018

I agree that having a BaseActivity is not a good idea, and we actually had one a long time ago that was causing problems

I agree that having the same method repeated in all class descending Activity is not a good idea either and very prone to error (unless we had a lint rule, but that kills me a little to add this kind of rules).

I disagree on the removal of the language selector. It's the only way to switch the app to certain languages (because some languages are not supported by in the core settings of Android). We'll also have to drop the work of our translators (we have very active translators for languages that are used by very few users). This would send a bad signal to our users and translators, and this only for avoiding a minor issue.

I think we can live with #7265 (and we could comment on it and mark it with the won't fix label, add a toast in the app to explain that, due to some Android limitation, users have to restart the app, etc.). Android < 5.0 represents less than 9% of our users and will decrease even more over time (we might kill support for it soon).

@maxme
Copy link
Contributor

maxme commented Mar 9, 2018

BTW, I misread the whole convo

Drop the support for changing language for api levels that don't support it without all this workaround.

Yes that's an option too, for API levels < 21.

@oguzkocer oguzkocer self-assigned this Mar 9, 2018
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

@AmandaRiu With my limited knowledge on the subject, it pains me to say that these changes look good :) I've left a few minor comments and thoughts. Let me know what you think about them and we can merge this in.

I don't know if we are planning to add a lint check for these beautiful attachBaseContext methods, but we might want to open a new issue for that and ping Maxime and Alex in it to start a discussion.

// In case of a "ArithmeticException: divide by zero", force the use of "US" locale to format the string.
txtFollowCount.setText(String.format(Locale.US, getContext().getString(R.string.reader_label_follow_count),
blogInfo.numSubscribers));
txtFollowCount.setText(String.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like our try and catch are the same now. Maybe we don't need the try and catch anymore, but if we do, shouldn't they be different statements?

} catch (ArithmeticException exception) {
// See https://github.com/wordpress-mobile/WordPress-Android/issues/5568
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do need this try/catch I assume we still need to deal with this bug in which case I think we should keep the comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah good catch. The bug is still relevant, but now it's being properly handled by my getSafeLocale(...) method so I can remove this catch altogether.

* @param language The language to compare
* @return True if the languages are the same, else false
*/
public static boolean isDifferentLanguage(String language) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might just be me, but I think it'd be better if we had a isSameLanguage method instead of isDifferentLanguage to avoid the double negative like the one in AppSettingsFragment:

if (!LocaleManager.isDifferentLanguage(languageCode)) {
    return;
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Love it...I'll get it updated 💯

return;
}

if (Locale.getDefault().toString().equals(newLocale.toString())) {
// remove custom locale key when original device locale is selected
mSettings.edit().remove(LANGUAGE_PREF_KEY).apply();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this mSettings property anymore after these changes. Should we remove that as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Removed

public static Context setNewLocale(Context context, String language) {
Locale newLocale = WPPrefUtils.languageLocale(language);

if (Locale.getDefault().toString().equals(newLocale.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use the helper method isDifferentLanguage here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed :)

* @return True if the languages are the same, else false
*/
public static boolean isDifferentLanguage(String language) {
Locale newLocale = WPPrefUtils.languageLocale(language);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this shouldn't really be a problem (at least not right now), but I am wondering if it's worth to check the emptiness of language ourselves instead of letting WPPrefUtils.languageLocale() to return the default language to us. Alternatively, it might be even better to just mark the language as @NonNull so the activity/fragment that's calling this method has to deal with the issue.

To clarify, let's say the current locale is Turkish and we somehow passed an empty string to this method, it'll return true since Turkish is not the default locale. By handling this case in the method we can return false and that'd be good enough. But if we were to change the method to isSameLocale, returning false in that case is not what we want. So, I feel the @NonNull approach is the better one, assuming that it's even worth to do anything here since it will pretty much only prevent developer errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default locale is the current locale unless the user is in the process of manually changing the language in which case the language string would not be empty and if it were then we wouldn't want to make a change so we would want it to return the default locale. When a new language is requested a locale object is created from that language and then set as the default locale. When the app is closed and restarts, one of the first few things it does during application startup is to check for the presence of a language override, and if it exists, the default locale is set from it immediately.

Also the @NonNull would only check for a null object and not an empty string so it wouldn't gain us anything. 🤔

Oh, but I did move all the language-related methods out of WPPrefUtils since they were all just helper methods for localization and not really relevant to preferences. It's also nice to keep all those methods in one place so developers can find them in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default locale is the current locale unless the user is in the process of manually changing the language in which case the language string would not be empty

That sounds good to me, it was just a thought, because some check outside the control of isDifferentLanguage was happening in the WPPrefUtils.languageLocale which is always a worrisome thing for me. In this specific case, there is no harm and we don't have to do anything here. Thanks for indulging my question.

Also the @nonnull would only check for a null object and not an empty string so it wouldn't gain us anything.

It doesn't fully fix it, but it covers half the case. Having said that, I was not really sure about if it's good enough either. I think the move to the LocaleManager will help with my concern 👍

* selected language is properly saved and resources appropriately updated for the
* android version.
*/
public abstract class LocaleManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot to mention this during my review. I am curious about why this class is marked as abstract, is there a benefit that I am missing about it? All abstract tells me is that we might have a different implementation, something like WordPressLocaleManager but we want to be able to pass the LocaleManager around instead which I don't think is the case here. I'd love to learn if there are other benefits I didn't know about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It prevents people from instantiating an instance of this class since it's a utility class. I can remove it if you think it's misleading...I think it's one of those things where some developers do it and some do not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we create an instance, we won't be able to do anything with it, but that's a good point. Just for consistency, I think it might still be better not to make it abstract as other such classes don't follow that pattern.

P.S: I actually don't know which one I'd prefer if I was working solo in a project 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

…caleManager

* Move all language related helper methods from WPPrefUtils to LocaleManager
* Remove unnecessary try/catch
…eLanguageCode(context) and deprecate method signatures

Methods stopped working as expected as of API 25. Context argument is no longer needed, but may still be used by other libraries so deprecated those method signatures and added new updated method signatures.
@AmandaRiu
Copy link
Contributor Author

@oguzkocer Implemented suggestions. I also went in and made the suggested changes to WordPressUtils.languageUtils() as explained here:

WordPressUtils -> LanguageUtils.getCurrentDeviceLanguage(...) is fetching the locale using this logic:

public static Locale getCurrentDeviceLanguage(Context context) {
    // better use getConfiguration as it has the latest locale configuration change.
    // Otherwise Locale.getDefault().getLanguage() gets
    // the config upon application launch.
    Locale deviceLocale = context != null ? context.getResources().getConfiguration().locale : Locale.getDefault();
    return deviceLocale;
}

For Android 25 and above, this no longer works since the locale is updated differently. Looking at the code comment, the developer was worried that Locale.getDefault() would return an outdated locale, but this was only because the default locale was not being updated when the app language was changed (Locale.setDefault(locale)). This has been added so Locale.getDefault() will always return the current locale settings no matter the version of Android. So this method should be changed to:

public static Locale getCurrentDeviceLanguage(Context context) {
    return Locale.getDefault();
}

Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

Assuming I didn't miss something, I think we are almost good to go. I've left a couple super minor comments.

/**
* Generates display strings for given language codes. Used as entries in language preference.
*/
@android.support.annotation.Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth importing this even if just for consistency: import android.support.annotation.Nullable;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

}

@SuppressWarnings("WeakerAccess")
public static Locale getCurrentDeviceLanguage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can mark this as private and remove the suppression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I guess we don't know if this class is used at all outside of WPAndroid. Let's skip this change for now.

@@ -1,24 +1,43 @@
package org.wordpress.android.util;

import android.content.Context;
import android.support.annotation.Nullable;

import java.util.Locale;

/**
* Methods for dealing with i18n messages
*/
public class LanguageUtils {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to understand the deprecated methods in this class, but I feel like we should drop this all together if we are not going to use it. What do you think?

P.S: I don't think it has to happen in this PR, I'd actually prefer a separate one for this.

public static Context setNewLocale(Context context, String language) {
Locale newLocale = languageLocale(language);

if (Locale.getDefault().toString().equals(newLocale.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change was missed #7413 (comment)

…pdated login in method to just exit it the new language is the same as the current language.
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot for this PR @AmandaRiu :shipit:

@oguzkocer oguzkocer merged commit 3bad00d into develop Mar 9, 2018
@oguzkocer oguzkocer deleted the issue/language-default branch March 9, 2018 22:54
maxme added a commit that referenced this pull request Apr 9, 2018
3117e2d9a3 Bump version to 1.20.1
6d127814c4 Update Travis build tools in all subtrees to v27
6e697f276e Update Gradle wrapper in all subtrees to 4.4
22d3f56300 Fix a build problem
3467a1f7d7 Update Bintray plugin to 0.8.1, which supports Gradle 4.4
f8a61eed7c Update Android Gradle plugin to 3.1
dacea1bc1b Update WordPress Utils support lib modules to 27.1.0
dc6cc663c0 Merge branch 'develop' into feature/awesome-accessibility
5aefdeafeb using NumberFormat.getInstance() factory method as suggested in docs https://developer.android.com/reference/java/text/DecimalFormat.html
3111f9361d simplified statement
fc23f04a64 put DecimalFormat instance back, as we need to re-create it once the local/settings have changed
9418672d5e fixed small typo
b13bbd1991 made constant final
d3fda0f3eb added file size units and made them translatable
f650818eb9 fixed minor typo in comment
ddb0419e4f using DECIMAL_INSTANCE instead of new Decimal()
b86cc2478a Fix style warnings.
917befde81 Add method to format percentages.
4d63e29192 Merge branch 'develop' into issue/display_site_quota
3d9b3c80ea Display site settings quota.
f0eb0b8f18 Merge pull request #7413 from wordpress-mobile/issue/language-default
aac0ac3a60 Fix broken methods getCurrentDeviceLanguage(context), getCurrentDeviceLanguageCode(context) and deprecate method signatures
abfaa0ed59 Merge branch 'develop' into feature/santa-lint-issues
85774cc246 Replace strings in code for better support of RTL translations
88b9f6332c Only set up checkstyle for utils if it's not already defined
493063e7a2 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into feature/awesome-accesibility-merge-dev-fixes
4c6e99dc65 Fix visibility modifier in AccessibilityUtils
884e5f6626 Increase toast with action duration when accessibility mode is enabled

git-subtree-dir: libs/utils
git-subtree-split: 3117e2d9a39ff10ee01fb9b1177a37990220585e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants