-
Notifications
You must be signed in to change notification settings - Fork 191
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
Allow any number of pasted images as attachments. #2681
base: unstable
Are you sure you want to change the base?
Conversation
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. Now, we ignore files called `image.png` when deduping. Deduping on file name is flawed anyway, because: 1. It's trivial to post multiple identical attachments, so long as each of them has a different file name. 2. 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#2345, 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; ```
Ah i was a little confused about what this PR did, but just worked it out by reading #2345, 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? |
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.
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 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. |
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 #2345, 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:
UPDATE 2023-03-14:
This PR has now been updated to not dedupe attachments at all at staging time.
Contributor checklist:
clearnet
branchyarn ready
run passes successfully (more about tests here)