Allow any number of pasted images as attachments.
Previously, the code simply deduped attachments on file name, using the internal name image.png for any pasted image. This effectively made it possible to paste only a single image into a message.
With this patch, we now ignore files called image.png when deduping.
Deduping on file name is flawed anyway, because:
-
It's trivial to post multiple identical attachments, so long as each of them has a different file name.
-
It's not possible to post different attachments if they happen to have the same base file name (i.e. after the directory component is removed).
So, we're actually getting both false positives and false negatives here.
Fixes oxen-io/session-desktop-temp#410, but introduces a new problem:
When deleting a pasted attachment from a list of such attachments prior to sending, all pasted attachments are removed, presumably due to their all having the same internal file name.
It may be better not to dedupe attachments at all, rather than use the crude mechanism of the file name. In that case, the following code would suffice:
- const uniqAttachments = _.uniqBy(allAttachments, m => m.fileName);
- state.stagedAttachments[conversationKey] = uniqAttachments;
+ state.stagedAttachments[conversationKey] = allAttachments;
UPDATE 2023-03-14:
This PR has now been updated to not dedupe attachments at all at staging time.
Contributor checklist:
- [x] My commits are in nice logical chunks with good commit messages
- [x] My changes are rebased on the latest
clearnetbranch - [x] A
yarn readyrun passes successfully (more about tests here) - [x] My changes are ready to be shipped to users
Ah i was a little confused about what this PR did, but just worked it out by reading oxen-io/session-desktop-temp#410, basically if an image has the same filename as another image, but the content is different Session will still try to deduplicate the images and wont allow the second image to be attached.
I think the proper solution here is to deduplicate based on the content of the image instead of the image name, for this we can probably just compare a hash of the image data. Otherwise we will have the issue described above where both images are deleted when cleared if they share the same filename. Although the simple solution is probably just to remove deduplication, is there a major case in which deduplication of attachments is actually useful i can't think of it being very useful?
Ah i was a little confused about what this PR did, but just worked it out by reading oxen-io/session-desktop-temp#410, basically if an image has the same filename as another image, but the content is different Session will still try to deduplicate the images and wont allow the second image to be attached.
The specific problem I was trying to fix was the inability to paste into a message more than a single image from the clipboard. All such pasted images get the same internal file name, which runs afoul of the deduping code.
Although the simple solution is probably just to remove deduplication, is there a major case in which deduplication of attachments is actually useful i can't think of it being very useful?
Yes, my approach here tried to preserve as much of the current behaviour as possible, whilst still allowing multiple images to be pasted into a message from the clipboard. I agree, however, that simply removing the deduping code would be the easier solution. If it is to be kept, however, I also agree that the deduping should be done on actual content, not on file name.
I have now completely removed deduping from the attachment staging code, as I think we agree this serves no useful purpose.
The issue with deleting a pasted image from a series of pasted images remains, however. Because they all receive the internal name image.png, deleting one deletes all of them.
I cannot find where in the code this internal name is assigned, but it would be much better to use a checksum (MD5 or SHA256) of the file's content for the name.