p0deje / Maccy

Lightweight clipboard manager for macOS
https://maccy.app
MIT License
11.94k stars 512 forks source link

Fix copying multiple files in Finder #714

Closed kthchew closed 7 months ago

kthchew commented 7 months ago

Fixes #695.

The UI currently looks like this:

Screenshot 2024-01-25 at 12 40 51 AM

In some places, I sometimes used .first as a shortcut to support the changes in HistoryItem from Data? to [Data]. I'm not sure if this is the best solution, but I believe it would match the behavior before this PR for those data types.

As for commit 00d41fc, my changes to fix the bug uncovered an issue where HistoryItemContent.value is sometimes nil, which caused a crash with images copied on another iCloud device with Universal Clipboard when Maccy is restarted. Without this PR, you can also observe this crash by trying to copy the affected image item (which appears blank after restarting Maccy). I thus had to remove the force unwrap.

p0deje commented 7 months ago

This is quite a change in how storage is organized, do you think it might be possible to keep the current contract and only change fileURL to fileURLs so that it always returns a list of files? That's so far the first case in years where we might want to represent pasteboard items in a single history item.

kthchew commented 7 months ago

I believe that would be possible. I think contentData might still need to return an array, but .first can be used everywhere except file URLs. I should be able to experiment with this by tomorrow.

kthchew commented 7 months ago

I pushed some less significant changes that only change how file URLs are handled. Let me know what you think.

p0deje commented 7 months ago

As for commit https://github.com/p0deje/Maccy/commit/00d41fc1da1cf9a26f8c2edd4f9a7623cc106d5e, my changes to fix the bug uncovered an issue where HistoryItemContent.value is sometimes nil, which caused a crash with images copied on another iCloud device with Universal Clipboard when Maccy is restarted. Without this PR, you can also observe this crash by trying to copy the affected image item (which appears blank after restarting Maccy). I thus had to remove the force unwrap.

Can you provide the exact steps to reproduce the issue? I tried copying a random image on my iPhone and ii unwrapped properly in Maccy.

kthchew commented 7 months ago

Can you provide the exact steps to reproduce the issue? I tried copying a random image on my iPhone and ii unwrapped properly in Maccy.

On Maccy 0.29.4:

  1. Copy some image on the iPhone.
  2. Check Maccy to see that it appears in the clipboard history (it works correctly at this point)
  3. Copy something else on the phone (like text for example). I did not have to do this originally (maybe a wait period will also trigger the crash), but it seems like this crash doesn't happen 100% of the time unless you do.
  4. Quit Maccy
  5. Reopen Maccy
  6. Check the history again, the entry that used to have the image is blank. Clicking it causes a crash.
p0deje commented 7 months ago

I am not in front of a computer, but I think the code below would replace newlines with special symbol. If that’s the case, you can just use \n. On Jan 27, 2024, at 16:00, Kenneth Chew @.***> wrote: @kthchew commented on this pull request.

In Maccy/History/HistoryItem.swift:

@@ -183,8 +181,10 @@ class HistoryItem: NSManagedObject { return title }

Should it appear as a literal newline in the history, or as a "⏎" like other newlines when copying text?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

p0deje commented 7 months ago

I still couldn't reproduce the crash, but the changes make storage access safer in any event. Thank you!

p0deje commented 7 months ago

Not sure why exactly, but looks like this also introduced a bug where copied formatted text could be duplicated. For example, when I copy something from Safari and then select it in history, it's pasted in 3 copies. @kthchew Would you like to try and implement a fix?

kthchew commented 7 months ago

It seems like this is happening due to the change from setData to writeObjects to write to the pasteboard. But for some reason, the old method of using setData meant that only the first file was copied if selecting it from the history. I'll make a new PR that only uses writeObjects for files.