Setup compose multiplatform and migrate first screen to it#2829
Setup compose multiplatform and migrate first screen to it#2829Luna712 wants to merge 21 commits into
Conversation
I explained most in code comments the best I could as well. Icons are automatically generated from https://fonts.google.com/icons which provides automatically generated code for Compose as well. In the future most icons will use this if a relevant one is found, otherwise we can use SVG icons. Compose multiplatform also supports android XML versions in a KMP context, but I actually found this to be very inconsistent and we shouldn't use that directly if we can avoid it. I am also going to go through a lot of other changes here and some reasoning as well: This adds reusable components that will be useful elsewhere as well: * `.cloudStreamRipple`: replacement for `selectableItemBackground` * `.tvFocusable`: handle manual focus for TV to ensure it is correctly following it and handles the border outline for it also, will probably need improved/changed more later though as we need different versions for it in some places * ProfilePicture: to reuse in all the different profile picture containers, accepts parameters where things will need to be different later * SettingsItem: Used for all settings category headers for now, will later also be used in all settings subpages for consistency and avoid code duplication. This may need some changes later though also This handles Monet/Material You differently. This is a dynamic color scheme only available on Android, therefore to define it it uses expect/actual declarations. Other platforms do have their own concept of dynamic themes though, so in the commonMain API it just uses the `DYNAMIC` terminology for better support for these later on and a more consistent cross platform API. This applies to both the theme and primary colors for it. Since things are not XML based (and when we do use XML based stuff like strings in composeResources, there is still no `@string/` usage), this also adds new `PreferenceKeys` and `PreferenceDefaults` objects with constants for preferences. `PreferenceKeys` will replace all the string keys values in the current `donottranslate-strings.xml` and `PreferenceDefaults` will just include the default values for the preferences. Both are there to ensure consistency (not different defaults in different places) and to avoid accidental typos or something. For now it keeps what is added there only to what we actually use in `composeApp` as more are needed there these will be populated more as well. For now, this also duplicates the default profile background images to `composeResources` so we can use them in `:composeApp` directly as well. This also avoids refactoring to much of the app intentionally like removing/moving the companion object from the old SettingsFragment to avoid this PR growing massively. This adds a KMP version for device layout detection though as I needed to get if its a TV in `:composeApp`. Eventually we will have to split `:app` into `:androidApp` which just includes the signing stuff and entry points (AndroidManifest.xml, MainActivity (but a much slimmer version) and CloudStreamApp and other entry points like services (things defined in AndroidManifest). AGP 9 prevents the ability to have both `com.android.application` and `com.android.kotlin.multiplatform.library` at the same time, which means android entry points/APK generation can't be in a KMP module. `:app` will slowly be drained into fully KMP APIs into `:composeApp`. Stuff in there should not be stuff extensions will typically need though, otherwise it should go in `:library` instead. Also, it is not immediately necessarily, but eventually we should also add the `composeResources` directory to weblate. This also sets up the locales python script to support that directory if and/or when we add it. Happy to make any changes necessary though.
fire-light42
left a comment
There was a problem hiding this comment.
Please try to add @Preview annotations to components and screens. To make compose accessible people need to see the layouts.
That was a good idea and is now done. Thank you! |
|
Thanks for your efforts, just a question Google material contains the vector images, is it not possible to use them instead of using pure vector designin in code? |
Google material extended that includes the actual icons is deprecated and no longer maintained, and increases the app size by about 20MB as it loads everything. We don't need a library with thousands of icons we will never use. |
|
Based on my experience migrating several apps to Compose multiplatform, here is how to save yourself time and headaches: |
|
2 and 3 are planned. Koin is still under consideration. If it will end up helpful it will be used. SQLDelight I have already started writing a lib and migration code for SharedPreferences for it. |
Thanks I didn't know that it increases the size that much but R8 will remove unnecessary items from code and resources |
There was a problem hiding this comment.
I think we can make the layout system a bit more compose friendly.
By having a mutable state, we do not need to restart the entire activity.
Instead we simply have to call the update function and every "by isLayoutState" will cause a recomposition.
private val layoutState = mutableStateOf(layoutId)
fun isLayoutState(layoutFlags : Layout) = derivedStateOf { layoutState.value.and(layoutFlags) }
fun update() {
layoutId = resolveLayout()
layoutState.value = layoutId
}val isTV by isLayoutState(TV)This is easier to read, and reason about.
I also have some nitpicks that I want to clear up early.
-
More things can be marked as
@Immutable, this includes "Layout", "SettingsProfileState" and "SettingsVersionState" -
No need to specify the structuralEqualityPolicy, it is the default
-
I would rather you use LocalContext.current and pass it as a variable than use globals
val version = buildVersionState(LocalContext.current)
val profile = buildProfileState(LocalContext.current)Where the functions use context.currentCommitHash() and DataStoreHelper.getDefaultAccount(context)
This is because we want to avoid all requireContext/requireActivity, use inflater.context for ComposeView and LocalContext.current for @Composable.
|
Now after we have released a new stable, I think this should be merged as soon as possible! |
|
I will work on what you mentioned later today and push in the next few days. I am a little confused about some of what you said though I will work on it and might ask here for some clarity in a bit if I can't figure out what you mean. |
|
I pushed some of the fixes now. Will do the rest later. Also just to note, I have tests written for compose (both UI and logic tests) but I will push that in a seperate PR. I also have detekt setup to ensure code standards while things are migrated that I will push in a seperate PR. It would ensure things like that bracket newline if-block thing you wanted in my other PRs and other standards. I am not sure what all you will want but I can push it later, and it can be reviewed. I mention that now because some of what I also just pushed was found from that. |
|
@fire-light42 I believe I did everything you were wanting. Hopefully correctly. If not I apologize and can adjust. |
LagradOst
left a comment
There was a problem hiding this comment.
Using this PR I started porting QN to compose. And I found out that
- Missing AmoledLight, Amoled is not the same as AmoledLight
- MaterialTheme typography? I think we use google_sans.xml for all fonts
|
Please also include comments directly on the colors, as it makes it a lot easier to convert. /** primaryBlackBackground */
var background by mutableStateOf(background)
/** primaryGrayBackground */
var surfaceVariant by mutableStateOf(surfaceVariant)
/** iconGrayBackground */
var surface by mutableStateOf(surface)
/** boxItemBackground */
var surfaceContainer by mutableStateOf(surfaceContainer)
/** textColor */
var onBackground by mutableStateOf(onBackground)
/** grayTextColor */
var onSurfaceVariant by mutableStateOf(onSurfaceVariant)
/** iconColor */
var icon by mutableStateOf(icon)
/** colorPrimary */
var primary by mutableStateOf(primary)
/** colorOngoing */
var ongoing by mutableStateOf(ongoing) |
|
Also, is there a good reason to use https://fonts.google.com/icons imagevectors directly? I found |
This was a personal preference, because it was easier for me to build and maintain it in pure Kotlin where possible, faster build by reducing the stuff in composeResources a bit, and while mostly negligible reduced the app size just a tiny bit. I did first use drawables before I realized fonts.google.com gave the Kotlin version also. Not 100% against changing if you really want though. |
I have a finished version that readds this but the more I think about it the more I don't really like it. By default MaterialTheme uses The way I have it written would make it easy to eventually add a setting to use system default font, fonts from composeResources, or a user custom selected font via local file path (though it only adds the initial API and doesn't set anything up there, was just to make it easier later on). Bur the more I was thinking of it, I don't think this was the right approach. I think what we should do is system default font, or a user selected font (eventually). Again, just my opinion. I was hoping for opinions on this before I push anything else with it. My main goals with this PR is to make it easily expandable, readable, and maintainable in the future so I just want to get this right the first time. At the very least I do thing we should in some way support using the system default font rather than the app font itself as it looks more universal across other aspects of the system that way also. |
The problem is that our "custom app font" is used everywhere in the app, and the migration will take longer than 1 day. Therefore we will have a lot of time in-between the full port, where half the app uses another font compared to the other. I could agree that FontFamily.Default might be good in the end, but while porting I need the font to match the rest of the app exactly. We can make it customizable later, but now we have to hardcode it as otherwise it will look very weird. |
|
I know I may just be being stubborn right now and I apologize and I am not 100% against what you said, I just don't think it is necessary, what I would rather is make the existing app also use the system font by default. This could be done by simply removing the custom app font, as android will use the system font by default as well. Again if you really want me to hard code it I will but the point of this is that composeApp won't need so many changes later on, only a few temp-only stuff, not much at all... so that as things are migrated it will be easy to use. If I do it manually that is a much larger change because the way I did it use MaterialTheme typography that controls the size as well, as I would have to override for each typography style the font only, upload the all the ttfs to composeResources, all for a temporary solution that would eventually be changed again, and that is kinda what I wanted to avoid. |
I think we need the built in font. I choose it because it fits the app. and changing it randomly like this to system is a no-go for me right now. And no, adding a font is really easy, check out https://github.com/LagradOst/QuickNovel/blob/master/app/src/main/java/com/lagradost/quicknovel/compose/theme.kt#L464-L496 |
|
I had a very similar version though not exact that works but I felt it was more boilerplate than I want. I guess I can just later push what I have and can get a review if you want it like that or not. |
I explained most in code comments the best I could as well. Icons are automatically generated from https://fonts.google.com/icons which provides automatically generated code for Compose as well. In the future most icons will use this if a relevant one is found, otherwise we can use SVG icons. Compose multiplatform also supports android XML versions in a KMP context, but I actually found this to be very inconsistent and we shouldn't use that directly if we can avoid it.
I am also going to go through a lot of other changes here and some reasoning as well:
This adds reusable components that will be useful elsewhere as well:
.cloudStreamRipple: replacement forselectableItemBackground.tvFocusable: handle manual focus for TV to ensure it is correctly following it and handles the border outline for it also, will probably need improved/changed more later though as we need different versions for it in some placesThis handles Monet/Material You differently. This is a dynamic color scheme only available on Android, therefore to define it it uses expect/actual declarations. Other platforms do have their own concept of dynamic themes though, so in the commonMain API it just uses the
DYNAMICterminology for better support for these later on and a more consistent cross platform API. This applies to both the theme and primary colors for it.Since things are not XML based (and when we do use XML based stuff like strings in composeResources, there is still no
@string/usage), this also adds newPreferenceKeysandPreferenceDefaultsobjects with constants for preferences.PreferenceKeyswill replace all the string keys values in the currentdonottranslate-strings.xmlandPreferenceDefaultswill just include the default values for the preferences. Both are there to ensure consistency (not different defaults in different places) and to avoid accidental typos or something. For now it keeps what is added there only to what we actually use incomposeAppas more are needed there these will be populated more as well.For now, this also duplicates the default profile background images to
composeResourcesso we can use them in:composeAppdirectly as well.This also avoids refactoring to much of the app intentionally like removing/moving the companion object from the old SettingsFragment to avoid this PR growing massively. This adds a KMP version for device layout detection though as I needed to get if its a TV in
:composeApp.Eventually we will have to split
:appinto:androidAppwhich just includes the signing stuff and entry points (AndroidManifest.xml, MainActivity (but a much slimmer version) and CloudStreamApp and other entry points like services (things defined in AndroidManifest).AGP 9 prevents the ability to have both
com.android.applicationandcom.android.kotlin.multiplatform.libraryat the same time, which means android entry points/APK generation can't be in a KMP module.:appwill slowly be drained into fully KMP APIs into:composeApp. Stuff in there should not be stuff extensions will typically need though, otherwise it should go in:libraryinstead.Also, it is not immediately necessarily, but eventually we should also add the
composeResourcesdirectory to weblate. This also sets up the locales python script to support that directory if and/or when we add it.Happy to make any changes necessary though.