-
Notifications
You must be signed in to change notification settings - Fork 1
Language Settings #408
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
Language Settings #408
Conversation
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.
Pull Request Overview
This PR implements language selection functionality for the Bitkit Android app, allowing users to change the app language both from within the app settings and through Android system settings. The implementation provides a comprehensive language management system with support for 12 languages plus system default.
- Adds a new Language Settings screen accessible from General Settings
- Implements
AppLocaleManagerfor handling locale changes across different Android versions - Creates proper MVVM architecture with
LanguageViewModelfor language state management
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/src/main/java/to/bitkit/models/Language.kt |
Defines Language enum with supported languages and utility functions for locale conversion |
app/src/main/java/to/bitkit/ui/utils/AppLocaleManager.kt |
Handles locale management with Android version compatibility for API 33+ and legacy versions |
app/src/main/java/to/bitkit/viewmodels/LanguageViewModel.kt |
Manages language selection state and interactions following MVVM pattern |
app/src/main/java/to/bitkit/ui/settings/LanguageSettingsScreen.kt |
Composable screen for language selection with list of available languages |
app/src/main/java/to/bitkit/ui/settings/general/GeneralSettingsScreen.kt |
Adds language settings button to general settings with current language display |
app/src/main/java/to/bitkit/ui/ContentView.kt |
Adds navigation route and handler for language settings screen |
app/src/main/java/to/bitkit/ui/shared/util/Modifiers.kt |
Adds screen modifier utility for consistent screen layout |
app/build.gradle.kts |
Enables locale config generation for proper Android localization support |
app/src/main/AndroidManifest.xml |
Adds AppCompat locale service for backward compatibility |
app/src/main/java/to/bitkit/ui/settings/general/GeneralSettingsScreen.kt
Show resolved
Hide resolved
ovitrif
left a comment
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.
LGTM, works great in all checks:
- via settings
- via os app info
- manually when
system defaultis on and returning to the app
Kudos also for:
nice addition of `Modifier.screen()
nice and clean code
added a few remarks, non-blocking and non-mandatory that could help make the code even cleaner 🔮
| } | ||
| } | ||
|
|
||
| @Preview |
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.
nit: best to always use @Preview(showSystemUi = true) for screen previews, at times it could reveal some hidden issues with insets paddings. It's not the case now though, fortunately :)
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.
I forgot in this one. Will add to AS quick actions
| fun getCurrentLanguage(): Language { | ||
| val locale = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| val localeList = context.getSystemService(LocaleManager::class.java)?.applicationLocales | ||
| localeList?.get(0).takeIf { localeList != null && !localeList.isEmpty } |
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.
nit: works but this doesn't make a lot of sense :)
localeList?.get(0).takeIf { localeList != null && !localeList.isEmpty }
is equal to localeList?.get(0).takeIf { it.isNotEmpty() }
we already use safe call AFAIU
| fun getSupportedLanguages(): List<Language> { | ||
| return Language.entries.sortedWith( | ||
| compareBy<Language> { !it.isSystemDefault }.thenBy { it.displayName } | ||
| ) | ||
| } |
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.
nit: doesn't need to be instance method on appLocaleManager, it's not using any state of the class
| fun NavController.navigateToLanguageSettings() = navigate( | ||
| route = Routes.LanguageSettings, | ||
| ) | ||
|
|
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.
nit: wish I'd removed most of the navigateTo… methods, it's not really adding value, plus the entire nav setup might change entirely when we can adopt nav3
Close #406 (comment) , #406 (comment)
FIGMA
Description
Roadmap item
This PR implements the language selection from inside the app and from android system settings
Preview
Screen_recording_20250924_081210.webm
QA Notes