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

Refactor and expand special registers #6985

Merged
merged 10 commits into from
Jul 31, 2023
Merged

Refactor and expand special registers #6985

merged 10 commits into from
Jul 31, 2023

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented May 6, 2023

This implements some special registers from Kakoune:

And adds special registers for the system and primary clipboards: * and + respectively. This also re-uses the trick from #6889 to store the selections yanked to clipboards so they can be re-used on paste if the clipboard hasn't changed.

The strategy here mirrors how Kakoune implements these special registers and should extend to the remaining special registers in Kakoune (1..9 for regex captures), but it's an unfortunately large refactor that adds complexity and allocations and needs updates for all register callsites. The overview is...

  • Register moves to helix-view
  • read functions now may return owned values. This is necessary because of the way some special registers produce their values. We use a Cow to prevent allocations in places where we don't need them.
  • read, first and last functions now take the Editor as a parameter. This is the main change that enables special registers and is what mirrors Kakoune.

Closes #5577
Closes #5242
Closes #5241

@the-mikedavis the-mikedavis added E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. labels May 6, 2023
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.

This looks great! Support for special register like this is something I have been missing. These can come kin quite handy when combined with other editing primitives. The code is very readable/well structured and seems like a good extensible solution. I left some minor comments

book/src/usage.md Outdated Show resolved Hide resolved
book/src/usage.md Outdated Show resolved Hide resolved
helix-view/src/register.rs Outdated Show resolved Hide resolved
helix-view/src/register.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels May 11, 2023
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2023
pascalkuthe
pascalkuthe previously approved these changes May 13, 2023
@edrex
Copy link

edrex commented May 22, 2023

#: index numbers for the current selections
.: contents of the current selections

Is the buffer save state exposed somehow? That would be useful.

Also, out of scope for this issue but is there any way to turn this into selection ranges?

@the-mikedavis
Copy link
Member Author

I can't imagine use-cases for either of these, could you expand on that in a separate discussion? This is specifically about the Kakoune registers & clipboard registers

@gibbz00
Copy link
Contributor

gibbz00 commented Jul 2, 2023

When and how is the # ever used? I found this line, but I can't find where it set...

let increase_by = if cx.register == Some('#') { sign } else { 0 };

@gibbz00
Copy link
Contributor

gibbz00 commented Jul 2, 2023

Is there a pipe register too?

Some('|'),

@gibbz00 gibbz00 mentioned this pull request Jul 2, 2023
@pascalkuthe
Copy link
Member

When and how is the # ever used? I found this line, but I can't find where it set...

let increase_by = if cx.register == Some('#') { sign } else { 0 };

that register was a special case for that command in the past this PR adds it as a general purpose register.

Is there a pipe register too?

Some('|'),

Is there a pipe register too?

Some('|'),

Not in this PR. I don't think that makes too much sense for us given that we don't have an interactive shell yet

@the-mikedavis
Copy link
Member Author

Yeah the existing line you linked is used for "#<C-a> or "#<C-x> so you can increment/decrement by selection number. With this PR you can use "#P to paste the selection numbers before each cursor.

@gibbz00
Copy link
Contributor

gibbz00 commented Jul 2, 2023

Huh, cool! Where # set? Got it.

@gibbz00 gibbz00 mentioned this pull request Jul 4, 2023
helix-view/src/register.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis marked this pull request as draft July 13, 2023 22:55
@the-mikedavis the-mikedavis marked this pull request as ready for review July 15, 2023 22:54
pascalkuthe
pascalkuthe previously approved these changes Jul 30, 2023
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.

I think this looks great! I have been daily driving this for a while and found no issues. I did another pass and this lgtm (although it has a couple of conflicts right now). This adds a bunch of (very) useful functionality while only adding 150LOC

archseer
archseer previously approved these changes Jul 30, 2023
the-mikedavis and others added 7 commits July 30, 2023 10:28
This sets up a new Registers type that will allow us to expand support
for special registers. (See the child commits.)

We start simple with the regular (`Vec<String>`) registers and the
simplest special register, the black hole. In the child commits we
will expand these match arms with more special registers.

The upcoming special registers will need a few things that aren't
possible with the current Registers type in helix-core:

* Access to the `Editor`. This is only necessary when reading from
  registers, so the `&Editor` parameter is only added to
  `Registers::read`.
* Returning owned values. Registers in helix-core returns references
  to the values backed by the `Vec<String>` but future special registers
  will need to return owned values. We refactor the return value of the
  read operations to give `Cow<str>`s and iterators over those.
* Returning a `Result` for write/push functions. This will be used by
  the clipboard special registers.
These come from Kakoune:

* '#' is the selection index register. It's read-only and produces the
  selection index numbers, 1-indexed.
* '.' is the selection contents register. It is also read-only and
  mirrors the contents of the current selections when read.

We switch the iterators returned from Selection's `fragments` and
`slices` methods to ExactSizeIterators because:

* The selection contents register can simply return the fragments
  iterator.
* ExactSizeIterator is already implemented for iterators over Vecs, so
  it's essentially free.
* The `len` method can be useful on its own.
This register also comes from Kakoune. It's read-only and produces the
current document's name, defaulting to the scratch buffer name
constant.

(Also see PR5577.)

Co-authored-by: Ivan Tham <[email protected]>
These special registers join and copy the values to the clipboards with
'*' corresponding to the system clipboard and '+' to the primary as
they are in Vim. This also uses the trick from PR6889 to save the values
in the register and re-use them without joining into one value when
pasting a value which was yanked and not changed.

These registers are not implemented in Kakoune but Kakoune also does
not have a built-in clipboard integration.

Co-authored-by: CcydtN <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
This is an unfortunately noisy change: we need to update virtually all
callsites that access the registers. For reads this means passing in the
Editor and for writes this means handling potential failure when we
can't write to a clipboard register.
These snippets use hardcoded registers but it can be useful to be able
to specify a register for these commands.
pascalkuthe
pascalkuthe previously approved these changes Jul 30, 2023
This fixes a discrepancy between regular registers which are used for
yanking multiple values (for example via `"ay`) and regular registers
that store a history of values (for example `"a*`).

Previously, the preview shown in `select_register`'s infobox would show
the oldest value in history. It's intuitive and useful to see the most
recent value pushed to the history though.

We cannot simply switch the preview line from `values.first()`
to `values.last()`: that would fix the preview for registers
used for history but break the preview for registers used to yank
multiple values. We could push to the beginning of the values with
`Registers::push` but this is wasteful from a performance perspective.
Instead we can have `Registers::read` return an iterator that
returns elements in the reverse order and reverse the values in
`Register::write`. This effectively means that `push` adds elements to
the beginning of the register's values. For the sake of the preview, we
can switch to `values.last()` and that is then correct for both usage-
styles. This also needs a change to call-sites that read the latest
history value to switch from `last` to `first`.
Since the clipboard provider now lives on the Registers type, we want
to eliminate it from the Editor. We can do that and clean up the
commands that interact with the clipboard by calling regular yank,
paste and replace impls on the clipboard special registers.

Eventually the clipboard commands could be removed once macro keybinding
is supported.
The clipboard special registers are able to retain multiple selections
and also join the value when copying it to the clipboard. So by default
we should yank regularly to the '*' and '+' registers. That will have
the same behavior for the clipboards but will allow pasting multiple
selections if the clipboard doesn't change between yanks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select_register incorrectly shows the oldest value in registers
5 participants