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.

Sign up for GitHub

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

Convert DraftService to suspend functions #2405

Merged
merged 2 commits into from
Nov 18, 2020
Merged

Convert DraftService to suspend functions #2405

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
574d505
Convert DraftService to suspend functions
Dominaezzz Nov 8, 2020
82b21e6
Ensure draft is saved with NonCancellable
Dominaezzz Nov 17, 2020
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
6 changes: 2 additions & 4 deletions ...ix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/send/DraftService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,19 @@
package org.matrix.android.sdk.api.session.room.send

import androidx.lifecycle.LiveData
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.util.Cancelable
import org.matrix.android.sdk.api.util.Optional

interface DraftService {

/**
* Save or update a draft to the room
*/
fun saveDraft(draft: UserDraft, callback: MatrixCallback<Unit>): Cancelable
suspend fun saveDraft(draft: UserDraft)

/**
* Delete the last draft, basically just after sending the message
*/
fun deleteDraft(callback: MatrixCallback<Unit>): Cancelable
suspend fun deleteDraft()

/**
* Return the current draft or null
Expand Down
14 changes: 5 additions & 9 deletions ...d/src/main/java/org/matrix/android/sdk/internal/session/room/draft/DefaultDraftService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,14 @@ package org.matrix.android.sdk.internal.session.room.draft
import androidx.lifecycle.LiveData
import com.squareup.inject.assisted.Assisted
import com.squareup.inject.assisted.AssistedInject
import org.matrix.android.sdk.api.MatrixCallback
import kotlinx.coroutines.withContext
import org.matrix.android.sdk.api.session.room.send.DraftService
import org.matrix.android.sdk.api.session.room.send.UserDraft
import org.matrix.android.sdk.api.util.Cancelable
import org.matrix.android.sdk.api.util.Optional
import org.matrix.android.sdk.internal.task.TaskExecutor
import org.matrix.android.sdk.internal.task.launchToCallback
import org.matrix.android.sdk.internal.util.MatrixCoroutineDispatchers

internal class DefaultDraftService @AssistedInject constructor(@Assisted private val roomId: String,
private val draftRepository: DraftRepository,
private val taskExecutor: TaskExecutor,
private val coroutineDispatchers: MatrixCoroutineDispatchers
) : DraftService {

Expand All @@ -43,14 +39,14 @@ internal class DefaultDraftService @AssistedInject constructor(@Assisted private
* The draft stack can contain several drafts. Depending of the draft to save, it will update the top draft, or create a new draft,
* or even move an existing draft to the top of the list
*/
override fun saveDraft(draft: UserDraft, callback: MatrixCallback<Unit>): Cancelable {
return taskExecutor.executorScope.launchToCallback(coroutineDispatchers.main, callback) {
override suspend fun saveDraft(draft: UserDraft) {
withContext(coroutineDispatchers.main) {
draftRepository.saveDraft(roomId, draft)
}
}

override fun deleteDraft(callback: MatrixCallback<Unit>): Cancelable {
return taskExecutor.executorScope.launchToCallback(coroutineDispatchers.main, callback) {
override suspend fun deleteDraft() {
withContext(coroutineDispatchers.main) {
draftRepository.deleteDraft(roomId)
}
}
Expand Down
38 changes: 21 additions & 17 deletions vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,22 +476,24 @@ class RoomDetailViewModel @AssistedInject constructor(
* Convert a send mode to a draft and save the draft
*/
private fun handleSaveDraft(action: RoomDetailAction.SaveDraft) = withState {
when {
it.sendMode is SendMode.REGULAR && !it.sendMode.fromSharing -> {
setState { copy(sendMode = it.sendMode.copy(action.draft)) }
room.saveDraft(UserDraft.REGULAR(action.draft), NoOpMatrixCallback())
}
it.sendMode is SendMode.REPLY -> {
setState { copy(sendMode = it.sendMode.copy(text = action.draft)) }
room.saveDraft(UserDraft.REPLY(it.sendMode.timelineEvent.root.eventId!!, action.draft), NoOpMatrixCallback())
}
it.sendMode is SendMode.QUOTE -> {
setState { copy(sendMode = it.sendMode.copy(text = action.draft)) }
room.saveDraft(UserDraft.QUOTE(it.sendMode.timelineEvent.root.eventId!!, action.draft), NoOpMatrixCallback())
}
it.sendMode is SendMode.EDIT -> {
setState { copy(sendMode = it.sendMode.copy(text = action.draft)) }
room.saveDraft(UserDraft.EDIT(it.sendMode.timelineEvent.root.eventId!!, action.draft), NoOpMatrixCallback())
viewModelScope.launch {
Copy link
Member

Choose a reason for hiding this comment

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

We call this action when the Fragment is paused, so there is maybe a risk that the action is cancelled, because the vieWModelScope will be cancelled (I think). Have you tested that change on a device?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

viewModelScope is cancelled when the ViewModel is cleared, which happens when the owning Activity/Fragment is destroyed (so it will survive rotations and other config changes too).
Does this need to outlive the fragment's destruction?

To inspire confidence, I've linked docs that outline the behaviour.

I have not tested this on a device.

Copy link
Member

Choose a reason for hiding this comment

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

Does this need to outlive the fragment's destruction?

Yes, I think so, because we save the draft when the user leaves, and the RoomDetailActivity will be destroyed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay. I'll probably just use withContext(NonCancellable) for it but I'll give it some thought first.

Copy link
Member

Choose a reason for hiding this comment

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

Yes NonCancellable is the way to go I think 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up putting it in the launch directly, to ensure it doesn't get cancelled between launch and withContext.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, LGTM now :)

when {
it.sendMode is SendMode.REGULAR && !it.sendMode.fromSharing -> {
setState { copy(sendMode = it.sendMode.copy(action.draft)) }
room.saveDraft(UserDraft.REGULAR(action.draft))
}
it.sendMode is SendMode.REPLY -> {
setState { copy(sendMode = it.sendMode.copy(text = action.draft)) }
room.saveDraft(UserDraft.REPLY(it.sendMode.timelineEvent.root.eventId!!, action.draft))
}
it.sendMode is SendMode.QUOTE -> {
setState { copy(sendMode = it.sendMode.copy(text = action.draft)) }
room.saveDraft(UserDraft.QUOTE(it.sendMode.timelineEvent.root.eventId!!, action.draft))
}
it.sendMode is SendMode.EDIT -> {
setState { copy(sendMode = it.sendMode.copy(text = action.draft)) }
room.saveDraft(UserDraft.EDIT(it.sendMode.timelineEvent.root.eventId!!, action.draft))
}
}
}
}
Expand Down Expand Up @@ -778,7 +780,9 @@ class RoomDetailViewModel @AssistedInject constructor(
} else {
// Otherwise we clear the composer and remove the draft from db
setState { copy(sendMode = SendMode.REGULAR("", false)) }
room.deleteDraft(NoOpMatrixCallback())
viewModelScope.launch {
room.deleteDraft()
}
}
}

Expand Down