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 register history and access using a picker. #10604

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

shaleh
Copy link
Contributor

@shaleh shaleh commented Apr 26, 2024

Well, technically an info panel.

All registers are now backed by a history aware datastructure. Pushing onto this datastructure will remember the most recent 16 elements. Why 16? Because the info panel selection uses a single letter so 0-F it is. Although I suspect it could be 36 and use all the alphabet. But that might be too big and if the user is heavily utilizing registers is a bunch of likely wasted space. Also, as a plus 'p' is available. More on that in a moment.

paste_before and paste_after are still there. But paste_picker_after and paste_picker_before are now the default. That could 100% flip. Setup this way made it easier to write tests.

All data sent to registers is now kept as each one stores up to the 16 element limit. An 'element' here means one or more strings.

The datastructure holding the register items is still a vector of strings. To assist with retrieving the correct data another queue keeps track of the lengths of each element pushed to the vector of contents. For example, to push 3 selections [a, b, c] each one is pushed independently and the length of "3" is saved.

As before, the contents are stored in reverse order. Reading happens from the tail.

How are registers used in Helix?

Users can push things into a register and retrieve. This is done as a string. So the contents would act like a one element vector.

Users can yank part of document. Each selection is captured as a string meaning the vector length would equal the number of selections.

Picker history for search, regexes, etc. is stashed into a register as single strings.

Macros are squashed into a string and stored.

None of that effectively changes with this PR. However, now that history exists all of the commands can act like the history of a picker. Show it, pick it. Right now, paste and macro replay use this capability through an info panel.

I added the rand crate and used it in my unit tests because I wanted to fuzz things a bit to ensure the code was not simply working because everything had 2 elements or 3 elements. This is what caused the Cargo.lock change.

Macro history picking
Screenshot 2024-04-26 at 10 26 20 AM
Paste history picking
Screenshot 2024-04-26 at 10 24 01 AM

Well, technically an info panel.

All registers are now backed by a history aware datastructure. Pushing
onto this datastructure will remember the most recent 16 elements.
Why 16? Because the info panel selection uses a single letter so 0-F
it is. Although I suspect it could be 36 and use all the alphabet. But
that might be too big and if the user is heavily utilizing registers is
a bunch of likely wasted space. Also, as a plus 'p' is available. More on
that in a moment.

paste_before and paste_after are still there. But paste_picker_after and
paste_picker_before are now the default. That could 100% flip. Setup this
way made it easier to write tests.

All data sent to registers is now kept as each one stores up to the 16
element limit. An 'element' here means one or more strings.

The datastructure holding the register items is still a vector of strings.
To assist with retrieving the correct data another queue keeps track of
the lengths of each element pushed to the vector of contents. For example,
to push 3 selections [a, b, c] each one is pushed independently and the
length of "3" is saved.

As before, the contents are stored in reverse order. Reading happens from
the tail.

How are registers used in Helix?

Users can push things into a register and retrieve. This is done as a
string. So the contents would act like a one element vector.

Users can yank part of document. Each selection is captured as a string
meaning the vector length would equal the number of selections.

Picker history for search, regexes, etc. is stashed into a register as
single strings.

Macros are squashed into a string and stored.

None of that effectively changes with this PR. However, now that history
exists all of the commands can act like the history of a picker. Show it,
pick it. Right now, paste and macro replay use this capability through
an info panel.
@shaleh shaleh mentioned this pull request Apr 26, 2024
@shaleh
Copy link
Contributor Author

shaleh commented Apr 26, 2024

And yes, annoyingly that screen shot is showing some form of off by one or similar weirdness. More testing is needed. I left the PR draft appropriately.
Edit: Or it was late and I was running an old build....

})
}

fn parse_keys_from_register_index(
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 needed to break this out because of a conflict between mutable and immutable borrows of Context and Editor.

@inifares23lab
Copy link

inifares23lab commented Apr 26, 2024

It would be nice to also have something like pop_<ignore/paste/other_operation>_item.

For context, at times I use some compound commands like the following:

[keys.normal]
C-j = ["extend_to_line_bounds", "delete_selection", "paste_after"]
C-k = ["extend_to_line_bounds", "delete_selection", "move_line_up", "paste_before"]

I often don't need the yanked text after this operations, so it would be nice if it didn't end up in the history, or even in the registers with the current implementation.

For clarity, if I can pop the item it means that it ended up in the history/register, but for example with the following keymaps it would be as if it didn't.

[keys.normal]
C-j = ["extend_to_line_bounds", "delete_selection", "pop_paste_after"]
C-k = ["extend_to_line_bounds", "delete_selection", "move_line_up", "pop_paste_before"]
# anoter case is that I need some yanked text that is not the last one and I don't want it there anymore"
"someotherkey" = "select_pop_paste_after"

The system clipboard history(or not) should be left as is and managed by the system I guess.

@shaleh
Copy link
Contributor Author

shaleh commented Apr 26, 2024

yep, I started this but did not go crazy. The registers support pop first and pop oldest. Exactly for this use case. If this PR direction is approved it would not be hard to add them. What I am imagining is less pop_paste and more:
[extend_to_line_bounds, delete_selection, paste_after, pop_register_contents]

@shaleh
Copy link
Contributor Author

shaleh commented Apr 26, 2024

Test failure was an oops when I moved my test to using concat. The quote for picking a register was confused with the string delimiting quote. Fixed.

@@ -65,4 +65,12 @@ impl Info {
infobox.width = 30; // copied content could be very long
infobox
}

pub fn from_selected_register(registers: &Registers, register: char) -> Self {
let title = "Collected bits";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an arbitrary place holder I chose. Open to better ideas. "Register contents" maybe?

@shaleh shaleh marked this pull request as ready for review April 26, 2024 17:28
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.

This is not really what I had in mind in #10446 (comment), I was thinking a true Picker rather than an info box. The issue I have with this alternative is that it adds an extra keystroke for some of the most commonly used commands. Reaching into the history of a register should be a less common task than pasting or replacing.

At a high level what I'm imagining is a clipboard manager. https://github.com/sentriz/cliphist?tab=readme-ov-file#video is a good example of the UX. Yanking, pasting and replacing should need no changes at all. We would introduce a new command that shows an infobox of registers to select and with an on_next_key callback we would open a picker that has the history for the entered register character. Choosing one of the items from the picker would 'select' that history item, deleting it from the history and yanking it. So the value is what's used if you then paste or replace.

I pushed my thoughts on the Register changes into a register-history branch. The changes should live almost entirely in the register module in helix-view. The specs of Registers::read, Registers::write and Registers::push shouldn't change.

As a sidenote, we can slightly improve the spec of Registers::write to be pub fn write<I: IntoIterator<Item = String>>(&mut self, values: I) -> Result<()>. That would accept the existing callsites that pass a Vec<String> and enable a few callers to avoid collecting into a Vec needlessly. But that change is not really related to the refactor here.

On top of that branch we still need a UI for seeing and selecting items from history. A Menu might be a good choice since we limit the number of items. Or we could bump the limit and use a Picker so that you can easily fuzzy find within the values. They can display the history for a register with the new Registers::history and Registers::select_history_entry functions when you choose an item from history.

@darshanCommits
Copy link

Aren't numbers a bit too far (qwerty)? Wouldn't that be inconvenient.

@shaleh
Copy link
Contributor Author

shaleh commented Apr 28, 2024

It is hex in my PR. 0 to 9, a to f. Not that hard really. Or press p again to paste the last yank. I also type on a split Ortho linear so no, it is not a reach. I prefer it over Ctrl N/P in a picker to be honest

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

4 participants