-
Notifications
You must be signed in to change notification settings - Fork 156
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
fix : use RoomMembershipObserver to close room screen when leaving #3887
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3887 +/- ##
===========================================
- Coverage 82.87% 82.85% -0.02%
===========================================
Files 1784 1784
Lines 45090 45088 -2
Branches 5324 5323 -1
===========================================
- Hits 37368 37358 -10
- Misses 5857 5865 +8
Partials 1865 1865 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, 2 small remarks.
// If the user leaves the room from this client, close the room flow. | ||
membershipObserver.updates | ||
.filter { it.roomId == roomId && !it.isUserInRoom } | ||
.onEach { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add .take(1)
to navigateUp
only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we get two membership updates there is something wrong, but yeah sure, I can change
} | ||
room.leave() | ||
.onFailure { | ||
Timber.e(it, "Error while leaving room ${room.displayName} - ${room.roomId}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not new but we should only log the roomId
for privacy reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
…clining invite/canceling knock request
e75d6fa
to
aa1a151
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
onCancelKnockSuccess = ::navigateUp, | ||
onKnockSuccess = { }, | ||
onCancelKnockSuccess = {}, | ||
onKnockSuccess = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for later, but I believe that the parameters onCancelKnockSuccess
and onKnockSuccess
could be removed.
Content
Close the room flow when leaving a room from the client.
If leaving, being banned from another client, the room screen will update but won't navigate up to the room list.
Motivation and context
Screenshots / GIFs
No ui change.
Tests
Tested devices
Checklist