helix icon indicating copy to clipboard operation
helix copied to clipboard

Feat: Sticky Context

Open SoraTenshi opened this issue 1 year ago • 99 comments

!! If your preferred colorscheme / language is not yet in this PR, please feel free to open a PR on my branch: sticky-context to 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:

  • [x] Basic functionality
  • [x] Max viewport height (limits the max. amount of contexts)
  • [x] Use top of viewport instead of cursor pos
  • [x] Do not stick contexts that are less than at least half the height of the viewport
  • [x] Configurable tree-sitter nodes
  • [x] Calculate precise viewport based on recent sticky nodes
  • [x] Sticky context indicator, a la context.vim
  • [x] Render Cursor over contextes
  • [x] Adjusted Line number rendering
  • [x] Configurable line limit for context
  • [x] Collapsing function Arguments to include everything from name to the start of the first { 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.
  • [x] Add more tree-sitter queries
    • [x] C, C++
    • [x] Elixir
    • [x] Go
    • [x] ~~Typescript~~ ECMAScript
    • [x] Nix
    • [x] Rust
    • [x] Zig
    • [x] JSON
    • [x] TOML
    • [x] Markdown
    • [x] YAML
    • [x] HTML
    • [x] Python (thank you, winged!)
    • [x] Scala (thank you, jpaju!)
    • [x] C# (thank you, vlad-anger!)
    • [ ] Odin
  • [x] Add documentation on how to add more context queries
  • [x] Update docs when finalized

----- Bug Smashing: ------

  • [x] Fix bug where nodes jitter
  • [x] Redo the context.end logic, work with parameter captures instead of from func decl to end, there should be a way capture the byte range of all the parameters.
  • [x] Fix the "line shrinking" logic when the cursor gets on a sticky node
  • [x] Improve overall calculation performance
  • [x] Figure out a way to improve calculation performance if cache is empty (currently tanks too much)
  • [ ] Somewhere, cache invalidation is still not correct
  • [ ] In some situations, unnecessary many nodes are rendered
  • [ ] Somewhere there's a crash (can't repr though)

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

  • The nodes are rendered by the following rules:
    • The last node, if configured, is always the indicator
    • Prioritize closer nodes more than much further nodes (e.g. functions are more prioritized than e.g. impl of structs
  • of 1 tree-sitter node, always the 1st line is taken, even if the node is let's say 3 lines long.
  • The sticky context may not render at all when the viewport is too small. Yes i tried to keep it as dynamic as possible, but at some point it's getting too small
  • Eventually it would be very beneficial, if me or someone else in the future can make argument on multi-line function declarations collapseable (e.g.
fn render(
	i: i32,
	x: u32,
	y: usize,
	z: Option<void>,
) {

should then be collapsed to: fn render(...) {

Small gif: FancyWM_VdrUeBZJGN

closes #396

SoraTenshi avatar Feb 27 '23 09:02 SoraTenshi

here is a preview of how it looks like at the moment. With the new changes, a lot of things became much easier than expected.

image

SoraTenshi avatar Feb 28 '23 20:02 SoraTenshi

i think in order to satisfy clippys complaint, i would have to use the #[allow(clippy::too_many_arguments)] macro, i could of course just wrap one of the parameter in a tuple, but what's the point in it.

Maybe someone else got an idea how to get the render_sticky_context function down to a max of 7 Arguments.

SoraTenshi avatar Mar 01 '23 09:03 SoraTenshi

i think in order to satisfy clippys complaint, i would have to use the #[allow(clippy::too_many_arguments)] macro, i could of course just wrap one of the parameter in a tuple, but what's the point in it.

Maybe someone else got an idea how to get the render_sticky_context function down to a max of 7 Arguments.

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)

poliorcetics avatar Mar 03 '23 12:03 poliorcetics

Note: the Neovim plugin this is based on is switching to queries for determining the nodes (https://github.com/nvim-treesitter/nvim-treesitter-context/pull/198). We should do the same (similar to indents where we started with a toml config but now use proper queries)

archseer avatar Mar 08 '23 10:03 archseer

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.

SoraTenshi avatar Mar 08 '23 11:03 SoraTenshi

I would advocate not to depend on tree-sitter on this feature. There are still many languages that don't have TS. I use such languages and I often miss even simple things, like match pairs because it also depends on TS. Please be considerate to us, poor people, who have to deal with multiple languages. A simple word-split is more than enough.

yerlaser avatar Mar 08 '23 14:03 yerlaser

I would advocate not to depend on tree-sitter on this feature. There are still many languages that don't have TS. I use such languages and I often miss even simple things, like match pairs because it also depends on TS. Please be considerate to us, poor people, who have to deal with multiple languages. A simple word-split is more than enough.

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

SoraTenshi avatar Mar 08 '23 14:03 SoraTenshi

I would advocate not to depend on tree-sitter on this feature. There are still many languages that don't have TS. I use such languages and I often miss even simple things, like match pairs because it also depends on TS. Please be considerate to us, poor people, who have to deal with multiple languages. A simple word-split is more than enough.

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

pascalkuthe avatar Mar 08 '23 15:03 pascalkuthe

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. Another case is markup languages like markdown. As far as I understand, even if a TS existed for it, an object would be the whole text block which is too large of a target to go to.

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.

yerlaser avatar Mar 08 '23 15:03 yerlaser

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 git-rebase, ini, wit, passwd, hosts,.. . If you are missing a TS grammar for something it should be easy enough to create one (and you would be surprised how much is already covered).

pascalkuthe avatar Mar 08 '23 15:03 pascalkuthe

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.

erasin avatar Mar 09 '23 04:03 erasin

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? Can you provide an example?

SoraTenshi avatar Mar 09 '23 08:03 SoraTenshi

I have added the following configuration.

[[language]]
name = "go"
sticky-context-nodes = ["package_clause","type_declaration","method_declaration","function_declaration","for_statement","if_statement", "expression_switch_statement"]

I test with golang base library net/http/request.go.

Some times it is displayed correctly and some times it is not displayed.

erasin avatar Mar 09 '23 12:03 erasin

sticky-context-nodes = ["package_clause","type_declaration","method_declaration","function_declaration","for_statement","if_statement", "expression_switch_statement"]

ahh maybe i know why. I added that if a node occupies more than 1 half of the screen, it may not be sticked.

I will remove that, as it's an old artifact. Thanks for the finding! :)

SoraTenshi avatar Mar 09 '23 12:03 SoraTenshi

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

https://user-images.githubusercontent.com/716514/224236473-07d57707-d088-49ef-b4e0-c6563f1ed649.mov


I 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>


erasin avatar Mar 10 '23 06:03 erasin

Do i understand it correctly, that you'd want seperate injection queries (tree-sitter) for the context?

Yes, we should reuse the nvim-treesitter format for these queries too:

https://github.com/nvim-treesitter/nvim-treesitter-context/blob/master/queries/go/context.scm https://github.com/nvim-treesitter/nvim-treesitter-context/blob/master/queries/rust/context.scm

This ways they are shareable across editors.

archseer avatar Mar 10 '23 06:03 archseer

@yerlaser We leverage tree-sitter because it makes the implementation of various features easier and more performant. We'll sometimes implement fallbacks though, for example matchbrackets is getting one: https://github.com/helix-editor/helix/pull/4288

archseer avatar Mar 10 '23 06:03 archseer

Do i understand it correctly, that you'd want seperate injection queries (tree-sitter) for the context?

Yes, we should reuse the nvim-treesitter format for these queries too:

nvim-treesitter/nvim-treesitter-context@master/queries/go/context.scm nvim-treesitter/nvim-treesitter-context@master/queries/rust/context.scm

This ways they are shareable across editors.

Ok, have started working on it, and will continue these days.

SoraTenshi avatar Mar 10 '23 09:03 SoraTenshi

@poliorcetics i appreciate that you already di a new review, but I wasn't finished ^^`

SoraTenshi avatar Mar 11 '23 09:03 SoraTenshi

@poliorcetics i appreciate that you already di a new review, but I wasn't finished ^^`

I didn't even see it was a draft, sorry!

poliorcetics avatar Mar 11 '23 09:03 poliorcetics

When I have non-ascii in my code, the capture is incorrect. For example, the following insert method.

test file: gist.github.com/erasin/a15c4f250beff8c9f7e4aaeae4a1128c

I get a panic when using the mouse to scroll end of file. The error was not found in master.

I think your panic should be resolved now. I have tested it and it worked fine for me, with the new changes.

Big Thanks to @poliorcetics for the review comments.

The current state is: Injection-queries work (at least the base version), but i have a feeling that how i am doing them right now is not very efficient, effectively somehow wasted resource / cpu time. Also i think caching might be important here, i seem to have slightly more performance loss with the queries now, but like i said, it may be due to me not using the queries as effectively as possible.

SoraTenshi avatar Mar 11 '23 17:03 SoraTenshi

thanks for fixed.

In my tests, the context disappears before the function is finished, which is a bit different from vscode.

erasin avatar Mar 12 '23 02:03 erasin

thanks for fixed.

In my tests, the context disappears before the function is finished, which is a bit different from vscode.

Yes i think it's better to keep mind of the cursor context as well as the top viewport.

If your cursor is out of the context of the function, it will get un-sticked. Is this behaviour not wanted?

SoraTenshi avatar Mar 12 '23 14:03 SoraTenshi

I'm not sure, I've only using this feature in vscode.

It seems to be displayed when cursor scroll to the top, rather than in the function.

erasin avatar Mar 12 '23 15:03 erasin

It seems to be displayed when cursor scroll to the top, rather than in the function.

ah i think that's my caching. You would have to move the viewport (e.g. with zz) but this isn't finished yet, as i could with the query approach do it a bit easier (but this isn't quite implemented as i want to be, and will take some more time as i have a great idea for how to handle multi-line function signatures.

SoraTenshi avatar Mar 12 '23 15:03 SoraTenshi

I would love if some people can contribute with colorschemes and additional context queries.

SoraTenshi avatar Mar 17 '23 21:03 SoraTenshi

I think every feature like this should have a good fallback choice regarding colors, that way every theme should work out of the box. That's not happened for inlay-hints and now most themes need to be updated. A good fallback solution for inlay-hints would have been the comment color for example.

For this particular feature maybe the best choice is taking the background color from the statusline.

eulerdisk avatar Mar 17 '23 23:03 eulerdisk

I think every feature like this should have a good fallback choice regarding colors, that way every theme should work out of the box. That's not happened for inlay-hints and now most themes need to be updated. A good fallback solution for inlay-hints would have been the comment color for example.

For this particular feature maybe the best choice is taking the background color from the statusline.

There is a fallback for inlay hints. It's ui.virtual (which existed previously in quite a few themes and looked good in many of them). Theme keys fundamentally fallback along their paths (so ui.virtual.inlay-hint falls back to ui.virtual). I don't think we should start introducing fallbacks to random theme keys. Some of these already exist but they are far and few between and usually well justified. Introducing a bunch of new random fallbacms will just make the themeing more confusing and introduce unnecessary complexity (and debate about the best fallback).

Themes usually get updated after a while I don't think it's really a major concern.

pascalkuthe avatar Mar 18 '23 00:03 pascalkuthe

There is a fallback for inlay hints. It's ui.virtual (which existed previously in quite a few themes and looked good in many of them). Theme keys fundamentally fallback along their paths (so ui.virtual.inlay-hint falls back to ui.virtual). I don't think we should start introducing fallbacks to random theme keys. Some of these already exist but they are far and few between and usually well justified. Introducing a bunch of new random fallbacms will just make the themeing more confusing and introduce unnecessary complexity (and debate about the best fallback).

Themes usually get updated after a while I don't think it's really a major concern.

that's actually a good point, which i didn't know. I will change it so that it won't have a fallback, e.g. ui.sticky.context and ui.sticky.indicator

SoraTenshi avatar Mar 18 '23 01:03 SoraTenshi

There is a fallback for inlay hints. It's ui.virtual (which existed previously in quite a few themes and looked good in many of them). Theme keys fundamentally fallback along their paths (so ui.virtual.inlay-hint falls back to ui.virtual). I don't think we should start introducing fallbacks to random theme keys. Some of these already exist but they are far and few between and usually well justified. Introducing a bunch of new random fallbacms will just make the themeing more confusing and introduce unnecessary complexity (and debate about the best fallback). Themes usually get updated after a while I don't think it's really a major concern.

that's actually a good point, which i didn't know. I will change it so that it won't have a fallback, e.g. ui.sticky.context and ui.sticky.indicator

Hmm looking at the sourcecode again ui.bufferline does indeed fallback to ui.statusline. Maybe to be consistent with other fallbacks the bufferline fallback should be changed. Perhaphs just using ui.statusline.bufferline and ui.statusline.inactive.bufferline would work? (and ui.statusline.context equivalently). This also makes sense to me, because both the sticky context and bufferline are really just a special kind of statusline in a way. This would be a breaking change tough so at least changing bufferline theming should be a separate PR. But we could do it "right" from the start for this feature so we don't have to do another breaking change later. I would be in favor of ui.statusline.context for this PR.

The indicator could be ui.virtual.seperator.context (it's really more of a seperator than an indicator to me anyway). Most themes would just set ui.virtual.seperator (or ui.virtual). It's not technically virtual text but it's close enough I think that this would seem intuitive

pascalkuthe avatar Mar 21 '23 01:03 pascalkuthe