Skip to content
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

Open
wants to merge 2 commits into
base: clearnet
Choose a base branch
from

Conversation

ianmacd
Copy link

@ianmacd ianmacd commented Feb 17, 2023

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:

  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 #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;

UPDATE 2023-03-14:

This PR has now been updated to not dedupe attachments at all at staging time.

Contributor checklist:

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;
```
@KeeJef
Copy link
Collaborator

KeeJef commented Feb 27, 2023

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?

@ianmacd
Copy link
Author

ianmacd commented Feb 27, 2023

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.

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.

@ianmacd
Copy link
Author

ianmacd commented Mar 14, 2023

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.

@ianmacd ianmacd changed the title Dedupe attachments by file name, but allow any number of pasted images. Allow any number of pasted images. Mar 14, 2023
@ianmacd ianmacd changed the title Allow any number of pasted images. Allow any number of pasted images as attachments. Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot paste more than one image into a new posting.
2 participants