Add bookmarks

[savente93]

Hiya, folks! I see folks talking about wanting a Harpoon like feature in Helix a lot. While I haven’t used vim (and by extension it) extensively, I definitely understand its raison d’être. I think it would fit in helix nicely. The feature seems relatively simple, so I figured aw hell, why not give it a whirl? This PR would fix #5167.

However, given that helix is a fairly large code base (by my standards at least), and this would be my first contribution. So I figured it would be an idea to communicate a bit before investing tons of time in it. Hence this draft PR in which I would like to get some feedback and answers on a few questions I have:

1. would maintainers be interested/open to this PR?

2. The current patch set includes a bare minimum skeleton for how I’d plan to implement this feature. Is this a reasonable way to go, or should it be structured differently?

3. To any harpoon enjoyers stumbling across this PR, have I missed any core features? I’m away it has fancy stuff like per branch marks, but so far I only want to get the basic functionality in. If this gets merged I’d be happy to work on those too.

4. The one bit I’m not sure about right now is where to store the MarkList. Currently, I put it next to the JumpList but since it doesn’t make much sense to give each view its own MarkList this is probably not a great place. I just couldn’t find another better place to put it, so if anyone knows a good home for it, please do share.

5. Questions and comments are welcome of course :)

[archseer]

I have no clue what Harpoon is but it seems like the marks feature with a couple of nice UX bindings on top. I'd build on @the-mikedavis's marks PR #3720 though it's likely that the snippet PR also contains a newer implementation. Marks are remapped through text edits so they stay relevant across code changes.

[savente93]

that was quick! Thanks for the reply.

FYI harpoon is a popular vim plugin: https://github.com/ThePrimeagen/harpoon the TL;DR is providing manual bookmarks that you can switch to from anywhere.

In any case, thanks for linking the PR, I'll take a look at that. I'll take your comment as meaning you'd be open to the PR so I'll start working on it.

[pascalkuthe]

whhile conceptully similar I think harpoon is different in that the locations can not be stored in registers. I would really like to see marks / #3720 revived. I think putting marks into registers is a really useful feature.

Particularly I would like to see the ability to compute union/difference/intersect of the current selection with a mark (register). That unlocks some really useful multicursor usecases.

I think the mapping trough changes is comparatively the easiest problem. Much of the mapping complexity in the snippet PR comes from some snippet-specific complications. Marks would just be a normal selection that could be mapped the exact same way we map the "normal" selection.

I think we definitely want a marks like feature like in #3720 but I think we should primarily take inspiration from kakoune and not a vim plugin.

[savente93]

I'm happy to take inspiration from anywhere, my aim with this PR would not be to emulate the behavior of harpoon (nor Kakuone for that matter) 1:1, harpoon was just the initial point of inspiration. the DoD as I imagine it is basically:

1. There is a list (which I will call mark list) in which I can store locations, I can add and remove items from this list with a command/keystroke
2. I can jump to any location saved in the mark list with a command/keystroke
3. The locations refer to ranges not specific line numbers so the mark refers to the same code even across additions and removals in the same file, If the code itself is removed, the mark should also be removed.
4. when a buffer is closed remove all marks associated with that buffer from the list (more because of implementation details than user reasons)

Particularly I would like to see the ability to compute union/difference/intersect of the current selection with a mark (register). That unlocks some really useful multicursor usecases.

At least initially I want to keep the scope of this small, so for me this would be out of scope for this PR. If this PR gets merged, and I still have time I'd be open to working on that.

P.S. I'm fine with using registers for this if you think that is better, I don't have a strong opinion on that.

[pascalkuthe]

yeah you don't have to implement the union/differenc/intersect operations but the design shouldn't block the future possibility of adding that.

I think the kakoune approach of using registers is both very nice ux and is a particularly good fit for these kinds of features.

[savente93]

I've implemented the first bit, which is to save the current (primary) selection into a register. For this feature I think it makes sense to just concern the primary selection. I was having trouble reading from the registers though, because I get the registers from cx.editor.registers but registers.read also requires a reference to the editor so I'm running into borrow checker issues. I'll try and figure that out next time I work on it.

[pascalkuthe]

Multicursor are central to helix so it should not just be the primary selection (kakoune also saves all selections and supproting multi cursor is why union/intersect/difference are so useful). That is something I would see as a requirement for any prototype. If you look at Mikes old mark PR that was linked that already supported that.

[hakan-demirli]

Harpoon solves two problems as explained in its README.
1- Navigating small set of files.
2- Execute some project specific commands/actions such as switching tmux panes or running builds.

A hypotetical harpoon mark list:

1- src/rust/main.rs:172:37
2- src/rust/test.rs:81:38
3- $ cargo build
4- $ tmux last-window

By pressing a single key you can switch between terminal and helix, navigate 2 different files and instantly launch builds. Harpoon also keeps this list per working directory. So, you don't have to add everything to the list again.

Marks only address the first problem. So, the issue #5167 should not be closed if the scope of this PR is only limited to file navigation.

Obviously any tool specific code should not be in the repo since that would be a maintenance nightmare. So, there should be a way for users to add their own commands/scripts to the mark list. In harpoon vim plugin you can view/edit the mark list in a floating window. You can reorder the list or delete items from the list by just editing the file in the floating window.

[savente93]

Multicursor are central to helix so it should not just be the primary selection (kakoune also saves all selections and supproting multi cursor is why union/intersect/difference are so useful). That is something I would see as a requirement for any prototype. If you look at Mikes old mark PR that was linked that already supported that.

Fair enough, taking multi cursor into account is not much more work I think. I'll give that a try as well.

[savente93]

The marks now indeed take all selection ranges into account, so that is sorted. I'll do some cleanup and write a few integration tests for it later, as the code is now functional but not super clean. Aside from that there are a few things left to be sorted out for now:

1. How to update these ranges when the document changes. Because I had to serialize them as strings to put them in the registers, they don't have any real binding to the document, so they will be have to updated when the document changes. The problem is also that unless we dedicate special registers for this (which I assume we don't want to do), well have to check every register at every update which seems sub optimal. No idea yet how I'll go about this, but I'll see what I can do.
2. What key (default) keybindings do we want for this? Currently I have 1 for register_mark using either the selected register or ^ as a default and 2 for goto_mark for debugging (same register behavior), but I think we should find some nicer defaults, I'm not really sure what keys are still open, so I'm happy to take suggestions here.
3. Should this be a static or typable command? Right now it's a static which just uses the selected register command, which is fine I guess, but I'd like it to be able to take arguments. This is an idea I had while debugging but that would enable a user to target a specific register in one keystroke. This aligns much closer with how I'd like to use the feature, which would be to set the mark in register 1 with SHIFT+1 and goto that mark with 1 (or any readable and writable register for that matter). To do this the toml could look something like:

[keys.normal]
"!" = [":register_mark", "1"]
1 = [":goto_mark", "1"]

The typable command can just take the current selected register if none is provided so I'm really leaning towards a typeable command unless someone has objections. I think this still enables the Kakuone kind of marks in the previous PR but also allow for the workflow that I would like to use them for.

Anyone got thoughts?

[savente93]

Okay I've made the commands typable and done some manual testing (the integration tests and cleanup are still to follow) and I think it works really really well. That also solves the point about key bindings, since people can just type it, or bind it to whatever keystroke they want so it doesn't need a default key binding I think, unless someone requests it. That only leaves the problem of how to update the ranges. I could see if I can do the "check every register" thing I described before, but I'm hoping someone more familiar with the code base has a better idea. TBC

[savente93]

Actually, I think I might have stumbled on the solution for the "which register to update" by just applying any needed updates lazily during goto_mark. (brain is tired so not 100% sure). In any case, the selections now reflect changes made between the registering and the goto (I'm sorry Dijkstra), so that's very exciting to me. I think I'm done for today, but if I'm correct, this PR only needs a little polish and then it should be ready for review :D

[savente93]

I think we're ready for review :D Not sure what the MO is but I'll just leave it like this to avoid necessarily pinging people. Happy reviewing!

[savente93]

@pascalkuthe Anything I can do to help get this reviewed?

[dj8yfo]

@savente93 will this be a per-helix-session marks persistence or persistence across helix sessions?
Will 2 intances of helix be able to share saved marks?
I think any kind of implementation of this feature will be absolutely great, just asking out of curiosity/impatience.

[savente93]

@savente93 will this be a per-helix-session marks persistence or persistence across helix sessions? Will 2 intances of helix be able to share saved marks? I think any kind of implementation of this feature will be absolutely great, just asking out of curiosity/impatience.

The information is stored in the registers, so technically the answer is "whatever is the case for register contents". That being said, persistent state (aside from what you have in your config) is a much anticipated features that's still being worked on elsewhere so for now the answer is per-helix-session, though pending future developments I wouldn't rule it out. Once the infra is there (and this PR has been merged) I'd be open to looking into that if enough people are interested and the maintainers are open to it.

[TornaxO7]

Found a little typo but the rest looks fine IMO

[baldwindavid]

@savente93 Thanks for your work on this; I've been looking forward to this sort of feature. I'm seeing an issue in the following scenario:

• Mark registered, say, at the bottom of a file.
• Move to top of that file and attempt to go to that mark.
• The cursor is moved to the mark, but the view stays in the same place.

[savente93]

is this a bug you actually experienced? because when I test it on my own fork the view moves to the first cursor so I can't replicate the behavior you talk about. could you share either a more specific MWE or a screen recording?

[fnuttens]

@savente93 I reproduce the out-of-view cursor issue, see https://asciinema.org/a/STgXxVUSqk87aKWRKCVrf874Z

[baldwindavid]

is this a bug you actually experienced?

Yes, as @fnuttens showed, this is the same issue I see. I've yet to see a case where the view moves to the cursor either going to a mark in the same or a different file. Pressing a key such as z or j after going to the mark results in the view moving to the cursor.

[savente93]

Thanks for the report, I'll try and take a look at it when I have some time.

[savente93]

@fnuttens @baldwindavid Turns out there was just a function called ensure_cursor_in_view that I forgot to call. I think it should be fixed now (at least it seemed to on my end). Could you double check?

[fnuttens]

@savente93 LGTM 😃

[baldwindavid]

@savente93 Working for me as well. This is probably more of a general Helix question, but I'm wondering if it's possible to set keybindings to make this a little simpler operation. In vim registering a mark for, say, s, is as simple as m s. Here, we need to start typing :register-mark, tab to that command, enter the desired s mark, and then enter to save it.

In vim accessing that mark is just ` s whereas here it is the same :goto-mark, tab, enter mark, enter dance.

Are you aware if there is a way to remap keys to set my own register/goto keys similar to m (I know this one is taken) and `?

About the best I've come up with are some dedicated keybindings for using j, k, l, and ; as common marks...

[keys.normal."^"]
j = ":register-mark j"
k = ":register-mark k"
l = ":register-mark l"
";" = ":register-mark ;"

[keys.normal."$"]
j = ":goto-mark j"
k = ":goto-mark k"
l = ":goto-mark l"
";" = ":goto-mark ;"

I'd prefer to be able to set any sort of mark though.

Perhaps there's a way to set a keybinding to simulate the start of a command like :register-mark, but setting "^" = ":register-mark", as expected, is the same as typing :register-mark and then enter rather than allowing to select a character to register.

[savente93]

I like the idea, and I think it should be possible in theory. I think the code for actually selecting a register is pretty much what we'd need (

helix/helix-term/src/commands.rs

Line 5281 in 84fbadb

cx.editor.autoinfo = Some(Info::from_registers(&cx.editor.registers));

) however, this is adding a new sort of mode (kinda) so I'd like a sign off from a maintainer (or someone else with merge rights) before looking into this more seriously