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

Implement no-yank delete/change #1099

Merged
merged 1 commit into from
Nov 24, 2021
Merged

Conversation

ath3
Copy link
Contributor

@ath3 ath3 commented Nov 14, 2021

No description provided.

book/src/keymap.md Outdated Show resolved Hide resolved
Comment on lines 229 to 231
delete_selection_noyank, "Delete selection (dont yank)",
change_selection, "Change selection (delete and enter insert mode)",
change_selection_noyank, "Change selection (delete and enter insert mode, dont yank)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
delete_selection_noyank, "Delete selection (dont yank)",
change_selection, "Change selection (delete and enter insert mode)",
change_selection_noyank, "Change selection (delete and enter insert mode, dont yank)",
delete_selection_noyank, "Delete selection (don't yank)",
change_selection, "Change selection (delete and enter insert mode)",
change_selection_noyank, "Change selection (delete and enter insert mode, don't yank)",

@Omnikar
Copy link
Contributor

Omnikar commented Nov 14, 2021

Couldn't this have just been done by changing the reg parameter of delete_selection_impl to take an Option<&mut Register> and then adjusting the function accordingly?

@ath3
Copy link
Contributor Author

ath3 commented Nov 14, 2021

Yes, and thats how i started doing it, but it felt like change and delete share a lot of the same code handling register, so i moved it to one central place (delete_selection_impl).
The only thing that differs in delete/change is what happens after the register related stuff and the deletion of text is done, so this feels more logical to me.

Btw the test is failing because sometimes git clone related errors occur, like it happened now.

// first yank the selection
let values: Vec<String> = selection.fragments(text).map(Cow::into_owned).collect();
reg.write(values);
if with_reg {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make a "black hole" register '_'. Then the extra parameter can be removed, and this condition can be if cx.register == Some('_'). You can then explicitly set the register in the noyank versions

Comment on lines 229 to 233
delete_selection_noyank, "Delete selection (don't yank)",
change_selection, "Change selection (delete and enter insert mode)",
change_selection_noyank, "Change selection (delete and enter insert mode, don't yank)",
Copy link
Member

Choose a reason for hiding this comment

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

Let's instead use ", without yanking" for both of these.

https://github.com/mawww/kakoune/blob/master/src/normal.cc#L2236-L2239

@ath3 ath3 force-pushed the noyank-delete branch 2 times, most recently from dace94f to 4c6ffe9 Compare November 14, 2021 16:36
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

🎉

@archseer
Copy link
Member

Ah, looks like there's a conflict now, can you resolve it? Good to merge after that

@ath3
Copy link
Contributor Author

ath3 commented Nov 24, 2021

Done!

@archseer archseer merged commit 72f606e into helix-editor:master Nov 24, 2021
@sudormrfbin
Copy link
Member

(The black hole register needs to be added to https://docs.helix-editor.com/usage.html#special-registers)

@ath3
Copy link
Contributor Author

ath3 commented Nov 24, 2021

Black hole register is a special case for noyank versions of change and delete, so outside this they are still usable.
Should i change them to behave globally as a black hole?

@ath3
Copy link
Contributor Author

ath3 commented Nov 24, 2021

Maybe that would be a good idea, because this might be confusing for users, a register behaving differently for only some functions.

@sudormrfbin
Copy link
Member

Yeah giving _ the role of the black hole register globally and explicitly would be good.

@pickfire
Copy link
Contributor

I wonder why not "_d?

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.

None yet

5 participants