Feat: Sticky Context #6118
Feat: Sticky Context #6118
Conversation
SoraTenshi
force-pushed
the
sticky-context
branch
4 times, most recently
from
7ce93e9 to
f1d26a6
Compare
|
here is a preview of how it looks like at the moment.
|
|
i think in order to satisfy clippys complaint, i would have to use the Maybe someone else got an idea how to get the |
SoraTenshi
force-pushed
the
sticky-context
branch
from
56c857f to
2e6c02b
Compare
SoraTenshi
force-pushed
the
sticky-context
branch
from
acbc1a0 to
f22cbf9
Compare
Don't worry too much about it, this one lint is helpful most of the time but it can't account for the context of the code, ignoring it is often ok (I'm on my phone so I won't review right now, I can't tell for your use case yet) |
|
Note: the Neovim plugin this is based on is switching to queries for determining the nodes (nvim-treesitter/nvim-treesitter-context#198). We should do the same (similar to indents where we started with a toml config but now use proper queries) |
Do i understand it correctly, that you'd want seperate injection queries (tree-sitter) for the context? If so, i think i can do that. |
|
I would advocate not to depend on tree-sitter on this feature. There are still many languages that don't have TS. |
in order for this feature to work without tree-sitter, i would have to manually parse scopes, which are VERY inconsistent throughout languages, therefore tree-sitter is, imo, the only way of doing this kind of properly. You also could - of course - always create tree-sitter grammars as well as add languages to helix |
Helix is primarily a TS based editor and languages, I think supporting plaintext files for some basic editing operations (like pair matching) can make sense but not for features that require semantic information (as this one does). The right fix is to create a TS grammar for those languages instead of adding language specific code into helix. Creating a TS grammar is not that hard and most mainstream languages do have one |
|
OK. I see your point. Unfortunately, for people like DevOps or SysAdmins it still means opening lots of text files (scripts, configs etc.) in weird languages that will never have a TS. But, again, it's just my concern and wish. This feature is so needed that I would be happy if it works at least for the supported languages. |
|
Helix has grammars for markup and configuration languages too (like markdown). Any file that has syntax highlighting in helix has a TS grammar. We even have TS grammars for filetypes like |
|
I tested this pr and it shows a bit strange in golang. I also support the use of tree-sitter queries, nvim-treesitter-context/queries provides a lot of language , we can use it, other languages can be added later. |
What exactly looks strange? |
|
I have added the following configuration. I test with golang base library Some times it is displayed correctly and some times it is not displayed. |
ahh maybe i know why. I will remove that, as it's an old artifact. Thanks for the finding! :) |
|
When I have non-ascii in my code, the capture is incorrect. For example, the following insert method. test file: https://gist.github.com/erasin/a15c4f250beff8c9f7e4aaeae4a1128c 2023-03-10.13.52.18.movI get a panic when using the mouse to scroll end of file. The error was not found in master. thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Char index out of bounds: char index 4184, Rope/RopeSlice char length 4173', /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/ropey-1.6.0/src/slice.rs:379:41
stack backtrace:
0: 0x109be68c6 - std::backtrace_rs::backtrace::libunwind::trace::h310cbd77a7d2ae59
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
1: 0x109be68c6 - std::backtrace_rs::backtrace::trace_unsynchronized::h5768bae568840507
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
2: 0x109be68c6 - std::sys_common::backtrace::_print_fmt::hd104a205649a2ffb
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/sys_common/backtrace.rs:65:5
3: 0x109be68c6 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h521420ec33f3769d
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/sys_common/backtrace.rs:44:22
4: 0x109c08c6a - core::fmt::write::h694a0d7c23f57ada
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/fmt/mod.rs:1208:17
5: 0x109bdfb9c - std::io::Write::write_fmt::h1920a3973ad439e5
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/io/mod.rs:1682:15
6: 0x109be66aa - std::sys_common::backtrace::_print::h75582c4ed1a04abb
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/sys_common/backtrace.rs:47:5
7: 0x109be66aa - std::sys_common::backtrace::print::hef1aa4dbdc07ee06
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/sys_common/backtrace.rs:34:9
8: 0x109be88d3 - std::panicking::default_hook::{{closure}}::h529701a1070b4ce0
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:267:22
9: 0x109be8628 - std::panicking::default_hook::hfeeab2c667b2d7c2
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:286:9
10: 0x1083df5c1 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h9afa8b8b4a6f4294
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/alloc/src/boxed.rs:2032:9
11: 0x108406e1f - helix_term::application::Application::run::{{closure}}::{{closure}}::h59304607632a2a9a
at /Users/erasin/github/helix/helix-term/src/application.rs:1061:13
12: 0x109be9027 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h23ed9dbfdf16f482
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/alloc/src/boxed.rs:2032:9
13: 0x109be9027 - std::panicking::rust_panic_with_hook::h1b5245192f90251d
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:692:13
14: 0x109be8dd4 - std::panicking::begin_panic_handler::{{closure}}::h3658f3a9566379d4
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:579:13
15: 0x109be6d68 - std::sys_common::backtrace::__rust_end_short_backtrace::h9e01645d962f8882
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/sys_common/backtrace.rs:137:18
16: 0x109be8a9d - rust_begin_unwind
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:575:5
17: 0x109c4bee3 - core::panicking::panic_fmt::h0097ad8ec0b07517
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/panicking.rs:64:14
18: 0x109c4c365 - core::result::unwrap_failed::h2a0ffdcdbffb9262
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/result.rs:1791:5
19: 0x10964bc3a - core::result::Result<T,E>::unwrap::h76fa627ea2ccb7e6
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/result.rs:1113:23
- 20: 0x10870149f - ropey::slice::RopeSlice::char_to_line::h83b3a9f53cafd361
- at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/ropey-1.6.0/src/slice.rs:379:9
- 21: 0x1087b4dac - helix_term::ui::editor::EditorView::calculate_sticky_nodes::hd44adebca1ce03c2
- at /Users/erasin/github/helix/helix-term/src/ui/editor.rs:857:24
- 22: 0x1087b0298 - helix_term::ui::editor::EditorView::render_view::h801f2329103ac9f0
- at /Users/erasin/github/helix/helix-term/src/ui/editor.rs:191:17
- 23: 0x1087b9fa4 - <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render::h0d6ad5e880ce309a
- at /Users/erasin/github/helix/helix-term/src/ui/editor.rs:1604:13
- 24: 0x1087cb16e - helix_term::compositor::Compositor::render::h421b649aa5e8a5c7
- at /Users/erasin/github/helix/helix-term/src/compositor.rs:170:13
- 25: 0x108407ee7 - helix_term::application::Application::render::{{closure}}::h9128185baddbd711
- at /Users/erasin/github/helix/helix-term/src/application.rs:285:9
- 26: 0x108401061 - helix_term::application::Application::handle_terminal_events::{{closure}}::h452798700217d49a
- at /Users/erasin/github/helix/helix-term/src/application.rs:618:26
- 27: 0x1083ffbfe - helix_term::application::Application::event_loop_until_idle::{{closure}}::h8cc9ae38c69ea5ec
- at /Users/erasin/github/helix/helix-term/src/application.rs:326:55
- 28: 0x1083fd20c - helix_term::application::Application::event_loop::{{closure}}::hf15c5275827c0534
- at /Users/erasin/github/helix/helix-term/src/application.rs:302:57
- 29: 0x10840691d - helix_term::application::Application::run::{{closure}}::h64d788da4e75353b
- at /Users/erasin/github/helix/helix-term/src/application.rs:1064:38
- 30: 0x1083f5e1e - hx::main_impl::{{closure}}::h64babcb08100836d
- at /Users/erasin/github/helix/helix-term/src/main.rs:156:53
31: 0x1083d659e - tokio::runtime::park::CachedParkThread::block_on::{{closure}}::h04680cd7055be189
at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.26.0/src/runtime/park.rs:283:63
32: 0x1083d6403 - tokio::runtime::coop::with_budget::h3450751a913955ea
at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.26.0/src/runtime/coop.rs:107:5
33: 0x1083d6403 - tokio::runtime::coop::budget::hfece1804ad294c58
at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.26.0/src/runtime/coop.rs:73:5
34: 0x1083d6403 - tokio::runtime::park::CachedParkThread::block_on::h50891113bf07b160
at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.26.0/src/runtime/park.rs:283:31
35: 0x1083ed336 - tokio::runtime::context::BlockingRegionGuard::block_on::h97a7cfd8b0f5191b
at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.26.0/src/runtime/context.rs:315:13
36: 0x1083b9bd5 - tokio::runtime::scheduler::multi_thread::MultiThread::block_on::h6a41409b5c3a1748
at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.26.0/src/runtime/scheduler/multi_thread/mod.rs:66:9
37: 0x108417cfb - tokio::runtime::runtime::Runtime::block_on::h72dfd34556937174
at /Users/erasin/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.26.0/src/runtime/runtime.rs:304:45
38: 0x10840905e - hx::main_impl::h594c06858aac3bd2
at /Users/erasin/github/helix/helix-term/src/main.rs:158:5
39: 0x108408f01 - hx::main::hbd7e18d2aacb3ddc
at /Users/erasin/github/helix/helix-term/src/main.rs:38:21
40: 0x108409f7e - core::ops::function::FnOnce::call_once::h81bdc8e78b625ab3
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ops/function.rs:507:5
41: 0x1083df8a1 - std::sys_common::backtrace::__rust_begin_short_backtrace::h582282e1d3fcd9fa
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/sys_common/backtrace.rs:121:18
42: 0x1083e5d64 - std::rt::lang_start::{{closure}}::h8919f335f7d65c4f
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/rt.rs:166:18
43: 0x109bd9a24 - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h2302f1d25ef2ca9b
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ops/function.rs:606:13
44: 0x109bd9a24 - std::panicking::try::do_call::h6695e32a593de2cc
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:483:40
45: 0x109bd9a24 - std::panicking::try::hd4a93095627721a9
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:447:19
46: 0x109bd9a24 - std::panic::catch_unwind::he41b3dba63feca94
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panic.rs:137:14
47: 0x109bd9a24 - std::rt::lang_start_internal::{{closure}}::hbf45583011495a61
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/rt.rs:148:48
48: 0x109bd9a24 - std::panicking::try::do_call::ha3e6b3edab7da449
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:483:40
49: 0x109bd9a24 - std::panicking::try::hd4e0f354bf7022b9
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panicking.rs:447:19
50: 0x109bd9a24 - std::panic::catch_unwind::h1035b163871a4269
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/panic.rs:137:14
51: 0x109bd9a24 - std::rt::lang_start_internal::hd56d2fa7efb2dd60
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/rt.rs:148:20
52: 0x1083e5d37 - std::rt::lang_start::h2981ed93936c2adf
at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/std/src/rt.rs:165:17
53: 0x1084090e8 - _main
54: 0x7ff816549310 - <unknown>
|
Yes, we should reuse the https://github.com/nvim-treesitter/nvim-treesitter-context/blob/master/queries/go/context.scm This ways they are shareable across editors. |
Ok, have started working on it, and will continue these days. |
SoraTenshi
force-pushed
the
sticky-context
branch
from
49e657d to
7021fc4
Compare
helix-term/src/ui/editor.rs
Outdated
| .take(max_nodes_amount) | ||
| .rev() | ||
| .enumerate() | ||
| .take_while(|(i, _)| *i + 1 != visual_cursor_pos as usize) // also only nodes that don't overlap with the visual cursor position | ||
| .map(|(i, node)| { | ||
| let mut new_node = node; | ||
| new_node.visual_line = i as u16; | ||
| new_node | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
This will take max_node_amount and then remove elements again if they overlap, is that intended ?
The version below avoids one Vec allocation but I'm not convinced it would be faster, how big are you expecting context to be most of the time ?
context.reverse();
if context.len() > max_node_amount {
// only take the nodes until 1 / 3 of the viewport is reached or the maximum amount of sticky nodes
context.drain(max_node_amount..);
}
let mut idx = 0;
let mut overlap_reached = 1 == visual_cursor_pos;
context.retain_mut(|node| {
if overlap_reached { return false; }
overlap_reached = idx + 1 == visual_cursor_pos;
node.visual_line = idx;
idx += 1;
true
});There was a problem hiding this comment.
i would say that it will mostly sit around 3~5 nodes.
So i think performance there is not really that big of a deal, i would much rather avoid more of the text.x_to_y calls, as they're rather heavy. O(log n)
|
@poliorcetics i appreciate that you already di a new review, but I wasn't finished ^^` |
|
I'm able to crash helix after cherry-picking your patch. Without it, not happening. More surprised to find it after couple of days. To reproduce: Sync with latest HEAD, cherry-pick your patch. Now, go to commands.rs -> func global_search. Check the FileImpl struct, it's used inside PickerColumn::new("path"), inside it, type "item.", LSP will give pop-up with possible contents, use ctrl+n to navigate, helix crashes. Sometimes it doesn't happen when list returned by LSP is small. |
SoraTenshi
force-pushed
the
sticky-context
branch
2 times, most recently
from
18a63ef to
8ad3a03
Compare
based on the backtrace and what you describe i can safely assume this doesn't seem to be an issue of this pr, and it's most likely fixed here: #11266 This has been fixed with the latest rebase. |
SoraTenshi
force-pushed
the
sticky-context
branch
2 times, most recently
from
18555e7 to
ea3ac71
Compare
A lot more work has been put into this and those were 116 commits up to this point. I decided to squash all of them so that i will have an easier time rebasing in the future.
SoraTenshi
force-pushed
the
sticky-context
branch
from
a0efb1a to
9a7c439
Compare
|
That looks amazing and I’ve been missing that feature since my move away from |
Thank you! |
|
How severe and how often are the known issues occurring? Would it be on to merge with the feature toggled off by default and work on them separately? |
quite frequently. They're not really severe, but also not perfect.
On another node, i am not sure how this PR will continue. I suspect that it could make much more sense to make it a plugin instead (once the plugin support is finished) |
I believe this belongs in core. I am eager to hear a maintainer's opinion on this Thank you for doing this |
|
Works quite well! not sure if these have already been reported, but ive noticed two things, im on this branch but with
EDIT:
|
`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy
|
@SoraTenshi : Can you please tell me how to write context queries? The above (class_declaration) @Cotnext incorrectly renders the annotation "@" instead of the class className. Is there a away where I can precisely say the context to render which line? |
How to generally write context queries is basically re-building the TreeSitter AST that you want to match. Examples can be found in this PR, and helix has great tooling in how this works: |
I just compiled the source code as instructed by https://docs.helix-editor.com/building-from-source.html, pointed the new I've used C and Python with clangd and pylsp respectively. Thanks a lot for working on it! |
|
@marcosps it's mentioned in the book/src/editor.md file changed in this PR. It definitely should have been spelt out in the top post :D
|
| Key | Description | Default |
|---|---|---|
enable |
Display context of current line if outside the view | false |
indicator |
Display an additional line to indicate what part of the view is the sticky context | false |
max-lines |
The maximum number of lines to be shown as sticky context. 0 = 1/3 of the viewports height | 0 |
| Example: |
[editor.sticky-context]
enable = true
indicator = true
max-lines = 10
Thanks a lot @thomasaarholt ! I can confirm that it works most of the time while reading linux kernel source code (sometimes it doesn't show the function/struct), and it also happens to python. But it's better than nothing :) Thanks a lot @SoraTenshi for working on it! Do you have plans to keep working on it and maybe merge it somehow? :) (I saw some comments stating that maybe it should be a plugin, but the current status of the feature is practically finished) |
I am not sure how i will proceed with this pr. Considering that, i don't know. I am still very active though and will accept any PR that lands for this PR to become more stable. |
In the past I've pushed myself through early burnout warning signs and feelings and the result is almost always the same: more burnout, maybe the point where I want nothing to do with the thing in question for weeks or months. I'd highly suggest you take a break while you're only on the "sort of burned out" stage. You've put in a great deal of commendable effort over an extended period of time, allow your mind and your feelings to recover from all the work they've been doing! :) |
|
If there are smaller PRs you can make to core that make this one easier to merge, that could be a good strategy, and less tiring but yeah, what tsujp said above about burnout should come first, I agree |
Of course your health should come first! Thanks a lot for working on it until this point! The feature itself is already working fine for me, so IMHO this could be merged as it is :) |
Oh to everyone, I didn't mean in a literal sense that this PR got me burned out but more in a sense that the future of it is uncertain hence why I don't want to spend much more time on it. If anyone of the core team would give their approval, I would pick up work again. But I don't wanna see my polishing efforts go to waste just because it may as well be a plugin. (Which I might also would work on considering I am heavily in favor of scheme) |
Makes sense. @archseer @pascalkuthe what do you think about it? :) |
Maybe @the-mikedavis is interesting on it, or knows somebody that can decide of which direction this should go :) |
!! If your preferred colorscheme / language is not yet in this PR, please feel free to open a PR on my branch:
sticky-contextto add those! It helps me quite a lot, as i can't keep track of every single colorscheme / language that helix supports !!This is supposed to be the "successor PR" of #3944 .
Since @matoous has no time to pick up on it, i have started to continue on it (as i have also previously worked a bit on it!).
This is for now working, but with very rough edges and needs some fine adjustments.
Features, in no particular order:
{or just"..."- this should be possible via the text api -> i would also strongly believe, that this will make the 'configuration of tree-sitter nodes' obsolete.TypescriptECMAScript----- Bug Smashing: ------
I beg anyone to find another bug as mentioned here, to immediately notify me, even if it's minor, i am happy to investigate it (also; if possible, create a reproducer)
----- Overview: ------
implof structsshould then be collapsed to:
fn render(...) {Small gif:
closes #396