-
Notifications
You must be signed in to change notification settings - Fork 732
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
Compress video and improve file too big error detection #3264
Conversation
|
||
override fun onCancelled() { | ||
Timber.d("Compressing: cancel") | ||
job.cancel() |
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.
@Dominaezzz there is probably room for improvement here, on https://github.com/AbedElazizShe/LightCompressor API, but also on my workaround :)
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.
I'm slightly confused by the use of both Job
and Dispatchers.IO
, I think one of them has to go.
In reality this API should've exposed a flow but oh well, it's not too hard to wrap.
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.
I've added the support to track the progress, but I'm a bit confused too about how I can improve that. The Dispatchers.IO is here because we are creating a file, and the Job is here to make the call "synchronous". What do you suggest?
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.
Looking at https://natario1.github.io/Transcoder/docs/events , (unfortunately) transcoding is already done on a background thread, so you don't need to use Dispatchers.IO
. You could also just limit it to just the createDestinationFile()
.
(I can't wait until ProgressListener
is replaced with Flow
)
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 feedback, I will change that. By the way the link you provide also shows me that there is a successCode
in the onTranscodeCompleted
fun. I did not noticed that. When the value is SUCCESS_NOT_NEEDED
it appears that the target file is not written, so it explain a bug I observed today with a 0-lenght file. I will fix that too.
ae668b0
to
52abb33
Compare
…ent fails if too large.
When using the file picker (and not the media picker). Now when using the file picker, we detect the mime type and we send the correct event Also some code duplication
Also create some extensions for future use
52abb33
to
9d225b7
Compare
… Light theme does not use .Bridge, so no reason to use it for Dark theme. I hope there will be no regression though...
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, you should update open_source_licence.html with transcoder lib
@BillCarsonFr the file |
Add support to compress video before sending.
Improve file too big detection. It's now handle by the SDK, since the file can be compressed, and so become acceptable for the server. The message bottom sheet now display the "too big file" error to the user.
Also the SDK checks the size after the encryption of the file (can be bigger when encrypted).
Thumbnails are now sent after the main content, to avoid sending them and then get an error when sending the main file.
Add support for preview of video files (no play preview though, just an indicator in the strip to show it's a video), to let the user selects the option to send the original uncompressed video.
Remaining issue (to be fixed in another PR?): if file is too large for the server when retrying, it forces to compress the file and so it has more chance to work. But it is not the original intent from the user.
This PR also:
EDIT:
Now when we do multiple selection, it handle correctly the several Event type (fixes #3177)
We can now take a video (this PR replaces #2411) Design input will be updated later, for now we are using a dialog when clicking on "Camera":
There also a new setting "Take photo or video" to update the stored value:
EDIT 2:
Added some information about attachment in the bottom sheet: when applicable user will see: duration, dimensions, size and mime type (for files only).
Example for a video: