Skip to content
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

wip: Reimplement genres list with Compose #152

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rivaldi8
Copy link
Contributor

@rivaldi8 rivaldi8 commented Feb 13, 2024

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 your feature/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 the GenreListPresenter with GenreListViewModel.

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:

    • 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. Do you think this is alright or should be implemented in another way?

  • Is the resetPosition from GenreListFragment.setGenres still needed or is it just a RecyclerView 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant