-
Notifications
You must be signed in to change notification settings - Fork 741
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
join and leave methods moved from MembershipService to RoomService an… #5183
Conversation
…d SpaceService to split logic for rooms and spaces
Matrix SDKIntegration Tests Results:
|
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.
Small remarks otherwise ok for me!
@@ -184,6 +184,10 @@ internal class DefaultSpaceService @Inject constructor( | |||
return joinSpaceTask.execute(JoinSpaceTask.Params(spaceIdOrAlias, reason, viaServers)) | |||
} | |||
|
|||
override suspend fun leaveSpace(roomId: String, reason: String?) { |
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.
spaceId instead of 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.
initially changed to spaceId, but since we use Room for Space sometimes wasn't sure if it won't be confusing. Like
leaveSpace(spaceId = room.roomId). What do you think?
* @param roomId the roomId of the space to leave | ||
* @param reason optional reason for leaving the space | ||
*/ | ||
suspend fun leaveSpace(roomId: String, reason: String? = null) |
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.
spaceId instead of roomId?
@@ -225,7 +225,7 @@ class RoomListViewModel @AssistedInject constructor( | |||
val room = session.getRoom(roomId) ?: return@withState |
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.
remove this line?
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.
also looks good to me! just missing a changelog entry, as it's touching the sdk api I guess we'll need to use .sdk for the file type
Will leave to @ganfra to merge 💪
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 changes!
@@ -72,14 +72,14 @@ class SpaceLeaveAdvancedViewModel @AssistedInject constructor( | |||
try { | |||
state.selectedRooms.forEach { | |||
try { | |||
session.getRoom(it)?.leave(null) | |||
session.leaveRoom(it) |
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.
Behind the scene I think this is equivalent to leave a room or a space, but can it
be a spaceId
sometimes here?
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.
As far i understand we leave all selected rooms in a space here. Maybe there could be also subspaces, but I think rooms is more often and more relevant here.
There are methods to join and leave room/space in
MembershipService
but in context of this service (as part ofRoom
) it's not always possible to validate if current instance isRoom
orSpace
andJoinRoomTask
is always used, even though for spaces we should useJoinSpaceTask
. This PR removesjoin
andleave
methods fromMembershipService
and replaces it with a separate implementation inRoomService
andSpaceService
according to context. This will allow us to properly track events for analytics and also will prevent possible bugs when Space is treated as Room