wip: Reimplement genres list with Compose #152
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Well, it's been a long time since I proposed to start an incremental migration to compose, but finally I've got something working :)
This is not a final pull request. I'm opening it because I thought this would make it easier to discuss some questions.
I've started by migrating 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.Status of the fragment migration
I've replaced the genres list implementation through
RecyclerView
with the Compose equivalent. Also, to make the interaction more natural, I've replaced theGenreListPresenter
withGenreListViewModel
.On the UI part it remains to migrate the two loading indicators and the fast scroll component. I don't plan to migrate anything else for the first pull request.
I haven't written any unit tests yet, but I'd like to do so if it doesn't take me too much time.
Apart from that, I'll also clean up the code before opening the pull request.
Questions to discuss
Fast scroll component. The current implementation is quite tied to the
RecyclerView
. I don't think it can be easily adapted for the new Compose implementation. I've looked around for a library or something equivalent for Compose but haven't found anything. Are you aware of some library? If there's nothing, I'll have to implement it myself. Any guidance is welcome :)Side effects from the actions of the overflow menu. There are two cases:
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. Do you think this is alright or should be implemented in another way?Is the
resetPosition
fromGenreListFragment.setGenres
still needed or is it just aRecyclerView
thing?Well, that should be it for the time being. Now that I getting a better knowledge of the application's code and all the new stuff from Android and Kotlin, I guess I'll be developing at a faster pace :) Let me know if you see anything else that should be discussed.