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

feat(filesystem): Implement shared clipboard #1594

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

miroshQa
Copy link

This pull request aims to solve #1301 issue by adding the shared_clipboard functionality

And it also probably solves these two:
#168
#1235

You can watch how it works here:

shared_clipboard_preview.mp4

Copy link
Contributor

@cseickel cseickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR,I can't wait to use it! There are a few things I think we can improve.

lua/neo-tree/sources/filesystem/lib/shared_clipboard.lua Outdated Show resolved Hide resolved
lua/neo-tree/sources/filesystem/lib/shared_clipboard.lua Outdated Show resolved Hide resolved
@miroshQa
Copy link
Author

miroshQa commented Nov 3, 2024

Fixed it!

@miroshQa miroshQa requested a review from cseickel November 3, 2024 12:33
log.error("Failed to save clipboard. JSON serialization error")
return
end
file:write(data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually meant to put the error handling n the file:write / file:flush code. That's where you are going to get frequent errors due to permissions, file locked, disk full, etc.


-- Using watch_folder because we haven't "watch_file" :)
fs_watch.watch_folder(clipboard_state_dir_path, function()
if not clipboard_file_change_triggered_by_cur_neovim_instance then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bad idea. You can't say for sure that a file event came from this instance just because you wrote a file recently. This is fertile ground for hard to recreate bugs.

Between this and the modified time based check I would rather use the first solution where you just process every write, even your own.

I think I have a better idea:

  1. When you convert the clipboard to json, save the json string as last_clipboard_saved
  2. When you read in a new changed file event, compare the contents of the changed file to the last_clipboard_saved
  3. If they are equal, nil out the last_clipboard_saved and ignore the event.

Copy link
Author

@miroshQa miroshQa Nov 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I can't understand why we can't be sure about this. Can you give me an example, please?

  1. The variable clipboard_file_change_triggered_by_cur_neovim_instance is set in the save_clipboard function after writing the clipboard to the file
  2. After that an event is immediately triggered in response to the file change.

Why can't we be sure that the write occurred in the current instance? We are not performing any other actions between these two steps, are we?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not have exclusive access to this folder. Another program could write a file in this folder for some reason.

For that matter, neo-tree itself might write a file here. It doesn't do that now, but someone else may do so in the future and they likely won't know that some other section of the code is naively reading every file written without checking the contents or even the file name.

Finally, I don't think you should be so confident about the order of events when you are dealing with asynchronous actions.

return clipboard
end

M._update_all_cilpboards = function(clipboard)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo:

Suggested change
M._update_all_cilpboards = function(clipboard)
M._update_all_clipboards = function(clipboard)

@cseickel
Copy link
Contributor

#1606

@redoxahmii
Copy link

any updates on this ?

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.

3 participants