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 yank ring #10446

Closed
wants to merge 3 commits into from
Closed

Conversation

shaleh
Copy link
Contributor

@shaleh shaleh commented Apr 15, 2024

The yank ring is a thin wrapper on Rust's VecDeque. It is also purposefully as minimal as possible. Comments and changes are welcome and expected.

The following commands are implemented. Each is nearly exactly like the version without 'ring' in th name. The difference is each interacts with the yank ring and not a register. When a number is provided to a command it is used as the number of positions to rotate the ring instead of whatever the standard command would do.

  • `ring_yank
  • `ring_replace_with_yanked
  • `ring_paste_after
  • `ring_paste_before
  • `ring_delete_selection
  • `ring_change_selection
  • select_yank_ring

select_yank_ring will show the current yank ring with indexes. The user may select an index and that will become the new front of the ring.

2 new commands were added.

  • ring_next_paste
  • ring_previous_paste

These commands rotate the YankRing at least 1 and possibly more positions. Then they undo the previous paste and replace it with the new current element from the ring. These can be applied back and forth until the user is happy. The final position chosen is left as the current position so the next yank will go into the ring there. Each command only works when the previous command was a ring command.

The YankRing lives on the Editor object beside the Registers. However, to allow cycling I decided to track the last command and check if it was a ring action. This is implemented with tracking on the EditorView and the decision that yes, a paste happened is passed into the Context.

Where possible no previous yank or paste code was changed. This means some code was copied to implement the feature. This can be improved. Places where changes were made include:

  • the enum YankAction was updated to have a 'YankRing' as well as the previous 'Yank'.
  • replace_with_yanked_impl was split up into prep_replace_with_yanked and then the later half of the original replace_with_yank_impl remains.

It is possible to completely replace all of the yank and paste actions with the ring equivalents. I doubt people would want to do that because yanking and pasting out of registers is also super handy. So either the YankRing becomes their default and standard yank/paste are moved to some other set of keys or ring manipulation is moved to its own set of keys.

d = "delete_selection_noyank"
c = "change_selection_noyank"
A-d = "ring_delete_selection"
A-c = "ring_change_selection"
y = 'ring_yank'
R = 'ring_replace_with_yanked'
p = 'ring_paste_after'
P = 'ring_paste_before'
A-p = 'ring_next_paste'
C-p = 'ring_previous_paste'

Open questions:

  • Is the command tracking appropriate as an implementation solution and was it done correctly? Checking for commands starting with "ring_" feels more than a bit hackish but there was not a clear differentiation between commands to otherwise rely on. This was done because replacing the last paste requires knowing there was one and it felt like it would be weird to trigger a ring rotate on a previous paste when the user had moved past it and possibly entered new text or otherwise manipulated the document. Another option would be to remember the last ring paste until a mode change. In keeping with minimal solution to start the conversation I went for the easy solution.
  • What interface should be exposed? What should these new commands be called? How should users be recommended to use them?
  • The Ring is limited to 10 elements 0 - 9 because the Info panel cannot select more than a single character.
  • I chose to have the ring rotate actions also update the document. In theory these actions could manipulate the ring and only ring-paste-after/before or something like them would actually manipulate the text. The current behavior maps to my expectations but may not be anyone else's.
  • If the implementation is close to acceptable the duplicated code could likely bear re-evaluation.

The yank ring is a thin wrapper on Rust's VecDeque. It is also purposefully
as minimal as possible. Comments and changes are welcome and expected.

The following commands are implemented. Each is nearly exactly like
the version without 'ring' in th name. The difference is each interacts
with the yank ring and not a register. When a number is provided to a command
it is used as the number of positions to rotate the ring instead of whatever
the standard command would do.

 - ring_yank
 - ring_replace_with_yanked
 - ring_paste_after
 - ring_paste_before
 - ring_delete_selection
 - ring_change_selection
 - select_yank_ring

2 new commands were added.
 - ring_next_paste
 - ring_previous_paste

These commands rotate the YankRink at least 1 and possibly more positions. Then
they undo the previous paste and replace it with the new current element from
the ring. These can be applied back and forth until the user is happy. The final
position chosen is left as the current position so the next yank will go into
the ring there. Each command only works when the previous command was a ring
command.

The YankRing lives on the Editor object beside the Registers. However, to allow
cycling I decided to track the last command and check if it was a ring action.
This is implemented with tracking on the EditorView and the decision that yes,
a paste happened is passed into the Context.

Where possible no previous yank or paste code was changed. This means some code
was copied to implement the feature. This can be improved. Places where changes
were made include:
 - the enum YankAction was updated to have a 'YankRing' as well as the previous 'Yank'.
 - replace_with_yanked_impl was split up into prep_replace_with_yanked and then the
   later half of the original replace_with_yank_impl remains.

It is possible to completely replace all of the yank and paste actions with the
ring equivalents. I doubt people would want to do that because yanking and pasting
out of registers is also super handy. So either the YankRing becomes their default
and standard yank/paste are moved to some other set of keys or ring manipulation is
moved to its own set of keys.

d = "delete_selection_noyank"
c = "change_selection_noyank"
A-d = "ring_delete_selection"
A-c = "ring_change_selection"
y = 'ring_yank'
R = 'ring_replace_with_yanked'
p = 'ring_paste_after'
P = 'ring_paste_before'
A-p = 'ring_next_paste'
C-p = 'ring_previous_paste'

Open questions:
 - Is the command tracking appropriate as an implementation solution and was
   it done correctly? Checking for commands starting with "ring_" feels more
   than a bit hackish but there was not a clear differentiation between
   commands to otherwise rely on. This was done because replacing the last
   paste requires knowing there was one and it felt like it would be weird
   to trigger a ring rotate on a previous paste when the user had moved past
   it and possibly entered new text or otherwise manipulated the document.
   Another option would be to remember the last ring paste until a mode change.
   In keeping with minimal solution to start the converation I went for the
   easy solution.
 - What interface should be exposed? What should these new commands be called?
   How should users be recommended to use them?
 - The Ring is limited to 10 elements 0 - 9 because the Info panel cannot
   select more than a single character.
 - I chose to have the ring rotate actions also update the document. In theory
   these actions could manipulate the ring and only ring-paste-after/before or
   something like them would actually manipulate the text. The current behavior
   maps to my expectations but may not be anyone else's.
 - If the implementation is close to acceptable the duplicated code could likely
   bear re-evaluation.
@kirawi kirawi added the A-command Area: Commands label Apr 15, 2024
@pascalkuthe
Copy link
Member

I don't think this is the direction we want to go. @the-mikedavis discussed recently that we probably want to support something like this by simply appending any new yanks to each register and adding extra state to each register that would simply be an array of the len of each individual history entry.

I don't think we would add something that completely sidesteps the registers.

@shaleh
Copy link
Contributor Author

shaleh commented Apr 15, 2024

I am 100% ok with doing that. I kept this minimal to prove the implementation. There is also the space bloat of adding a Ring to each register. An option is to make the default yank register " special. But that would have been more invasive.

If we can agree on the logic here moving the interface to all registers or the yank register is work I can do. I was planning on living with this change for a bit to see how it works in practice.

@the-mikedavis
Copy link
Member

Default regular registers like " or / shouldn't be special-cased. I'm interested in yank history functionality but we need to make it mesh with registers rather than an opposing system like it is on HEAD or special cases. To handle space concerns we should have a constant limit of the history, either in terms of number of yanks to a register or total size of a register.

@shaleh
Copy link
Contributor Author

shaleh commented Apr 16, 2024

I call some of that out in the open questions.

The ring is currently limited to ten entries for other reasons.

Is the core of the implementation acceptable? If so I am happy to replace registers with rings. This implementation was purposefully done as new registers to keep the work small so we could evaluate the idea. MVE. I fully expected to iterate on it a few times.

@the-mikedavis
Copy link
Member

the-mikedavis commented Apr 16, 2024

I'm saying that we should be extending Register's capabilities rather than replacing registers. Rather than a new Ring type, we need some extra metadata in Register's inner field. We also shouldn't need to modify the existing yank/paste/replace commands. Plus I think we shouldn't try to replace the last paste - it's a lot of complexity for something I'm not convinced is that useful. I think it's not too cumbersome to undo a paste, use a new command that lets you select from a register's history, then paste again with the new value(s). I'm imagining something more like a clipboard manager rather than trying to emulate the emacs kill ring.

If I get a chance I will draft out my ideas and push up a branch.

@fgeller
Copy link

fgeller commented Apr 16, 2024

fwiw if i understand your suggestion, emacs' consult has something like you're describing (consult-yank-from-kill-ring): a command that lets you interactively select any entry from the kill-ring and paste it in place. would love such an addition.

@darshanCommits
Copy link

Can't we have a combined picker like a clipboard and with the #9647 it'll be easy to find and filter.

By default if it shows delete or change history it would be a lot of clutter and use the picker syntax to search amongst c/d.

(Now that I've articulated my thoughts, it seems a bit of plugin territory but still a nice feature regardless)

@the-mikedavis
Copy link
Member

Yeah I was imagining a menu or a smaller picker that you could use to select from history from a register. Selecting something from history would pull it to the front of the register so that the next paste or replacement would use that content.

@pascalkuthe also mentioned that what I was saying above about undoing, selecting other values from history and then pasting again wouldn't typically be necessary. Pasting in normal mode selects the pasted value so you could replace instead of undo and then paste.

@shaleh
Copy link
Contributor Author

shaleh commented Apr 17, 2024

Yep, after reading Mike's comments yesterday I was planning a picker. Give me a few days to iterate on this.

@shaleh
Copy link
Contributor Author

shaleh commented Apr 26, 2024

#10604 is a second go at this follow suggestions from this one.

@the-mikedavis
Copy link
Member

Closing in favor of #10604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants