-
-
Notifications
You must be signed in to change notification settings - Fork 522
feat(player): move screenshot writing task to background thread #7735
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
Conversation
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, please split this into two different commits, as it's two different changes (this makes it easier to follow the history).
Also, I think
- having a snackbar to show that a screenshot is being created isn't really necessary, as that is already obvious when the user is asked to choose a file and generally saving the file is relatively quick
- we should use a toast here, instead of a snackbar, considering the user does not have any useful interaction, similar to how we show a toast when a SponsorBlock segment is skipped. Alternatively I'm also open to adding a share button to the snackbar.
This depends on the quality and/or the content of what is being captured. PNG is quite costly when it comes to complex/detailed picture. It could take up to a few seconds depending on the device. I think it would be awkward if we only notify the user after it's succesfully saved, because the wait time could be several seconds. But I'm fine removing it though.
The reason I use snackbar is just to match the material design. Toast is displayed by the system framework, so different android version have different looks. So if we really follow the material design I think we need start to use snackbar from now on?
I think that's a great idea. |
Prevents UI thread blocking.
355b0c4 to
6b3aaae
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.
Thank you both, I agree that a share button justified adding the snackbar here 👍
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.
Added two more small nitpicks, but otherwise LGTM.
app/src/main/java/com/github/libretube/ui/fragments/PlayerFragment.kt
Outdated
Show resolved
Hide resolved
|
Thanks! |
Also added snackbar to notify user.