Allow downloading multiple images into one directory

major-mayer commented 1 month ago

First time contributor checklist:

[x] I have read the README and Contributor Guidelines
[x] I have signed the Contributor Licence Agreement

Contributor checklist:

[x] My contribution is not related to translations.
[ ] My commits are in nice logical chunks with good commit messages
[x] My changes are rebased on the latest main branch
[ ] A npm run ready run passes successfully (more about tests here)
[ ] My changes are ready to be shipped to users

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 :partying_face: 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?

No, but it's my take on trying to fix these two feature requests:
- https://community.signalusers.org/t/download-all-images/50998
- https://community.signalusers.org/t/desktop-multiple-image-download/31259

Please write a summary of your test approach:

What kind of manual testing did you do?
- I only used the staging environment so far, and sent a few pictures and other attachments to myself. I checked if I could download them without any problems.
Did you write any new tests?
- At least not yet. I checked briefly if I could find any test regarding the download of single attachments, but couldn't find one. Before I invest more time into this, I would like to get some feedback, if this has the chance to be accepted.
What operating systems did you test with? (please use specific versions: http://whatsmyos.com/)
- Manjaro KDE Wayland
- Windows 11 Home 23H2 22631.4037
What other devices did you test with? (other Desktop devices, Android, Android Simulator, iOS, iOS Simulator)
- None, but I could test it on my laptop as well (right now just desktop PC).

Edit: In addition to my regular Manjaro Linux system, I successfully tested the changes with Windows 11 as well.

scottnonnenberg-signal commented 1 month ago

I think we're ready to merge this as soon as that last TODO is removed. Would you consider pushing a squashed version of all these changes, taking it down to one commit?

major-mayer commented 1 month ago

I think we're ready to merge this as soon as that last TODO is removed. Would you consider pushing a squashed version of all these changes, taking it down to one commit?

Done, I hope the PR is ready now :slightly_smiling_face:

scottnonnenberg-signal commented 4 weeks ago

Thanks for all of your work on this - it was merged internally, and is now available in our latest beta: https://github.com/signalapp/Signal-Desktop/releases/tag/v7.31.0-beta.1

You can see the final commit here: https://github.com/signalapp/Signal-Desktop/commit/76e2597d30dd5bf86445022ef047492156125717

major-mayer commented 4 weeks ago

I'm glad (and a bit proud ;) to see my changes finally merged. Thanks for your great support during development :heart:

signalapp / Signal-Desktop