Complete the debug screen migration to Compose #12803
Conversation
HatakeKakashri
requested review from
Isira-Seneviratne,
Stypox and
theimpulson
HatakeKakashri
force-pushed
the
debug_screen_migration
branch
from
03d392b to
fce5ec6
Compare
HatakeKakashri
force-pushed
the
debug_screen_migration
branch
from
fce5ec6 to
ddbaa94
Compare
github-actions
bot
added
size/giant
|
Updated the PR with nav3 as requested here cc: @theimpulson @TobiGr |
theimpulson
left a comment
theimpulson
left a comment
There was a problem hiding this comment.
Please also add SPDX copyright header to all files which are being modified/added as done in recent merged PRs for Kotlin migration.
| import androidx.navigation3.runtime.NavKey | ||
| import kotlinx.serialization.Serializable | ||
|
|
||
| sealed interface SettingsKey : NavKey |
There was a problem hiding this comment.
Please use a common sealed class such as Screen and put a Settings class inside it. This one should be a Sealed class as well but as ExtraScreen or something as all of this screens are extra used by Settings only.
There was a problem hiding this comment.
Is this what you meant. Don't think there is much advantage to keeping all screens keys in one place. I'm fine either way.
There was a problem hiding this comment.
Please use Sealed class so that everyone who adds a new Screen is forced to keep it inside there and not able to put logic at any package. Then you should keep all screens which are child to Settings in its own package so that it can handle its own children and they don't take space in global screens.
| @StringRes title: Int, | ||
| @StringRes summary: Int?, | ||
| enabled: Boolean |
There was a problem hiding this comment.
Please pass actual string resources here and set a default boolean as well. Additionally, please add a preview and doc-comment on what this composable is used for.
There was a problem hiding this comment.
I'm not 100% aligned on passing strings instead of @StringRes. Did you have any specific reasoning for this change?
My preference for @StringRes is:
- Compile-time safety with resource IDs
- Automatic localization handling
- Android standard patterns
There was a problem hiding this comment.
I want to write all compose logic as multi-platform so that when everything is ready NewPipe can be used on OS other than Android. Second thing is testing. We should be able to test composable without being dependent upon resources.
app/src/main/java/org/schabi/newpipe/ui/components/common/ScaffoldWithToolbar.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/ui/components/common/ScaffoldWithToolbar.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/settings/viewmodel/SettingsViewModel.kt
Outdated
Show resolved
Hide resolved
| TextPreference( | ||
| title = R.string.settings_category_player_title, | ||
| icon = R.drawable.ic_play_arrow, | ||
| onClick = { backStack.add(PlayerSettings) } |
There was a problem hiding this comment.
Please consider making the onClick logic a bit more general and avoid interacting with backstack directly. An example would be that the screen forwards this to the class with NavDisplay so all navigation logic is in one place making it easy for refactor.
HatakeKakashri
force-pushed
the
debug_screen_migration
branch
from
ddbaa94 to
dca9b30
Compare
- Completed the UI and logic for pending items of Debug screen using Jetpack Compose. - Implemented a new navigation system for settings using the navigation3 lib as it is now stable. - Reuses the `ScaffoldWithToolbar` composable and removed the previous `Toolbar` composable to avoid redundancy in code. - Refactored the `SettingsViewModel` to use a `BooleanPreference` helper class to reuse and reducing boilerplate for state management. - Created a shared `TextBase` composable to remove duplicated UI logic between `SwitchPreference` and `TextPreference`. - Move the build-variant-dependent logic for LeakCanary and reused it in Compose and Fragment, this logic is used for ensuring the leak canary fields are only enabled in debug variants. - Fixed a layout bug in `SwitchPreference` where long summary text could misalign the switch component and also adjusted the paddings for consistency.
HatakeKakashri
force-pushed
the
debug_screen_migration
branch
from
dca9b30 to
017f991
Compare
3 participants
What is it?
Description of the changes in your PR
SettingsViewModelto use aBooleanPreferencehelper class to reuse and reducing boilerplate for state management.TextBasecomposable to remove duplicated UI logic betweenSwitchPreferenceandTextPreference.SwitchPreferencewhere long summary text could misalign the switch component and also adjusted the paddings for consistency.Before/After Screenshots/Screen Record (The toolbar colors are to be handled in a seperate PR #12654)
Fixes the following issue(s)
Relies on the following changes
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Due diligence