helix

helix copied to clipboard

Note: This PR is ~rough~ proof of concept. I am completely fine if the final decision is not to include this in the core. (although I believe it fits well with the rest of the tree-sitter support and should be part of the core).

Add option to render the context of current line if the context is outside the view. Parent Tree-sitter node(s) outside the view are displayed at the top of the view providing additional (hopefully helpful) context for the current cursor line. The nodes are filtered based on language-specific configuration to strike the right balance between helpful and too noisy.

Inspiration taken from context.vim and a treesitter port of context.vim - https://github.com/romgrk/nvim-treesitter-context.

• [x] render only context outside the view
• [x] per-language configurable tree-sitter nodes to display
• [x] proper line numbers - maybe in a separate PR

The desired behavior should resemble this:

or this

Fixes: https://github.com/helix-editor/helix/issues/396

I also want to throw in my tiny opinion i have:

the --- context is imo unnecessary, it's already signaled that the context is not the code itself by the changed background color.

Also this seems to be a bit untouched for a while i guess? Is some help needed to further implement this?

@s0LA1337 I agree, that was just for testing. No help needed, I just need to find some time 😅 sorry about that; or feel free to pick it up if you feel like.

@s0LA1337 I had some time and moved the PR a bit. Feel free to test it out locally and I would be happy for any further feedback.

Otherwise, I have a few open questions:

• How do we want to handle line numbers? Looking at the neovim plugins linked above for inspiration: they don't bother with line numbers and leave the line numbers as they are (e.g. of the lines covered by the context). Do we want to instead show line number of the line that is part of the context? What is preferrable?
• https://github.com/wellle/context.vim can do some additional fancy stuff - such as showing other case statements in a switch (for rust that would be for example other arms in match statement). Is that something we want to pursuade or we keep it simple for now?
• How do we want to go about the coloring? For now I use ui.cursorline.primary. Do we want dedicated config? Or re-use some highlighting option?
• Last but not least, could someone asses or help me asses what the performance implications of the implementation are?

Really cool addition! I personally use it a lot in my neovim setup. As for the comments:

• How do we want to handle line numbers? Looking at the neovim plugins linked above for inspiration: they don't bother with line numbers and leave the line numbers as they are (e.g. of the lines covered by the context). Do we want to instead show line number of the line that is part of the context? What is preferrable?

It would be cool to show the line numbers of the overlayed context lines instead of the ones under them, but I'd say that's best left to a separate PR.

• wellle/context.vim can do some additional fancy stuff - such as showing other case statements in a switch (for rust that would be for example other arms in match statement). Is that something we want to pursuade or we keep it simple for now?

I suppose cases like that would require writing actual tree-sitter queries instead of providing a simple list of scope names, and it does provide more granularity, so I suppose it would be nice to have. But I like how simple the current PR is and how it delivers most of the functionality already, so I would shelve that for a separate PR again :)

• How do we want to go about the coloring? For now I use ui.cursorline.primary. Do we want dedicated config? Or re-use some highlighting option?

We definitely need separate theme keys, and it can manually fallback to another scope like ui.cursorline if needed.

Last but not least, could someone asses or help me asses what the performance implications of the implementation are?

I'm not sure how expensive it is to traverse up the tree checking every parent, so someone with more experience in that area would have to give their input. The traversal is done on every render, so it might be costly. Maybe a cache could help? But only after checking actual perf numbers. Premature optimization is indeed the root of all evil :)

---

(Also removing the draft status and marking ready for review since it's no longer WIP in the title.)

@s0LA1337 I had some time and moved the PR a bit. Feel free to test it out locally and I would be happy for any further feedback.

Otherwise, I have a few open questions:

• How do we want to handle line numbers? Looking at the neovim plugins linked above for inspiration: they don't bother with line numbers and leave the line numbers as they are (e.g. of the lines covered by the context). Do we want to instead show line number of the line that is part of the context? What is preferrable?
• wellle/context.vim can do some additional fancy stuff - such as showing other case statements in a switch (for rust that would be for example other arms in match statement). Is that something we want to pursuade or we keep it simple for now?
• How do we want to go about the coloring? For now I use ui.cursorline.primary. Do we want dedicated config? Or re-use some highlighting option?
• Last but not least, could someone asses or help me asses what the performance implications of the implementation are?

i actually opened a pr for your branch to include line numbers :) Maybe i should resolve the conflicts :D

This is looking pretty good!

Agreed with @sudormrfbin.

* Last but not least, could someone asses or help me asses what the performance implications of the implementation are?

Unless you had performance issues when testing in a reasonable sized file, I wouldn’t dig that just yet. The feature is optional and can be disabled at any time if it is an issue. Worst case, we can optimize that in a separate PR =)

@s0LA1337 oh please go ahead with the PR, would love to take a look. And sorry, I amended a few more minor changes.

@sudormrfbin:

It would be cool to show the line numbers of the overlayed context lines instead of the ones under them, but I'd say that's best left to a separate PR.

since i have literally worked on that already - i am not sure what you mean with that.

What i essentially did is: replacing the current line nrs in the gutter with the absolute ones (next to the sticky context) of the sticky context. pictures:

Relative line numbers:

Absolute line numbers:

also @matoous the PR is here: https://github.com/matoous/helix/pull/2

waiting for your approval, as when you merge it (it's on your fork) it will be part of this PR.

unless anyone has against it already being in this pr.

@s0LA1337 we're on the same page about the line number display, your implementation was exactly how i imagined it to be.

@s0LA1337 we're on the same page about the line number display, your implementation was exactly how i imagined it to be.

~~pr is open to fix the linting issue~~ Fixed.

@CBenoit (and maybe @archseer for further input): what I don't like right now is the way the context lines are passed from the render_sticky_context to render_gutter. Maybe you have better/cleaner idea of how to do it? Or maybe it's fine. Either way, I would keep the config simple bool for now and if this gets green light I would add configuration for more languages. There are some ideas for improvements that I would prefer to address separately.

I also have a proposal if it's:

1. feasible
2. not too much additional work

Couldn't we capture via tree-sitter all the arguments and have them stick, too? I am not too sure whether this might end up being too complex. Maybe someone which is more knowledgeable might be able to answer this..

Either way, I would keep the config simple bool for now

For reference: https://github.com/helix-editor/helix/pull/3944#discussion_r977971333

My problem with this is that it will then be a breaking change to modify in order to add more options. But then, maybe we don’t want additional configuration and the additional keybinding only as suggested by @sudormrfbin is alright (I would like both) Not my hill to die on though

I actually have noticed a small bug with the line numbers that i introduced. => In insert mode, when the linenumbers are set to relative, it changes from the absolute linenr to relatives, basically my logic inverts the linenumbers. I might want to fix that before the PR gets merged.

other than that: scopes on the first line seems to be sometimes a bit... wrong? consider such a cpp file:

/**
  * i am a comment
  */
...
class Abc {
...
};

The comment will still be put on the sticky context, even though i completely left the scope.

@matoous i opened up a PR for your branch :)

hi! will there be option to choose the style between: separator (as shown in the context.vim's screen record), colour, both?

The separator line may say: Scope Context being the most descriptive term for this

The separator line will be extremely crucial choice for the default signature purple theme:

• as we don't have very many color choices remaining for the background in that theme
• so, having one more background color being stolen away by this sticky context color will make matters worst
• We are already struggling for finding a good color for primary vs secondary selection, (see https://github.com/helix-editor/helix/pull/5315)

Hi, i also just discovered that there is still a bug. When the sticky context is overflowing, helix crashes.

An example can be found in this file: https://github.com/ziglang/zig/blob/master/src/main.zig going past 1030 in this file will cause the following issue:

thread 'main' panicked at 'index out of bounds: the len is 4352 but the index is 4359', helix-tui/src/buffer.rs:513:33
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

stack trace:

thread 'main' panicked at 'index out of bounds: the len is 4352 but the index is 4359', helix-tui/src/buffer.rs:513:33
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_bounds_check
   3: helix_tui::buffer::Buffer::clear_with
   4: helix_term::ui::editor::EditorView::render_sticky_context
   5: helix_term::ui::editor::EditorView::render_view
   6: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render
   7: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
   8: helix_term::application::Application::run::{{closure}}
   9: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
  10: tokio::runtime::park::CachedParkThread::block_on
  11: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
  12: hx::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I took the time and integrated this pr's (partial) functionality to the new text api. What currently differs from this pr is:

• [ ] Line Numbers in Gutter
• [ ] Cursor draws over the sticky context
• [x] Use top viewport instead of cursor position
• [x] Maximum amount of sticky contexts

@matoous should i open up a pr for you to look over it, or open an entirely new pr for it? You can find the changes here: https://github.com/SoraTenshi/helix/tree/sticky-context

@SoraTenshi please feel free to open new PR, I am afraid I don't have time at the moment to pick this up.

New pr created, i think this one can be closed.