-
Notifications
You must be signed in to change notification settings - Fork 172
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
Close the media preview screen ASAP with sending queue enabled #4089
Close the media preview screen ASAP with sending queue enabled #4089
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
8f2ed8f
to
7e3d8b1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4089 +/- ##
===========================================
- Coverage 83.28% 83.26% -0.02%
===========================================
Files 1885 1885
Lines 49096 49128 +32
Branches 5764 5775 +11
===========================================
+ Hits 40888 40908 +20
- Misses 6139 6144 +5
- Partials 2069 2076 +7 ☔ View full report in Codecov by Sentry. |
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 improvement. I suspect that the gain is not as big as expected by the product, please read my comments for more details.
|
||
@SingleIn(RoomScope::class) | ||
@MergeSubcomponent(RoomScope::class) | ||
@MergeSubcomponent(RoomScope::class, modules = [SessionMatrixModule::class]) |
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.
This change does not seem to be necessary?
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.
Either it is or the build cache was causing some issues locally for me, since I couldn't get the app to build until I explicitly added that. I think I also tried removing it later and came back to the same error where it couldn't find the @SessionCoroutineScope
annotated coroutine scope, but I can double check just in case.
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 was the build cache ¯\_(ツ)_/¯.
@@ -240,7 +251,6 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( | |||
cleanUp(mediaUploadInfo) | |||
// Reset the sendActionState to ensure that dialog is closed before the screen | |||
sendActionState.value = SendActionState.Done | |||
onDoneListener() |
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 still need to invoke this in the case of sendOnBackground
is false, i.e. FeatureFlags.MediaUploadOnSendQueue
is disabled.
Confirmed by test: the screen does not close
sendPreProcessedMedia( | ||
mediaUploadInfo = mediaUploadInfo.data, | ||
caption = caption, | ||
sendActionState = sendActionState, | ||
) | ||
} | ||
|
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.
Just a remark here: when the send queue is enabled, the duration of the invokation of sendPreProcessedMedia
stay quite short (but not negligible). I have measure that it can take up to 1s for big files. The processing of the thumbnail creation and video shrinking is much longer. So this PR is OK, but the gain is not that big.
Also note that if the user close the screen during the media pre-processing, it will cancel the media sending as well. So maybe use the session scope also for the media pre-processing? In this case the local echo will take longer to appear and killing the app during the processing will stop the sending.
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 note that if the user close the screen during the media pre-processing, it will cancel the media sending as well.
But that's OK I think? The media pre-processing should be done before sending starts, and in the meantime the screen won't be dismissed since we'll have the progress dialog present: dismissing this dialog cancels the enqueued sending process, so AFAICT there's no way to close the screen before the preprocessing is done while expecting the sending to still work.
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 note that if the user close the screen during the media pre-processing, it will cancel the media sending as well. So maybe use the session scope also for the media pre-processing? In this case the local echo will take longer to appear and killing the app during the processing will stop the sending.
You can ignore this, I initially though that the PR will make the screen be closed as soon as the user press the send button (even if the pre-processing is not over), but this is not the case. Sorry about this!
…send queue is enabled
… after the media has been sent
7e3d8b1
to
47362ba
Compare
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.
LGTM, thanks for the update!
Quality Gate passedIssues Measures |
Content
@SessionCoroutineScope
from theSessionMatrixModule
and use it to send the media even if the preview screen or the chat room screen are dismissed.Motivation and context
We decided to have this behaviour to improve the UX.
Screenshots / GIFs
Screen_recording_20241223_133920.mp4
Tests
Tested devices
Checklist