Migrated GenreListFragment to Compose #163
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
timusus
force-pushed
the
tech/compose-genre-migration
branch
from
cd1128e to
df82ce2
Compare
Note: This is done on the back of some really great work by rivaldi8
timusus
force-pushed
the
tech/compose-genre-migration
branch
from
df82ce2 to
2b7b621
Compare
| genreRepository.getGenres(GenreQuery.All()), | ||
| mediaImportObserver.songImportState, | ||
| mediaImportObserver.playlistImportState | ||
| ) { genres, songImportState, playlistImportState -> |
There was a problem hiding this comment.
We are combining here MediaImportObserver.playlistImportState but it isn't used. Does it make any difference to have it in the combine or could it be removed?
There was a problem hiding this comment.
I think the logic below is incorrect, and we should show loading if the song import state or playlist import state is 'ImportProgress'
There was a problem hiding this comment.
The import of songs affects because we don't want to show the list until all the genres (songs) have been loaded. But why should we care about the loading status of playlists while loading genres? The list won't change, right?
| contentPadding = PaddingValues(vertical = 16.dp, horizontal = 8.dp), | ||
| state = state | ||
| ) { | ||
| items(genres + genres + genres + genres) { genre -> |
There was a problem hiding this comment.
These multiple genres look like a scrollbar test that you forgot to revert.
| modifier = Modifier.fillMaxSize().padding(vertical = 8.dp), | ||
| state = state, | ||
| getPopupText = { index -> | ||
| (genres + genres + genres + genres)[index].name.firstOrNull()?.toString() |
There was a problem hiding this comment.
These multiple genres look like a scrollbar test that you forgot to revert.
| LaunchedEffect(state.isScrollInProgress, isDragging) { | ||
| if (!state.isScrollInProgress && !isDragging) { | ||
| delay(1500) | ||
| if (!state.isScrollInProgress && !isDragging) { |
There was a problem hiding this comment.
Is this second check redundant or is it needed because the state might've changed while on the delay?
There was a problem hiding this comment.
Yes it might have changed, so we need to check the condition again
| style = MaterialTheme.typography.bodyMedium, | ||
| color = MaterialTheme.colorScheme.onBackground | ||
| ) | ||
| // Todo: Manually replacing "{count}" is not ideal. But, the Phrase library doesn't render correctly in Compose. |
There was a problem hiding this comment.
Wouldn't the solution be to change the {count} in strings_library.xml by %1$d? We can't do so now because it'd break other uses of songsPlural but we could do so in the future, right?
There was a problem hiding this comment.
Yeah, that's right. I'm reluctant to change it because we'd have to revert the usage of the Phrase library which is all over the app at the moment
Note: This is done on the back of some really great work by @rivaldi8
Supersedes #159