Skip to content
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

Merged
merged 2 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@ interface RoomService {
thirdPartySigned: SignInvitationResult
)

/**
* Leave the room, or reject an invitation.
* @param roomId the roomId of the room to leave
* @param reason optional reason for leaving the room
*/
suspend fun leaveRoom(roomId: String, reason: String? = null)

/**
* Get a room from a roomId
* @param roomId the roomId to look for.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,4 @@ interface MembershipService {

@Deprecated("Use remove instead", ReplaceWith("remove(userId, reason)"))
suspend fun kick(userId: String, reason: String? = null) = remove(userId, reason)

/**
* Join the room, or accept an invitation.
*/
suspend fun join(reason: String? = null, viaServers: List<String> = emptyList())

/**
* Leave the room, or reject an invitation.
*/
suspend fun leave(reason: String? = null)
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ interface Space {

val spaceId: String

suspend fun leave(reason: String? = null)

/**
* A current snapshot of [RoomSummary] associated with the space
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@ interface SpaceService {

suspend fun rejectInvite(spaceId: String, reason: String?)

/**
* Leave the space, or reject an invitation.
* @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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaceId instead of roomId?


// fun getSpaceParentsOfRoom(roomId: String) : List<SpaceSummary>

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import org.matrix.android.sdk.internal.session.room.create.CreateRoomTask
import org.matrix.android.sdk.internal.session.room.membership.RoomChangeMembershipStateDataSource
import org.matrix.android.sdk.internal.session.room.membership.RoomMemberHelper
import org.matrix.android.sdk.internal.session.room.membership.joining.JoinRoomTask
import org.matrix.android.sdk.internal.session.room.membership.leaving.LeaveRoomTask
import org.matrix.android.sdk.internal.session.room.peeking.PeekRoomTask
import org.matrix.android.sdk.internal.session.room.peeking.ResolveRoomStateTask
import org.matrix.android.sdk.internal.session.room.read.MarkAllRoomsReadTask
Expand All @@ -66,7 +67,8 @@ internal class DefaultRoomService @Inject constructor(
private val peekRoomTask: PeekRoomTask,
private val roomGetter: RoomGetter,
private val roomSummaryDataSource: RoomSummaryDataSource,
private val roomChangeMembershipStateDataSource: RoomChangeMembershipStateDataSource
private val roomChangeMembershipStateDataSource: RoomChangeMembershipStateDataSource,
private val leaveRoomTask: LeaveRoomTask,
) : RoomService {

override suspend fun createRoom(createRoomParams: CreateRoomParams): String {
Expand Down Expand Up @@ -133,6 +135,10 @@ internal class DefaultRoomService @Inject constructor(
joinRoomTask.execute(JoinRoomTask.Params(roomId, reason, thirdPartySigned = thirdPartySigned))
}

override suspend fun leaveRoom(roomId: String, reason: String?) {
leaveRoomTask.execute(LeaveRoomTask.Params(roomId, reason))
}

override suspend fun markAllAsRead(roomIds: List<String>) {
markAllRoomsReadTask.execute(MarkAllRoomsReadTask.Params(roomIds))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ import org.matrix.android.sdk.internal.query.QueryStringValueProcessor
import org.matrix.android.sdk.internal.query.process
import org.matrix.android.sdk.internal.session.room.membership.admin.MembershipAdminTask
import org.matrix.android.sdk.internal.session.room.membership.joining.InviteTask
import org.matrix.android.sdk.internal.session.room.membership.joining.JoinRoomTask
import org.matrix.android.sdk.internal.session.room.membership.leaving.LeaveRoomTask
import org.matrix.android.sdk.internal.session.room.membership.threepid.InviteThreePidTask
import org.matrix.android.sdk.internal.util.fetchCopied

Expand All @@ -48,8 +46,6 @@ internal class DefaultMembershipService @AssistedInject constructor(
private val loadRoomMembersTask: LoadRoomMembersTask,
private val inviteTask: InviteTask,
private val inviteThreePidTask: InviteThreePidTask,
private val joinTask: JoinRoomTask,
private val leaveRoomTask: LeaveRoomTask,
private val membershipAdminTask: MembershipAdminTask,
@UserId
private val userId: String,
Expand Down Expand Up @@ -139,14 +135,4 @@ internal class DefaultMembershipService @AssistedInject constructor(
val params = InviteThreePidTask.Params(roomId, threePid)
return inviteThreePidTask.execute(params)
}

override suspend fun join(reason: String?, viaServers: List<String>) {
val params = JoinRoomTask.Params(roomId, reason, viaServers)
joinTask.execute(params)
}

override suspend fun leave(reason: String?) {
val params = LeaveRoomTask.Params(roomId, reason)
leaveRoomTask.execute(params)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@ internal class DefaultSpace(

override val spaceId = room.roomId

override suspend fun leave(reason: String?) {
return room.leave(reason)
}

override fun spaceSummary(): RoomSummary? {
return spaceSummaryDataSource.getSpaceSummary(room.roomId)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaceId instead of roomId?

Copy link
Contributor Author

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?

leaveRoomTask.execute(LeaveRoomTask.Params(roomId, reason))
}

override suspend fun rejectInvite(spaceId: String, reason: String?) {
leaveRoomTask.execute(LeaveRoomTask.Params(spaceId, reason))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -801,14 +801,14 @@ class TimelineViewModel @AssistedInject constructor(

private fun handleRejectInvite() {
viewModelScope.launch {
tryOrNull { room.leave(null) }
tryOrNull { session.leaveRoom(room.roomId) }
}
}

private fun handleAcceptInvite() {
viewModelScope.launch {
tryOrNull {
room.join()
session.joinRoom(room.roomId)
analyticsTracker.capture(room.roomSummary().toAnalyticsJoinedRoom())
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ class MessageComposerViewModel @AssistedInject constructor(
is ParsedCommand.LeaveRoom -> {
viewModelScope.launch(Dispatchers.IO) {
try {
session.getRoom(slashCommandResult.roomId)?.leave(null)
session.leaveRoom(slashCommandResult.roomId)
popDraft()
_viewEvents.post(MessageComposerViewEvents.SlashCommandResultOk())
} catch (failure: Throwable) {
Expand Down Expand Up @@ -683,7 +683,9 @@ class MessageComposerViewModel @AssistedInject constructor(
?.roomId
?.let { session.getRoom(it) }
}
?.leave(reason = null)
?.let {
session.leaveRoom(it.roomId)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ class RoomListViewModel @AssistedInject constructor(
val room = session.getRoom(roomId) ?: return@withState
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line?

viewModelScope.launch {
try {
room.join()
session.joinRoom(room.roomId)
analyticsTracker.capture(action.roomSummary.toAnalyticsJoinedRoom())
// We do not update the joiningRoomsIds here, because, the room is not joined yet regarding the sync data.
// Instead, we wait for the room to be joined
Expand All @@ -245,10 +245,9 @@ class RoomListViewModel @AssistedInject constructor(
return@withState
}

val room = session.getRoom(roomId) ?: return@withState
viewModelScope.launch {
try {
room.leave(null)
session.leaveRoom(roomId)
// We do not update the rejectingRoomsIds here, because, the room is not rejected yet regarding the sync data.
// Instead, we wait for the room to be rejected
// Known bug: if the user is invited again (after rejecting the first invitation), the loading will be displayed instead of the buttons.
Expand Down Expand Up @@ -333,9 +332,8 @@ class RoomListViewModel @AssistedInject constructor(

private fun handleLeaveRoom(action: RoomListAction.LeaveRoom) {
_viewEvents.post(RoomListViewEvents.Loading(null))
val room = session.getRoom(action.roomId) ?: return
viewModelScope.launch {
val value = runCatching { room.leave(null) }
val value = runCatching { session.leaveRoom(action.roomId) }
.fold({ RoomListViewEvents.Done }, { RoomListViewEvents.Failure(it) })
_viewEvents.post(value)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import kotlinx.coroutines.sync.withPermit
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.failure.Failure
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.room.Room
import org.matrix.android.sdk.api.session.room.members.ChangeMembershipState
import org.matrix.android.sdk.api.session.room.model.Membership
import org.matrix.android.sdk.api.session.room.roomSummaryQueryParams
Expand Down Expand Up @@ -109,7 +108,7 @@ class InvitesAcceptor @Inject constructor(

private suspend fun Session.joinRoomSafely(roomId: String) {
if (shouldRejectRoomIds.contains(roomId)) {
getRoom(roomId)?.rejectInviteSafely()
rejectInviteSafely(roomId)
return
}
val roomMembershipChanged = getChangeMemberships(roomId)
Expand All @@ -126,16 +125,16 @@ class InvitesAcceptor @Inject constructor(
// if the inviting user is on the same HS, there can only be one cause: they left, so we try to reject the invite.
if (inviterId?.endsWith(sessionParams.credentials.homeServer.orEmpty()).orFalse()) {
shouldRejectRoomIds.add(roomId)
room.rejectInviteSafely()
rejectInviteSafely(roomId)
}
}
}
}
}

private suspend fun Room.rejectInviteSafely() {
private suspend fun Session.rejectInviteSafely(roomId: String) {
try {
leave(null)
leaveRoom(roomId)
shouldRejectRoomIds.remove(roomId)
} catch (failure: Throwable) {
Timber.v("Fail rejecting invite for room: $roomId")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class NotificationBroadcastReceiver : BroadcastReceiver() {
if (room != null) {
session.coroutineScope.launch {
tryOrNull {
room.join()
session.joinRoom(room.roomId)
analyticsTracker.capture(room.roomSummary().toAnalyticsJoinedRoom())
}
}
Expand All @@ -93,11 +93,8 @@ class NotificationBroadcastReceiver : BroadcastReceiver() {

private fun handleRejectRoom(roomId: String) {
activeSessionHolder.getSafeActiveSession()?.let { session ->
val room = session.getRoom(roomId)
if (room != null) {
session.coroutineScope.launch {
tryOrNull { room.leave() }
}
session.coroutineScope.launch {
tryOrNull { session.leaveRoom(roomId) }
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ class RoomProfileViewModel @AssistedInject constructor(
_viewEvents.post(RoomProfileViewEvents.Loading(stringProvider.getString(R.string.room_profile_leaving_room)))
viewModelScope.launch {
try {
room.leave(null)
session.leaveRoom(room.roomId)
// Do nothing, we will be closing the room automatically when it will get back from sync
} catch (failure: Throwable) {
_viewEvents.post(RoomProfileViewEvents.Failure(failure))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ class SpaceListViewModel @AssistedInject constructor(@Assisted initialState: Spa
private fun handleLeaveSpace(action: SpaceListAction.LeaveSpace) {
viewModelScope.launch {
tryOrNull("Failed to leave space ${action.spaceSummary.roomId}") {
session.spaceService().getSpace(action.spaceSummary.roomId)?.leave(null)
session.spaceService().leaveSpace(action.spaceSummary.roomId)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class SpaceMenuViewModel @AssistedInject constructor(
session.coroutineScope.launch {
try {
if (state.leaveMode == SpaceMenuState.LeaveMode.LEAVE_NONE) {
session.getRoom(initialState.spaceId)?.leave(null)
session.spaceService().leaveSpace(initialState.spaceId)
} else if (state.leaveMode == SpaceMenuState.LeaveMode.LEAVE_ALL) {
// need to find all child rooms that i have joined

Expand All @@ -143,13 +143,13 @@ class SpaceMenuViewModel @AssistedInject constructor(
}
).forEach {
try {
session.getRoom(it.roomId)?.leave(null)
session.spaceService().leaveSpace(it.roomId)
} catch (failure: Throwable) {
// silently ignore?
Timber.e(failure, "Fail to leave sub rooms/spaces")
}
}
session.getRoom(initialState.spaceId)?.leave(null)
session.spaceService().leaveSpace(initialState.spaceId)
}

// We observe the membership and to dismiss when we have remote echo of leaving
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class SpaceInviteBottomSheetViewModel @AssistedInject constructor(
setState { copy(joinActionState = Loading()) }
session.coroutineScope.launch(Dispatchers.IO) {
try {
session.getRoom(initialState.spaceId)?.join()
session.spaceService().joinSpace(initialState.spaceId)
setState { copy(joinActionState = Success(Unit)) }
} catch (failure: Throwable) {
setState { copy(joinActionState = Fail(failure)) }
Expand All @@ -116,7 +116,7 @@ class SpaceInviteBottomSheetViewModel @AssistedInject constructor(
setState { copy(rejectActionState = Loading()) }
session.coroutineScope.launch(Dispatchers.IO) {
try {
session.getRoom(initialState.spaceId)?.leave()
session.spaceService().leaveSpace(initialState.spaceId)
setState { copy(rejectActionState = Success(Unit)) }
} catch (failure: Throwable) {
setState { copy(rejectActionState = Fail(failure)) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,14 @@ class SpaceLeaveAdvancedViewModel @AssistedInject constructor(
try {
state.selectedRooms.forEach {
try {
session.getRoom(it)?.leave(null)
session.leaveRoom(it)
Copy link
Member

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?

Copy link
Contributor Author

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.

} catch (failure: Throwable) {
// silently ignore?
Timber.e(failure, "Fail to leave sub rooms/spaces")
}
}

session.getRoom(initialState.spaceId)?.leave(null)
session.spaceService().leaveSpace(initialState.spaceId)
// We observe the membership and to dismiss when we have remote echo of leaving
} catch (failure: Throwable) {
setState { copy(leaveState = Fail(failure)) }
Expand Down