Allow downloading multiple images into one directory #7030
Conversation
There was a problem hiding this comment.
In general, I tried to re-use as much existing code as possible and not mutate existing functions.
That's why I added an additional saveAttachments() function that takes an array of attachments, opens a directory picker dialog once and uses this base directory for all files.
One could probably combine the functionality to save a single attachment (with a user-defined name) and multiple attachments into one common function, but I didn't want to risk breaking too much existing functionality.
If wanted, I can use saveAttachments() for single attachments as well, skip the showSaveMultiDialog() call if the array's length is one and set dirPath to undefined.
| // TODO This will only get executed on the first time the component is rendered | ||
| // How to check if attachment[x].pending changes? | ||
| const hasPendingAttachments = (): boolean => { | ||
| // Ensure that no attachment is still pending | ||
| if (attachments && attachments.length) { | ||
| return attachments.every(attachment => !attachment.pending); | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
I could need some help here.
I added this function to check if any of the attachments is in pending state, and not just the first one, as it was before (maybe that's not even necessary?).
Right now, I think this function is only run once, when the component is rendered, which mean that if the pending state changes later, the result will not change and thus the download button will not be rendered.
How can I make sure that this function gets re-run whenever the pending attribute of an attachment changes?
I think using useCallback() or useMemo() is not the right approach here, because it only notices when the reference to attachments changes, right?
| // TODO shouldn't we throw some kind of error here? Original code also doesn't do anything, but... | ||
| if (fullPath == null) return; |
There was a problem hiding this comment.
In saveAttachments() (so the function to download a single attachment), I guess this was a check to skip showing the toast, when the user cancels the download/ file picker dialog.
Since I'm doing this check a few lines above, we probably should either remove this check here completely or throw an error, because something went wrong in the Attachments.save() method?
| @@ -3008,7 +3008,7 @@ ipc.handle('show-save-dialog', async (_event, { defaultPath }) => { | |||
| const { canceled, filePath: selectedFilePath } = await dialog.showSaveDialog( | |||
| mainWindow, | |||
| { | |||
| defaultPath, | |||
| defaultPath: join(app.getPath('downloads'), defaultPath), | |||
There was a problem hiding this comment.
Here I changed the original behavior of the save dialog for a single attachment, because I think it makes sense to use the users download directory as a base directory.
Otherwise, when I try to save a file, it uses ./ as the default directory and I have to navigate to downloads manually.
First time contributor checklist:
Contributor checklist:
mainbranchnpm run readyrun passes successfully (more about tests here)Description
Describe briefly what your pull request changes. Focus on the value provided to users.
Today I noticed, that right now it's not possible to download multiple images at once, which makes saving large galleries to the disk very time-consuming.
I was very surprised why this is the case, given that downloading a single image works without problems.
So I decided that I could maybe take a look into the code and try to fix things on my own, because the feature requests regarding this issue have not seen any attention for 3 years.
My experience with React is basically non-existent, so it was a lot of copy-pasting and adjusting the existing approach to download a single attachment.
It took a while to understand the data-flow between the different files, functions and actions, but I got it to work 🥳
I would like to get some feedback on my approach, then I can clean it up, maybe add some tests (although I would need some guidance on how to write these, maybe a similar example) and rebase it.
Does it address any outstanding issues in this project?
Please write a summary of your test approach:
Edit: In addition to my regular Manjaro Linux system, I successfully tested the changes with Windows 11 as well.