-
Notifications
You must be signed in to change notification settings - Fork 278
Leave space #5354
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
base: develop
Are you sure you want to change the base?
Leave space #5354
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5354 +/- ##
===========================================
+ Coverage 79.75% 79.77% +0.02%
===========================================
Files 2273 2276 +3
Lines 62817 63010 +193
Branches 7863 7887 +24
===========================================
+ Hits 50098 50268 +170
- Misses 9826 9838 +12
- Partials 2893 2904 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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! LGTM in general, a couple of comments but nothing blocking.
data object StartLeaveSpace : SpaceEvents | ||
data object LeaveSpace : SpaceEvents |
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.
Do we have some naming/structure we usually follow for these cases?
I think we'd usually have something like data class LeaveSpace(val needsConfirmation: Boolean)
and use tell apart the 2 states. Or we'd also sometimes have LeaveSpace
and some DoLeaveSpace
events, if I'm not mistaken.
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.
True, we also have AsyncAction
with a confirmation state, which is maybe better here. I'll give it a try
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.
import io.element.android.libraries.matrix.api.timeline.item.event.MembershipChange | ||
import kotlinx.coroutines.flow.MutableSharedFlow | ||
import kotlinx.coroutines.flow.asSharedFlow | ||
|
||
class RoomMembershipObserver { | ||
data class RoomMembershipUpdate( | ||
val roomId: RoomId, | ||
val roomInfo: RoomInfo, |
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.
It seems a bit weird to bundle the whole RoomInfo
here, is it just to have access to the isSpace
property?
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.
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.
Nit: the phrasing here seems a bit odd, I'd expect something like "You'll be leaving all your rooms in this space too", which seems simpler. But if it was designed like this 🤷 .
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.
The wording will probably get updated, I'll move this PR to draft.
|
Content
Add ability to leave space. For now we are using the leave room endpoint and no rooms are left when a space is left. We may have a new C api or expect new Rust SDK api to get the list of rooms to left when leaving the space.
Motivation and context
Closes #5300
Partially implement #5301
Screenshots / GIFs
See recorded ones
Tests
Tested devices
Checklist