-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Adds Bulk Processing Support #1428
base: dev
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
I'd love to see this added to Squoosh, this is fantastic. I wonder how it should handle multiple files with the same name but different formats? Maybe we can detect if this situation arises, and rename the files in question like Also, there are some minor visual bugs / inconsistencies:
|
Another thought: How should it handle the user's resize preferences? Particularly when images are of different aspect ratios and dimensions. |
@aryanpingle hey thanks for the feedback! I will have some more time later today to get through them plus some extra things I have in mind. Also found a bug when you export 100+ files as well. But bulk exporting as a single zip file should fix this? We'll see. |
So I find the UX for downloading multiple files right now kind of bad. I explored compression stream, thought it supports archiving too but turns out it doesn't. I would like to fix this first. @aryanpingle What do you think about using a zip library to archive files before download? I know this is going to be the first ever non-dev dependency of this project, but I can't think of any other way right now. The status quo is we loop over all files and for each, once it is processed, download. But before beginning the next iteration, we have to wait a bit as the browser will prevent fast, rapid consecutive downloads. This "a bit" amounts to quite a lot when exporting multiple files (200+). |
This is really great! For the zipping (presumably JSzip?): As there is no workaround, a dependecy is better than not providing the service all together. I could create a wasm file for you, providing just zipping, but this would still be a dependecy. |
About the download: If I upload multiple images for processing, I'd personally expect the download to be an archived one. Especially if I upload a lot. About the zip library: @surma correct me if I'm wrong, but adding dependencies shouldn't be against any design philosophy of Squoosh right? I think it's more about #UseThePlatform (taking advantage of native browser features rather than adding third-party, possible un-robust implementations). And of course, ensuring that unnecessary code from a dependency doesn't increase the page weight. If the JSzip package doesn't add too much to the bundle size, and we lazy-load it, this is a feature I'd rather have than not. |
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.
@Khongchai this is absolutely amazing! Thanks so much for taking this on! I applaud you for just jumping in. I haven’t given the code an in-depth review, but at a skim it looks like this PR is already in a really good spot.
Here’s a couple of things that I wanna surface:
- The controls at the top (with the new drop-down selector) do not react to touch inputs for me (Chrome on Android Pixel 5). They work on stable Squoosh.
- Like you said, the “Download All” buttons need some UX love. I actually think we should find a way to integrate it into the existing download button, because our UI space is scarce. Maybe @argyleink has time to help here.
- I’d love to make resizing make sense, but I understand that it may be better/easier to address that in a separate PR. Currently resizing behaves unexpectedly, so I wonder if — for now — the resize feature should get disabled if multiple images are loaded and they are not all the same size.
- I am not sure I am in favor of the ZIP approach. This is not because of a new dependency (that would indeed be fine), but for two other reason:
a. Downloading multiple images is supported by browsers. Downloading a zip means the user has to now unzip them. I am not sure why that is preferable?
b. More crucially, zipping means that the entire zip archive has to be built up in-memory, containing all images! This may get infeasible really quickly on mobile devices. Curious what others think here. @jakearchibald ?
* Download all using settings from one of the two sides by | ||
* queuing each image in succession. | ||
* | ||
* This may cause flashing on the user's screen. |
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.
Yeah, we should not go through multiple UI state changes just to do computational work. This may be finally the reason to refactor updateImage
(which is called by queueUpdateImage
) to separate the computational and caching parts from triggering preact state changes.
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.
Sounds like a good plan. Do you think this is enough work to justify doing it in a separate PR? Refactoring the computational part is going to affect existing functionality too, but I otherwise understand the need to for a polished UX before releasing.
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.
Yeah I understand. How do you feel about stacking PRs? We could declare this branch the new root for this feature, and fork off two branches — one for UX improvements on one for the computational refactor. How does that sound? (And it’s completely okay to say “that’s sounds shit”)
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.
lol I have no preference in that regard so if that's how you prefer to work then sure. I guess time will tell whether that's going to be confusing or not.
@simonmau Ha I had the same thought. I whipped up a really quick Wasm file with Rust and the |
Hi there, thanks for the review! Big fan you two :) The UI parts I think we all agree needs some more work. I think the main focus now seems to be how resizing and exporting (with zipping or not) should behave. I'll put these on hold for now. Also, I just reverted the zipping changes out of the PR. Now it's going to do sequential download. I was not able to do sequential download without waiting a bit before beginning the next download. With a lot of images, this will add up. Perhaps there are better ways to do bulk export in the browser? |
Amazing! Thanks for continuing to work on this.
To me, this is a really good start. Let’s leave the behavior like this. At some point, we could look into progressively enhancing using Filesystem API. |
Preview without zipping: https://squoosh-multiple-export.vercel.app/
Preview with zipping: https://squoosh-multiple-export-5j4zic097-khongchais-projects.vercel.app/
Key Changes
The core of the change involves selectively disabling one of the sides when calling queueUpdateImage and use the enabled one to process everything.
The rest are stylings to make the new buttons look ok.
Potential Improvements
One improvement that can be made would be to export all files as zip. Right now the browser sometimes do not prompt the user to allow multiple exports and now download will happen until the user refresh the page and retry again. However I see that there basically are no non-dev dependencies at all in this project -- I presumed this was a conscious decision to minimize dependencies so I was not sure if installing a zip library is the right approach here.
I'm open to feedback and suggestions for further improvements or alternative approaches to handling multiple images.
Post PR changes
More TODO items and feedback from somewhere else