Support different kinds of underline rendering (updated)

[pascalkuthe]

Supersedes #3015 (and closes #2809).

I have rebased the original PR on master and addressed the issues raised in the original PR.
The reasons for the disappearing undercurls/underlines was that the orignal PR
treated them as modifiers. However, underlines styles behave more like colors than modifiers:
There are multiple different underline styles that are incompatible, overwrite each other and
are removed with a single escape sequence.
Modifiers on the other hand can be added and removed independent of other modifiers.

Tracking underline styles in the modifier set can not work correctly.
The fix suggested by @archseer of only emitting NoUnderline if there is no
underline left in to does not work for nested highlights.
Consider the following example of 3 nested highlights (underline used by syntax highlight, underdotted used by diagnostic, undercurl used by cursor):

The input highlight ranges look like this (which correspond to the modifier sets)

  ~
 ...
______
foobar

The expected output looks like this:

foobar
_.~.__

The corresponding escape sequences look like this:

[_]f[.]o[~]o[.]b[_]ar[nounderline]

From the modifier set it is not possible to build these escape sequences.
Because the modifier set only contains which modifiers are present but not
the order in which they were added removed (so after the foo it's impossible
to determine which underline should be shown).

This example might seem contrived, but I noticed this almost immediately in real world use.
To fix the problems in the original PR I renamed the underline style attribute to underline_color
and added the underline_style attribute for underline styles.
Now underline styles are tracked through the same mechanisms that fg/bg/underline-color are tracked
and therefore cases with nested highlights are handled correctly.

To esnure backwards compatibility of themes the underlined modifier will keep working and is equivalent to underline-style="line"

Edit: This updated PR also improves upon the original PR by using a much more robust terminfo parsing library (called termini written by me), because cxterminfo was barely tested and caused crashes. Furthermore a workaround for an issue in crossterm was implemented that caused visual artifacts in terminal emulators that do not support underline colors.

[Omnikar]

cxterminfo is panicking for me, on MacOS, iTerm2 v3.4.16

thread 'main' panicked at 'attempt to add with overflow', /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/cxterminfo-0.2.0/src/terminfo.rs:319:27

[pascalkuthe]

That's good to know. I personally don't own a Mac. Could you provide a back trace and the panic message so I can try to debug this?

[Omnikar]

Here's the output with RUST_BACKTRACE=1

Panic output

thread 'main' panicked at 'attempt to add with overflow', /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/cxterminfo-0.2.0/src/terminfo.rs:319:27
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:48:5
   3: cxterminfo::terminfo::TermInfo::from_data
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/cxterminfo-0.2.0/src/terminfo.rs:319:27
   4: cxterminfo::terminfo::TermInfo::from_file
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/cxterminfo-0.2.0/src/terminfo.rs:240:9
   5: cxterminfo::terminfo::TermInfo::from_name
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/cxterminfo-0.2.0/src/terminfo.rs:231:24
   6: cxterminfo::terminfo::TermInfo::from_env
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/cxterminfo-0.2.0/src/terminfo.rs:192:13
   7: helix_tui::backend::crossterm::Capabilities::from_env_or_default
             at ./helix-tui/src/backend/crossterm.rs:30:15
   8: helix_tui::backend::crossterm::CrosstermBackend<W>::new
             at ./helix-tui/src/backend/crossterm.rs:55:27
   9: helix_term::compositor::Compositor::new
             at ./helix-term/src/compositor.rs:93:23
  10: helix_term::application::Application::new
             at ./helix-term/src/application.rs:142:30
  11: hx::main_impl::{{closure}}
             at ./helix-term/src/main.rs:143:19
  12: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/future/mod.rs:91:19
  13: tokio::park::thread::CachedParkThread::block_on::{{closure}}
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/park/thread.rs:267:54
  14: tokio::coop::with_budget::{{closure}}
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/coop.rs:102:9
  15: std::thread::local::LocalKey<T>::try_with
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/thread/local.rs:445:16
  16: std::thread::local::LocalKey<T>::with
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/thread/local.rs:421:9
  17: tokio::coop::with_budget
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/coop.rs:95:5
  18: tokio::coop::budget
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/coop.rs:72:5
  19: tokio::park::thread::CachedParkThread::block_on
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/park/thread.rs:267:31
  20: tokio::runtime::enter::Enter::block_on
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/runtime/enter.rs:152:13
  21: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/runtime/scheduler/multi_thread/mod.rs:79:9
  22: tokio::runtime::Runtime::block_on
             at /Users/redacted/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-1.21.1/src/runtime/mod.rs:492:44
  23: hx::main_impl
             at ./helix-term/src/main.rs:147:5
  24: hx::main
             at ./helix-term/src/main.rs:37:21
  25: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5

[pascalkuthe]

That looks like an upstream bug :/ Digging into this one will be a pain although it doesn't look specifically MacOS related. I don't have the time to dig into terminfo parsing right now but will try to really dig into this the next few days.

While looking into this, I noticed that the cxterminfo crate (where this bug occurred) is quite well documented (and is only like 500LOC) but has basically no tests or reverse dependencies.
Using this crate was decided upon in the previous PR because the alternatives (terminfo) have more dependencies.
I assumed due diligence was done and didn't recheck the dependencies but they probably just missed that.

I will probably have to inline that create, really understand how it works and then write a bunch of tests because using a basically untested crate will not do. Marking this as draft until then.

Thanks for pointing my attention to this

[pascalkuthe]

Looking at cxterminfo made very uncomfortable with including it in helix.
The code is quite clearly the first (or one of the first) rust projects of the author.
For example many std functions are reimplemented in the crate and the code is written
in a very panicky manner (tons of slice indexing instead of using the Read trait).
In many places a naive but insufficient solutions were implemented (for example looking up the
home directory using $HOME instead of using dirs-next).
Most importantly the code was basically untested (and without any dependencies on crates.io)
and was not very nice to read.

The code is simple enough that I choose to rewrite a small terminfo parsing library myself
instead of trying to contribute to cxterminfo.
I have published termini to crates.io and updated this PR to use it.
Like cxterminfo, termini is focused on providing a low-dependency terminfo libary.
It only has a single dependency (dirs-next) and a very small footprint (less than 1k loc).
However, termini is written using idiomatic rust to avoid panics and ensure correct behavior.
I have heavily fuzzed the implementation, so the parser should basically never panic even for completely
nonsensical input.
I have added a ton of different terminfo files as test (including detecting undercurl support from kitty and alacritty).

Could you confirm that this is now working for you @Omnikar?

[Omnikar]

It's not panicking, but I am getting this weird dimming effect, and no coloured underlines (although I'm not sure whether iTerm2 supports coloured underlines? But that should be detected shouldn't it?):

[Omnikar]

I just tested with Kitty and confirmed that it works as expected, so I'm not sure what's going on with iTerm2.

[Philipp-M]

It's not panicking, but I am getting this weird dimming effect, ...

Yes I've also had that in alacritty.

I had to update to alacritty on master to get rid of this issue (as there isn't support for underline coloring in the current stable version yet (alacritty/alacritty#5935 has fixed that though)).

Interesting that the issue is similar in ITerm2.

I'm not sure if it's possible to check, whether the terminal supports underline coloring or not.

[pascalkuthe]

Hmm this should indeed be detected with terminfo. Perhaphs we are checking the wrong capabilities here. But atleast the capability parsing is working as expected :D

[pascalkuthe]

Perfect, that's the best we can do for terminals that don't support colored underlines

[Omnikar]

Should this PR add underline theming to more of the themes?

[pascalkuthe]

Should this PR add underline theming to more of the themes?

I have taught about this but decided against it.
Instead I would rater leave that up to a future PR/each theme author individually.
Themes can make different choices here on what underlines to use and colors and we would basically need a review from every theme author.
Updating all themes would also take forever and would be super tedious to review so I would rather not do that in this PR

[EpocSquadron]

Verified this is still working in wezterm!

The docs in this PR are unclear on if the underline-style and underline-color require the underline modifier to also be present.

[pascalkuthe]

Verified this is still working in wezterm!

The docs in this PR are unclear on if the underline-style and underline-color require the underline modifier to also be present.

Thanks for testing it on wezterm!

The underline modifier is only supported for backwards compatibility with existing themes and not required. It is mapped to underline-style=line when the theme is parsed and does not exist as an actual modifier internally.

I have added a note to the docs to make that more clear

[the-mikedavis]

This appears to also exhibit #3015 (comment): the styled underlines behave oddly when the cursor is moved across them and the cursor uses underline styling

underline.mp4

Relevant parts of the theme:

"ui.cursor.primary" = { bg = "bg3", modifiers = ["underlined"] }
"diagnostic.error" = { underline-color = "red1", underline-style = "curl" }

This is on Kitty 0.26.2 and wezterm 20220905-102802-7d4b8249

[pascalkuthe]

This appears to also exhibit #3015 (comment): the styled underlines behave oddly when the cursor is moved across them and the cursor uses underline styling
underline.mp4

Relevant parts of the theme:

"ui.cursor.primary" = { bg = "bg3", modifiers = ["underlined"] }
"diagnostic.error" = { underline-color = "red1", underline-style = "curl" }

This is on Kitty 0.26.2 and wezterm 20220905-102802-7d4b8249

I tested this case while originally working on the PR and it worked.
However since then my work on the terminfo detection introduced a new bug that happened lead to the exact same behavior (although it was completely unrelated) and I did not retest tat case. I have fixed that bug so it should work now.

Part of the bug was (partly) caused by a typo in the terminfo searchpath of termini (my unit tests all use terminfo files inside the repo so that wasn't caught by the tests) so in some cases termini actually didn't parse the terminfo file at all. Could you confirm this still working with iterm @Omnikar, just in case that bug applied on your system aswell?

[the-mikedavis]

Building from the latest, I see that being fixed on Kitty & WezTerm 👍

What do you think about having underline be a toml table?

"diagnostic.error" = { bg = "red0", underline = { color = "red1", style = "curl" } }

[pascalkuthe]

What do you think about having underline be a toml table?

"diagnostic.error" = { bg = "red0", underline = { color = "red1", style = "curl" } }

I think this looks much nicer. I wasn't a big fan at first when I changed the themes because the most common use-case for these new attributes is diagnostics, which looked needlessly nested:

"diagnostic.error" = { underline = { color = "red", style = "curl"} }

However then I remembered that toml allows this:

"diagnostic.error".underline = { color = "red", style = "curl"} 

That looks is much nicer :) I have updated the PR.

On a slightly offtopic note. Wouldn't it be cleaner to alos implement highlight scopes like that so we could avaoid the quting and write:

diagnoistic.error.underline = {color = "red", style = "curl" }

[the-mikedavis]

I think it might make the parsing or merging awkward if theme keys weren't strings

[the-mikedavis]

I'm very excited for this to land, thanks for working on it!