Feature/fga/mark room as favorite #2397
Conversation
ganfra
added
the
Record-Screenshots
github-actions
bot
removed
the
Record-Screenshots
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
| hasNewContent = event.roomListRoomSummary.hasNewContent | ||
| ) | ||
| contextMenuState.value = initialState | ||
| client.getRoom(event.roomListRoomSummary.roomId)?.use { room -> |
There was a problem hiding this comment.
I think instead of returning a job to be manually cancelled, you could create a Flow<RoomListState.ContextMenu> with snapshowFlow here and use .takeWhile { ... }. I'm not sure if it's more convoluted, but it looks a bit less error-prone to me.
There was a problem hiding this comment.
Let me know if you want me to take a look at it while you're (in theory 😅 ) away.
There was a problem hiding this comment.
I've pushed the changes, let me know!
There was a problem hiding this comment.
It feels a bit weird the takeWhile is after the onEach, but it works well enough and it's simpler than using combine() and passing an extra parameter everywhere. LGTM!
There was a problem hiding this comment.
Yeah I don't know if it's better or worse actually, but I guess the result is almost the same.
There was a problem hiding this comment.
This is how it looks with combine instead :
val isContextMenuShownFlow = snapshotFlow { contextMenuState.value is RoomListState.ContextMenu.Shown }
.distinctUntilChanged()
val isFavoriteFlow = room.roomInfoFlow
.map { it.isFavorite }
.distinctUntilChanged()
combine(isContextMenuShownFlow, isFavoriteFlow) { isContextMenuShown, isFavorite ->
isContextMenuShown to isFavorite
}
.takeWhile { (isShowingContextMenu, _) -> isShowingContextMenu }
.onEach { (_, isFavorite) ->
contextMenuState.value = initialState.copy(
isFavorite = isFavorite,
)
}
.collect()
}
Let me know which one you prefer :P
There was a problem hiding this comment.
This is what I had tested locally. The previous version seems simpler, right? It's a bit harder to understand why it's after the onEach, but as long as we have at least 1 item emitted by the room info flow it should be fine.
There was a problem hiding this comment.
If you put it before it doesn't work because it won't cancel the stream because of the flatMapLatest creating a new stream.
Also the statement "as long as we have at least 1 item emitted by the room info flow it should be fine" is true also for the combine operator
| @@ -20,4 +20,5 @@ sealed interface RoomDetailsEvent { | |||
| data object LeaveRoom : RoomDetailsEvent | |||
| data object MuteNotification : RoomDetailsEvent | |||
| data object UnmuteNotification : RoomDetailsEvent | |||
| data class SetIsFavorite(val isFavorite: Boolean) : RoomDetailsEvent | |||
There was a problem hiding this comment.
Nit: I'd use SetFavorite instead
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2397 +/- ##
===========================================
+ Coverage 72.13% 72.16% +0.02%
===========================================
Files 1359 1359
Lines 32123 32196 +73
Branches 6277 6289 +12
===========================================
+ Hits 23173 23233 +60
- Misses 5696 5700 +4
- Partials 3254 3263 +9 ☔ View full report in Codecov by Sentry. |
ganfra
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
|
Not sure why, but something is wrong with the kover
|
|
ganfra
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
| present() | ||
| } | ||
| }.test(validate = validate) | ||
| } |
Type of change
Content
Add ways to mark a room as favourite from room list and room details.
Motivation and context
Closes #2208
Screenshots / GIFs
Tests
Tested devices
Checklist