-
Notifications
You must be signed in to change notification settings - Fork 16
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
Migrate genres list to Compose #159
base: main
Are you sure you want to change the base?
Conversation
Following these instructions: https://developer.android.com/jetpack/compose/setup#kotlin
I haven't managed to mock GenreListViewModel.
The full error was: Compose inspection unavailable. Could not determine the version of the androidx.compose.ui:ui artifact. Try a different version of compose
They are useful. From Kotlin coding conventions (*): Using trailing commas has several benefits: - It makes version-control diffs cleaner – as all the focus is on the changed value. - It makes it easy to add and reorder elements – there is no need to add or delete the comma if you manipulate elements. - It simplifies code generation, for example, for object initializers. The last element can also have a comma. Trailing commas are entirely optional – your code will still work without them. The Kotlin style guide encourages the use of trailing commas at the declaration site and leaves it at your discretion for the call site. (*) https://kotlinlang .org/docs/coding-conventions.html#trailing-commas
5552542
to
0b4017a
Compare
Sorry, I forgot of some stuff, like the linter, before opening the pull request. One of the complains from the linter was about trailing comas. As they are a good thing (as justified in the commit message), I've disabled the rule, if you are ok with it. Otherwise, I can revert it and remove the comas. While learning about KtLint, I found out about Ktlint plugin for Android Studio, which shows the issues directly in the editor. I can mention it in |
The unit tests have failed:
Sometimes it also happens when run locally on my machine. It seems jitpack.io sometimes takes a lot of time to respond, causing a time out. Not sure if anything can be done about it. |
0b4017a
to
05c304b
Compare
This is such a well put together PR, thank you @rivaldi8 I'll have a look and see if we can get it merged in |
android/app/src/main/java/com/simplecityapps/shuttle/ui/common/view/FastScrollBar.kt
Outdated
Show resolved
Hide resolved
Check to ensure we have items in the list before calling the `indicatorTextProvider`.
This is the first step in the migration of the application to Compose. I've started with the fragment used to show the list of genres in the Genres tab, as it looked a simple enough piece of the application to start with.
I wanted to make as few changes as possible to keep the pull request small and reduce the possibility of introducing regressions. The idea is to make the migration iteratively in increments as small as possible.
The approach I've followed has been to migrate the UI elements from the XML layout to compose and the minimum code to support it. This has meant reimplementing the UI in
GenreListFragment
with Compose and replacing its presenter with a view model. I've maintained the current repository and DAO. Although I've taken code from yourfeature/compose-emby
branch, I've left out most of it to avoid pulling too much code.On the UI part it remains to migrate the two loading indicators. I guess I'll migrate them after migrating some more tabs.
I've tried to write some unit tests, but I haven't managed to make them work. I found examples for View-only or Compose-only applications, but very few for hybrid View/Compose like this one. The only test runs but doesn't do any useful check, I couldn't manage to mock
GenreListViewModel
. As this was quite hard and took me a lot of time, I've left it so I can give it another try after I recover my Android skills again.Side effects from the actions of the overflow menu
I had some doubts while implementing them. There are two cases:
- Actions like Add to queue which show success/error message to the user.
- Actions like Edit tags which open a dialog/modal.
For the moment I've implemented these through callbacks on each method of the view model. From the examples I've found, it seems the usual practice is to use a Flow and then react to it accordingly. So, for the messages case, I'd have, for example, an
addedToQueue
Flow which would emit the name of the genre. Something similar could be done with the rest of cases. Maybe it's because I'm not used to this yet, but it looks a bit weird to me.As I wasn't sure of the proper way to implement this, I've left it with the callbacks implementation. Please, let me know if you think I should implement them in another way.