Allow downloading multiple images into one directory #7030

major-mayer · 2024-09-26T20:23:06Z

First time contributor checklist:

I have read the README and Contributor Guidelines
I have signed the Contributor Licence Agreement

Contributor checklist:

My contribution is not related to translations.
My commits are in nice logical chunks with good commit messages
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 🥳
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.

major-mayer

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.

ts/components/conversation/TimelineMessage.tsx

major-mayer · 2024-09-30T09:41:35Z

ts/state/ducks/conversations.ts

+    // TODO shouldn't we throw some kind of error here? Original code also doesn't do anything, but...
+    if (fullPath == null) return;


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?

Yeah, this should probably be a throw

Changed that, please check the error message.

app/main.ts

ts/components/conversation/TimelineMessage.tsx

scottnonnenberg-signal · 2024-10-03T03:54:55Z

ts/components/conversation/TimelineMessage.tsx

+          kickOffAttachmentDownload({
+            attachment,
+            messageId: id,
+          });


We probably want a toast here telling the user why the save dialog isn't showing up.

Something like "Can't save attachment, since it hasn't finished downloading yet." ?

Yeah, probably pluralized for the single vs. multiple case.

I added a new toast type and an action "showAttachmentDownloadStillInProgressToast" for this.
Hope that was correct, but I didn't know how to show the toast otherwise.

ts/state/ducks/conversations.ts

ts/windows/attachments.ts

scottnonnenberg-signal · 2024-10-03T04:06:35Z

ts/state/ducks/conversations.ts

+    dispatch({
+      type: SHOW_TOAST,
+      payload: {
+        toastType: ToastType.FileSaved,


We need a new toast for this situation, because the toast currently says the singular 'Attachment saved.' instead of listing the number saved.

(or a pluralized toast, with a number parameter or something)

I pluralized the toast message if multiple attachments are saved.

ts/state/ducks/conversations.ts

ts/components/ToastManager.stories.tsx

ts/components/conversation/Message.tsx

scottnonnenberg-signal · 2024-10-06T20:45:11Z

ts/components/conversation/TimelineMessage.tsx

@@ -23,7 +23,7 @@ import type {
  PropsData as MessagePropsData,
  PropsHousekeeping,
 } from './Message';
-import type { PushPanelForConversationActionType } from '../../state/ducks/conversations';
+import { type PushPanelForConversationActionType } from '../../state/ducks/conversations';


I think you want the type to be outside the braces, in fact. That's generally how we do things.

I reverted the change, but shouldn't npm run format take care of this.
I cannot remember that I did this change intentionally and had hoped that Prettier will include a rule for these imports.

ts/state/ducks/conversations.ts

ts/components/conversation/TimelineMessage.tsx

scottnonnenberg-signal · 2024-10-06T21:41:23Z

_locales/de/messages.json

  },
  "icu:attachmentSavedShow": {
    "messageformat": "Im Ordner anzeigen"
  },
+  "icu:attachmentStillDownloading": {
+    "messageformat": "{count, plural, one {Anhang kann} other {Anhänge können}} nicht gespeichert werden, da {count, plural, one {er} other {#}} noch nicht fertig heruntergeladen {count, plural, one {wurde} other {wurden}}."
+  },


We don't modify the non-english files - those are managed by our translators. This will be overwritten the next time we pull strings.

Okay, I wasn't sure how to interpret this "You only need to modify the default locale _locales/en/messages.json." statement.
I thought this means that I have to modify the English file, but can edit other languages as well.
In my case this was necessary, because I had the Signal instance running in German and it crashed when the translated string wasn't present.

But if I understand you correctly, this means that I can leave the string as is?

_locales/en/messages.json

ts/components/ToastManager.stories.tsx

scottnonnenberg-signal · 2024-10-09T23:47:32Z

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?

Use download directory as default for single file downloads Check if any attachment is still pending Cleaning up comments Apply suggestions from code review Co-authored-by: Scott Nonnenberg <[email protected]> Add more changes from code review Revert change to default path in show-save-dialog method Add toast when attachment download is still in progress + pluralize FileSaved toast + fix linter errors Remove unnecessary comment More changes from code review Remove comment Remove comment

major-mayer · 2024-10-10T12:25:35Z

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 🙂

scottnonnenberg-signal · 2024-10-24T04:41:26Z

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: 76e2597

major-mayer · 2024-10-25T08:52:27Z

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

scottnonnenberg-signal self-requested a review September 26, 2024 22:34

scottnonnenberg-signal self-assigned this Sep 26, 2024

scottnonnenberg-signal added the PR: Needs Review label Sep 26, 2024

major-mayer commented Sep 30, 2024

View reviewed changes