fix blink favorite icon #945
Conversation
core/data/src/commonMain/kotlin/io/github/droidkaigi/confsched/data/user/UserDataStore.kt
Outdated
Show resolved
Hide resolved
.../commonMain/kotlin/io/github/droidkaigi/confsched/data/sessions/DefaultSessionsRepository.kt
Outdated
Show resolved
Hide resolved
|
Thank you for your insights on the code. I think this PR introduces several changes. What do you think is the cause? If we use memory cache, is rememberRetained fine? It is a little difficult to manage if we add a source of information. |
|
Thank you for your comment.
I did some further investigation. The probable causes are:
The problem seems to be that the value during the transition to another screen cannot be reflected, so it doesn’t seem that rememberRetained can solve the issue.
I agree with that. How about changing getFavoriteSessionStream to return a StateFlow instead of exposing the memory cache, and using collectAsState so that the latest value is always reflected when returning to the original screen? I believe there may be some issues with this approach as well, so once I push the code, I’d appreciate it if you could review it. |
This reverts commit d55402c.
To return the same instance of StateFlow
|
Updated the source code and the overview 😄 |
|
Detekt check failed. Please run |
|
|
||
| public class UserDataStore(private val dataStore: DataStore<Preferences>) { | ||
|
|
||
| private val mutableIdToken = MutableStateFlow<String?>(null) | ||
| public val idToken: StateFlow<String?> = mutableIdToken | ||
|
|
||
| public fun getFavoriteSessionStream(): Flow<PersistentSet<TimetableItemId>> { | ||
| return dataStore.data | ||
| private val singletonCoroutineScope: CoroutineScope = CoroutineScope(Job()) |
There was a problem hiding this comment.
This could be an unmanaged dispatcher in tests, which could result in flaky tests. I think we need to pass a dispatcher for this.
There was a problem hiding this comment.
Thank you for your comment.
I modified it so that testDispatcher can be used during testing.
Does this fix seem okay to you?
move coroutineScope field to constructor parameter
Add TestUserDataStoreModule
![FavoritesScreenTest[FavoritesScreen - when server is operational - setup many favorite sessions - click first session bookmark - it should display favorite session without first session]_compare.png](https://github.com/DroidKaigi/conference-app-2024/blob/companion_fix-blink-favorite-icon/./screenshot-diff-reports/feature/favorites/build/outputs/roborazzi/FavoritesScreenTest%5BFavoritesScreen%20-%20when%20server%20is%20operational%20-%20setup%20many%20favorite%20sessions%20-%20click%20first%20session%20bookmark%20-%20it%20should%20display%20favorite%20session%20without%20first%20session%5D_compare.png){kind=link}
![FavoritesScreenTest[FavoritesScreen - when device is tablet - it should show timetable items]_compare.png](https://github.com/DroidKaigi/conference-app-2024/blob/companion_fix-blink-favorite-icon/./screenshot-diff-reports/feature/favorites/build/outputs/roborazzi/FavoritesScreenTest%5BFavoritesScreen%20-%20when%20device%20is%20tablet%20-%20it%20should%20show%20timetable%20items%5D_compare.png){kind=link}
![FavoritesScreenTest[FavoritesScreen - when server is operational - setup many favorite sessions - scroll favorites - it should first session is not displayed]_compare.png](https://github.com/DroidKaigi/conference-app-2024/blob/companion_fix-blink-favorite-icon/./screenshot-diff-reports/feature/favorites/build/outputs/roborazzi/FavoritesScreenTest%5BFavoritesScreen%20-%20when%20server%20is%20operational%20-%20setup%20many%20favorite%20sessions%20-%20scroll%20favorites%20-%20it%20should%20first%20session%20is%20not%20displayed%5D_compare.png){kind=link}
|
I'm not sure, but the tests don't show all the favorite sessions. 👀 |
|
Thank you. I also share your concern about having a CoroutineScope in DataStore, so I will try exploring alternative approaches, though it may be quite challenging. If I can’t resolve it, I may give up and close this pull request, so would it be possible to assign someone else? |
github-actions
bot
temporarily deployed
to
deploygate-distribution
|
I apologize. I tried various things, but I couldn't resolve the issue successfully. I will continue to investigate, but I would like to unassign myself and close this pull request for now. |
Issue
Overview (Required)
The probable causes are:
The value of collectAsRetainedState is not collected while navigating to another screen.
When returning to the original screen, the value is collected
but the update of the value likely occurs based on the execution timing of LaunchedEffect, so until the value is updated, the displayed value is not the latest.
Movie (Optional)
before_Screen_recording_20240904_002135.mp4
after_Screen_recording_20240906_002004.mp4