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

Issue/11724 app settings crash #11725

Merged
merged 2 commits into from
Apr 22, 2020
Merged

Conversation

renanferrari
Copy link
Member

@renanferrari renanferrari commented Apr 22, 2020

Fixes #11724

The problem was related to the string array app_theme_entries. This array is used to present to the user the options they have for the app theme preference. For Android 8 and below, it has only two options: light and dark. For Android 9 and above, it has a third option: default.

This string array wasn't supposed to be translatable, but it wasn't marked as so at first, causing it to be erroneously replicated to the strings.xml file of the following locales: ar, de, en-gb, es, es-MX, es-VE, fr, he, id, it, ja, nl, pl, ro, ru, sq, sv, tr, zh-CN, zh-HK, zh-TW.

Due to how Android resolves alternative resource files, locale qualifiers almost always takes precedence, meaning a string array defined for a specific locale always overrides a string array defined for a specific OS version.

Because of that, if the user had one of those languages selected and was using a device with Android 9 or above, that app preference would only load two options instead of three, and since the default option in those cases is the third one, which was missing, an ArrayIndexOutOfBoundsException was thrown, crashing the app.

This PR marks the string array app_theme_entries as not translatable and removes all the instances where it was erroneously replicated to the strings.xml file of the affected locales.

To test

  1. Make sure you're using a device with Android 9 and above and that the selected language is one of those listed above.
  2. On the site screen, tap the profile icon in the header bar
  3. Tap App Settings
  4. The app shouldn't crash

Notes

  • If those translations were added by some automated process, we may need to make sure they won't be added by mistake again.
  • The user could also experience the crash even if they had not selected any of the languages listed above. That's because when a string is missing for a specific language, Android can use a similar language as a fallback.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@renanferrari
Copy link
Member Author

@oguzkocer Pinging you here, as requested.

@peril-wordpress-mobile
Copy link

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

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 good, thanks for the fix @renanferrari! :shipit:

@loremattei I know we are not supposed to change the language specific string files normally, but since we are marking the original string as translatable=false and will not be pulling the strings from GlotPress for this hotfix, I think this current changeset is the right thing to do and no further action is necessary for the other releases. I am going to go ahead with the hotfix and the beta/alpha builds for the code freeze with this assumption, but could you look into this tomorrow and let me know if any further action is necessary? Thanks!

@oguzkocer oguzkocer merged commit 97ba825 into release/14.6.1 Apr 22, 2020
@oguzkocer oguzkocer deleted the issue/11724-app-settings-crash branch April 22, 2020 19:27
This was referenced Apr 22, 2020
@loremattei
Copy link
Contributor

@oguzkocer looks good to me 👍
Thanks for handling this!

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.

3 participants