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 clipboard paste with multiple selection #6889

Closed

Conversation

CcydtN
Copy link
Contributor

@CcydtN CcydtN commented Apr 26, 2023

Allow pasting content with multiple selection if two requirement is fulfilled:

  1. The content is yank from same instance.
  2. No other application overwrite the clipboard in that period. (unless the exact same string is copied or hash collision occurs)

Tested and working:

  • wl-paste+wl-copy
  • clipboard-win
  • termcode

Not tested:

  • pbpaste+pbcopy (mac)
  • xclip
  • xsel
  • win32yank.exe
  • termux-clipboard-set
  • tmux

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

The default hasher is not a cryptographically secure hash (like sha2) and collisions are quite likely. Usually that's account for by doing quadratic probing of all entries in the same hashmap bucket (basically comparing the contents) but here we don't need a hash here a simple text comparison should be just as fast.

@CcydtN CcydtN force-pushed the multi_selection_clipboard_paste branch from 6f64edc to f60af07 Compare April 28, 2023 19:22
@CcydtN CcydtN requested a review from pascalkuthe April 28, 2023 19:23
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

One minor point but otherwise I think this looks good

helix-view/src/editor.rs Show resolved Hide resolved
@pascalkuthe pascalkuthe added C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 4, 2023
@the-mikedavis
Copy link
Member

Ah I think I see where you were going now with #6820: was the idea to refactor the system and primary clipboards into special registers? (For context, _ is the only special register currently implemented but there are more we'd like to add.)

I think it might be ok to implement #2038 if it were a config option

// helix-view/src/editor.rs
// ...
pub struct Config {
  // ...
  #[serde(default)] // <- default this to `"`
  pub default_yank_register: char,
}
// ...

and then commands that yank could use that value if no register were provided through select_register (").

I played around with refactoring Register/Registers to implement the remaining special registers in a branch and it's a rather large refactor but I believe it's possible to implement the system & primary clipboards as special registers. The changes overlap with #5577 (but need to pass the editor rather than document and some other minor changes).

Do you have an opinion on the clipboards being special registers @archseer? I think they are not registers in Kakoune only because Kakoune interacts with clipboard tools only via shell piping.

@CcydtN
Copy link
Contributor Author

CcydtN commented May 5, 2023

Ah I think I see where you were going now with #6820: was the idea to refactor the system and primary clipboards into special registers?

yep, that's my final goal for that Pull request.

But I soon realized it is a huge refactoring and affecting other functionality, like search history. It require much more planning than I expected, so I stoped.

Comment on lines 3790 to 3792
let has_equal_contents = editor.last_clipboard[clipboard_type as usize]
.join(doc.line_ending.as_str())
.eq(&contents);
Copy link
Member

@pascalkuthe pascalkuthe May 5, 2023

Choose a reason for hiding this comment

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

Instead of joining last_clipboard here (which allocats and therefore causes some overhead) you could iterate over it and compare contents on the fly. That would roughly look like the following:

Suggested change
let has_equal_contents = editor.last_clipboard[clipboard_type as usize]
.join(doc.line_ending.as_str())
.eq(&contents);
let line_ending = doc.line_ending.as_str();
let mut pos = contents;
let mut last_clipboard = editor.last_clipboard[clipboard_type as usize].iter();
let mut has_equal_contents = true;
if let Some(first) = last_clipboard.next() {
if pos.starts_with(first) {
pos = &pos[first.len()..];
for selection in last_clipboard {
if pos.starts_with(line_ending) && pos[line_ending.len()..].starts_with(selection) {
pos = &pos[line_ending.len() + selection.len()..];
}else{
has_equal_contents = false;
break;
}
}
}
} else {
has_equal_contents = false
}

this is just something I came up quickly to illustrate what I mean so the implementation might be buggy/not compile/is not super idiomatic but I hope you get the idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some test with your idea and you are correct, it is much faster when there is large amount of data.
So, I just push a new commit to implement the idea. It uses a custom iterator for better readability, but the overall logic should remain the same.
Please take a look.

@CcydtN CcydtN force-pushed the multi_selection_clipboard_paste branch from 4e84917 to 39e5c48 Compare May 8, 2023 17:05
@CcydtN CcydtN requested a review from pascalkuthe May 13, 2023 18:26
@pascalkuthe
Copy link
Member

this was included in #6985 (which ahs been merged) so I am closing this as stale/duplicate of that. Thank you for your contributions, your idea here definitely helped improving our register handling!

@CcydtN CcydtN deleted the multi_selection_clipboard_paste branch July 31, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants