feat(ui): sticky context #3944

matoous · 2022-09-22T16:27:54Z

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.

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

The desired behavior should resemble this:

or this

Fixes: #396

CBenoit

Thank you!

Maybe it's better to wait for an additional maintainer before putting additional work, because I'm unsure whether everyone agrees on including this.

That being said, here is my initial feedback =)

EDIT: regarding the naming of the feature, I like "sticky context". 👍

book/src/configuration.md

CBenoit · 2022-09-22T18:34:20Z

helix-view/src/editor.rs

+    /// Display context of current cursor line if it is outside the view.
+    pub context: bool,


Maybe move to the editor::Config struct and go with a new category: sticky_context: StickyContext.

enable: to enable sticky context feature (default: false)

min_window_height: disable sticky context when the window is too small (default: ?)

Moved to editor::Config but left it as boolean config option for now. I am not sure if min_window_height is necessary as the context usually spans only a few lines. What do you thing?

I’m just thinking this can be useful when the editing some file in a small window. What do you think?

Maybe a keybind to toggle it? That way it can be decided per case.

helix-term/src/ui/editor.rs

SoraTenshi · 2022-10-08T13:40:51Z

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?

matoous · 2022-10-08T20:32:15Z

@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.

matoous · 2022-10-12T18:47:33Z

@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?

sudormrfbin · 2022-10-12T19:08:14Z

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.)

SoraTenshi · 2022-10-12T20:14:04Z

@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

CBenoit · 2022-10-12T20:21:15Z

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 =)

matoous · 2022-10-12T20:27:51Z

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

SoraTenshi · 2022-10-12T21:38:37Z

@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:

SoraTenshi · 2022-10-12T22:11:54Z

also @matoous the PR is here: matoous#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.

sudormrfbin · 2022-10-13T01:43:41Z

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

SoraTenshi · 2022-10-13T08:20:13Z

@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.

SoraTenshi · 2022-10-16T20:54:30Z

helix-term/src/ui/editor.rs

+
+            let mut line_numbers = Vec::new();
+            for line_num in context {
+                if line_num >= view.offset.row {


Suggested change

if line_num >= view.offset.row {

if line_num >= view.offset.row || text.byte_to_line(cursor_byte) == line_num {

this is required because the sticky context will overshadow the cursor if you're on the same line with the cursor.

Have you tried with the latest changes? I ammended it to the latest commit but for me it works just with line_num >= view.offset.row, previously it was line_num > view.offset.row.

Have you tried with the latest changes? I ammended it to the latest commit but for me it works just with line_num >= view.offset.row, previously it was line_num > view.offset.row.

actually i haven't. time to try it out when i have time :)

Update: yup seems good

CBenoit

This is looking pretty good to me code-wise!

@archseer I think this change is not too complex and can be part of helix’s built-ins. Are you okay with me merging this feature once last nits are addressed if any?

matoous · 2022-10-17T21:35:11Z

@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.

SoraTenshi · 2022-10-17T21:56:28Z

I also have a proposal if it's:

feasible
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..

CBenoit · 2022-10-18T13:34:20Z

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

For reference: #3944 (comment)

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

helix-term/src/ui/editor.rs

the-mikedavis · 2022-10-19T01:44:56Z

helix-term/src/ui/editor.rs

+
+            let mut parent = tree
+                .root_node()
+                .descendant_for_byte_range(cursor_byte, cursor_byte)


I think the sticky context lines should depend on the top of the viewport instead of the cursor location

That would complicate it a little bit as what can happen is that for example if the context would contain 3 nodes the 3 lines that we would display would cover more than just the top line of the viewport.

SoraTenshi · 2022-11-02T12:15:25Z

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 :)

goyalyashpal · 2022-12-26T17:06:20Z

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 Change primary selection color... #5315)

SoraTenshi · 2023-01-16T15:21:09Z

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.

SoraTenshi · 2023-02-26T23:47:50Z

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
Use top viewport instead of cursor position
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

matoous · 2023-02-27T08:01:53Z

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

SoraTenshi · 2023-02-27T09:38:29Z

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

matoous mentioned this pull request Sep 22, 2022

Show the context of the currently visible buffer contents à la context.vim #396

Open

CBenoit self-requested a review September 22, 2022 16:45

matoous force-pushed the md/lsp-context branch from 88a32c5 to f2e6d7e Compare September 22, 2022 17:18

CBenoit added A-tree-sitter Area: Tree-sitter A-gui Area: Helix gui improvements C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 22, 2022

CBenoit reviewed Sep 22, 2022

View reviewed changes

CBenoit added the S-needs-discussion Status: Needs discussion or design. label Sep 22, 2022

matoous changed the title ~~[WIP] feat(ui): lsp context~~ [WIP] feat(ui): sticky context Sep 22, 2022

matoous force-pushed the md/lsp-context branch 2 times, most recently from 440c940 to 091e327 Compare October 12, 2022 07:24

matoous changed the title ~~[WIP] feat(ui): sticky context~~ feat(ui): sticky context Oct 12, 2022

sudormrfbin marked this pull request as ready for review October 12, 2022 19:09

matoous force-pushed the md/lsp-context branch from 807e3c6 to f7d20c2 Compare October 12, 2022 20:10

matoous force-pushed the md/lsp-context branch from f7d20c2 to 5f95619 Compare October 12, 2022 20:14

matoous force-pushed the md/lsp-context branch 3 times, most recently from 3614b69 to 17271a3 Compare October 16, 2022 09:37

SoraTenshi reviewed Oct 16, 2022

View reviewed changes

CBenoit reviewed Oct 17, 2022

View reviewed changes

the-mikedavis reviewed Oct 19, 2022

View reviewed changes

matoous force-pushed the md/lsp-context branch from 17271a3 to f76a789 Compare October 19, 2022 09:53

matoous and others added 5 commits December 10, 2022 15:35

feat(ui): lsp context

21297ee

configurable nodes, remove separator

0c1caa3

update config

c677474

Sticky context shows line number

41ae90f

apply suggestions

60ac894

matoous force-pushed the md/lsp-context branch from f76a789 to 60ac894 Compare December 10, 2022 14:43

SoraTenshi mentioned this pull request Feb 27, 2023

Feat: Sticky Context #6118

Draft

38 tasks

matoous closed this Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): sticky context #3944

feat(ui): sticky context #3944

matoous commented Sep 22, 2022 •

edited by CBenoit

Loading

CBenoit left a comment •

edited

Loading

CBenoit Sep 22, 2022

matoous Oct 12, 2022

CBenoit Oct 12, 2022

sudormrfbin Oct 13, 2022

SoraTenshi commented Oct 8, 2022

matoous commented Oct 8, 2022

matoous commented Oct 12, 2022

sudormrfbin commented Oct 12, 2022 •

edited

Loading

SoraTenshi commented Oct 12, 2022

CBenoit commented Oct 12, 2022

matoous commented Oct 12, 2022 •

edited

Loading

SoraTenshi commented Oct 12, 2022 •

edited

Loading

SoraTenshi commented Oct 12, 2022

sudormrfbin commented Oct 13, 2022

SoraTenshi commented Oct 13, 2022 •

edited

Loading

SoraTenshi Oct 16, 2022

matoous Oct 17, 2022 •

edited

Loading

SoraTenshi Oct 17, 2022

SoraTenshi Oct 17, 2022

CBenoit left a comment

matoous commented Oct 17, 2022

SoraTenshi commented Oct 17, 2022

CBenoit commented Oct 18, 2022 •

edited

Loading

the-mikedavis Oct 19, 2022

matoous Oct 19, 2022

SoraTenshi commented Nov 2, 2022 •

edited

Loading

goyalyashpal commented Dec 26, 2022 •

edited

Loading

SoraTenshi commented Jan 16, 2023

SoraTenshi commented Feb 26, 2023 •

edited

Loading

matoous commented Feb 27, 2023

SoraTenshi commented Feb 27, 2023

		/// Display context of current cursor line if it is outside the view.
		pub context: bool,

	if line_num >= view.offset.row {
	if line_num >= view.offset.row \|\| text.byte_to_line(cursor_byte) == line_num {

feat(ui): sticky context #3944

feat(ui): sticky context #3944

Conversation

matoous commented Sep 22, 2022 • edited by CBenoit Loading

CBenoit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoraTenshi commented Oct 8, 2022

matoous commented Oct 8, 2022

matoous commented Oct 12, 2022

sudormrfbin commented Oct 12, 2022 • edited Loading

SoraTenshi commented Oct 12, 2022

CBenoit commented Oct 12, 2022

matoous commented Oct 12, 2022 • edited Loading

SoraTenshi commented Oct 12, 2022 • edited Loading

SoraTenshi commented Oct 12, 2022

sudormrfbin commented Oct 13, 2022

SoraTenshi commented Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

matoous Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CBenoit left a comment

Choose a reason for hiding this comment

matoous commented Oct 17, 2022

SoraTenshi commented Oct 17, 2022

CBenoit commented Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoraTenshi commented Nov 2, 2022 • edited Loading

goyalyashpal commented Dec 26, 2022 • edited Loading

SoraTenshi commented Jan 16, 2023

SoraTenshi commented Feb 26, 2023 • edited Loading

matoous commented Feb 27, 2023

SoraTenshi commented Feb 27, 2023

matoous commented Sep 22, 2022 •

edited by CBenoit

Loading

CBenoit left a comment •

edited

Loading

sudormrfbin commented Oct 12, 2022 •

edited

Loading

matoous commented Oct 12, 2022 •

edited

Loading

SoraTenshi commented Oct 12, 2022 •

edited

Loading

SoraTenshi commented Oct 13, 2022 •

edited

Loading

matoous Oct 17, 2022 •

edited

Loading

CBenoit commented Oct 18, 2022 •

edited

Loading

SoraTenshi commented Nov 2, 2022 •

edited

Loading

goyalyashpal commented Dec 26, 2022 •

edited

Loading

SoraTenshi commented Feb 26, 2023 •

edited

Loading