-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(android): add support for Android's API 33 Per-app languages #14120
Conversation
@hansemannn @m1ga a fresh clean PR for the Per-app languages. I kept searching for the test cases so I add to it, couldn't seem to find it tho. |
Nice 👍 This looks a lot better now. Will start testing your PRs tomorrow but from a quick first look it looked fine |
Will test using this, tho I was thinking about the automated unit testing. Update: The test app is working just fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some minor string changes you can apply.
I've used my test code from the comment above and works correctly:
- starting the app: empty array as no per-app language is set
- changing it in the app and then going to "app preferences - per-app language" shows the correct value
- changing the value in "app preferences - per-app language" and opening the app shows the correct value in the app
android/modules/locale/src/java/ti/modules/titanium/locale/LocaleModule.java
Outdated
Show resolved
Hide resolved
android/modules/locale/src/java/ti/modules/titanium/locale/LocaleModule.java
Outdated
Show resolved
Hide resolved
…aleModule.java Co-authored-by: Michael Gangolf <m1ga@users.noreply.github.com>
Co-authored-by: Michael Gangolf <m1ga@users.noreply.github.com>
…aleModule.java Co-authored-by: Michael Gangolf <m1ga@users.noreply.github.com>
android/modules/locale/src/java/ti/modules/titanium/locale/LocaleModule.java
Outdated
Show resolved
Hide resolved
if no languages defined, an empty array will be returned. You can set the value of it like this `en-US,en-GB,ar-SA`. | ||
This property requires Android API level 33 and above. | ||
See the [Android's Per-app languages](https://developer.android.com/guide/topics/resources/app-languages). | ||
type: Array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the array typings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please explain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the newer comments to split the API. But in general, you just have an array as the return value, but really it's Array<LocaleDetails>
to document all properties. This should not just refer to the Android docs, as there may be newer properties in the future that are not exposed in Titanium, yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some open issues to look at
fix(android): changed androidx app compact lib version to 1.6.0
…r/titanium-sdk into applicationLocales
} | ||
|
||
@Kroll.getProperty | ||
public KrollDict[] getApplicationLocales() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very uncommon to have different types for the getter and setter. And there is Ti.Locale.setLanguage
already. So I would propose:
- Use the new Android 33+ API in
Ti.Locale.setLanguage(language)
- Expose the getter as a new Titanium API (if there isn't one already to get language details - if so, update that one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this PR is with no use so I'll close it
LocaleListCompat localeListCompat = AppCompatDelegate.getApplicationLocales(); | ||
int size = localeListCompat.size(); | ||
KrollDict[] locales = new KrollDict[size]; | ||
for (int i = 0; i < size; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop looks a bit "oldskool". Things to change:
- Use a more modern loop technique, e.g. this one and append it a to an array instead of maintaining an
index
- Do not add all the logic in the null check. Instead, check for
locale == null
andcontinue
early
if no languages defined, an empty array will be returned. You can set the value of it like this `en-US,en-GB,ar-SA`. | ||
This property requires Android API level 33 and above. | ||
See the [Android's Per-app languages](https://developer.android.com/guide/topics/resources/app-languages). | ||
type: Array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the newer comments to split the API. But in general, you just have an array as the return value, but really it's Array<LocaleDetails>
to document all properties. This should not just refer to the Android docs, as there may be newer properties in the future that are not exposed in Titanium, yet.
This PR adds support for Android's Per-app languages presented in Android API 33/Android 12.
See Per-app languages preferences