-
Notifications
You must be signed in to change notification settings - Fork 742
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
Convert DraftService to suspend functions #2405
Conversation
Signed-off-by: Dominic Fischer <[email protected]>
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 new PR. I have one question though.
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 { |
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.
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?
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.
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.
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.
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
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.
Oh okay. I'll probably just use withContext(NonCancellable)
for it but I'll give it some thought first.
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.
Yes NonCancellable is the way to go I think 👍
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.
Ended up putting it in the launch
directly, to ensure it doesn't get cancelled between launch
and withContext
.
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 now :)
Signed-off-by: Dominic Fischer <[email protected]>
Thanks for the update @Dominaezzz . I have a doubt about using NonCancellable because in the (rare) case when the user will signout, we cancel every tasks and it won't be the case for this one. But as the draft is saved locally (no network delay...), the time for the user to signout, the draft will be saved. |
Signed-off-by: Dominic Fischer [email protected]
Pull Request Checklist