Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support different kinds of underline rendering #3015

Closed

Conversation

sudormrfbin
Copy link
Member

@sudormrfbin sudormrfbin commented Jul 8, 2022

Adds four new modifiers that can be used in themes:

  • undercurled
  • underdashed
  • underdotted
  • double-underline

Screenshot_2022-07-09_03-42-49

TODO

  • Specify color for the underline (currently automatically inherits the style of the cell).

@archseer archseer linked an issue Jul 9, 2022 that may be closed by this pull request
@A-Walrus
Copy link
Contributor

Wrote a PR to add color to the underlines: PR

@archseer
Copy link
Member

Wrote a PR to add color to the underlines: PR

Looks great! I wanted support for that too :)

@the-mikedavis the-mikedavis mentioned this pull request Aug 17, 2022
sudormrfbin and others added 5 commits September 7, 2022 21:50
Adds four new  modifiers that can be used in themes:

- undercurled
- underdashed
- underdotted
- double-underline
Add underline field to doctests, and fix bugs
The cxterminfo crate has been used over popular alternatives
like `term` since it supports querying for extended capabilities
and also for it's small codebase size (which will make it easy
to inline it into helix in the future if required).
@sudormrfbin
Copy link
Member Author

Added undercurl support detection based on terminfo. Thanks to @A-Walrus for the color customization code :)

@the-mikedavis
Copy link
Member

the-mikedavis commented Sep 7, 2022

This is really sweet 😀

I took it for a spin and I found some odd behavior when using move_char_left over a diagnostic: https://asciinema.org/a/QwKpcsg1IXFj84JTcvjqBWWxc

The asciicast doesn't really show it very well but I have diagnostic.error set to

"diagnostic.error" = { underline = "red1", modifiers = ["undercurled"] }

I also have the ui.cursor.primary set to underline too

"ui.cursor.primary" = { bg = "bg3", modifiers = ["underlined"] }

Moving right across the diagnostic doesn't do anything interesting but moving left seems to un-underline the parts of the diagnostic that the primary cursor was on.

So I think there might be some conflict with the intersect or resetting behavior?

@sudormrfbin
Copy link
Member Author

sudormrfbin commented Sep 7, 2022

This only seems to affect them when the underlines are different, for example when both the error and cursor modifiers are undercurl, there is no glitch. Also the glitching behavior is also slightly different for me; putting the cursor in the middle of the error underlined word cuts off the rest of the word underline rather than removing the underline only on pressing the left key:

helix-underline-bug

The modifiers used (the cursor is undercurled instead of the error):

"diagnostic.error" = { underline = "red", modifiers = ["underlined"] }
"ui.cursor.primary" = { fg = "white", modifiers = ["reversed", "undercurled"] }

@archseer
Copy link
Member

archseer commented Sep 8, 2022

There seems to be no reset call for SetUnderlineColor in this block:

map_error(queue!(
self.buffer,
SetForegroundColor(CColor::Reset),
SetBackgroundColor(CColor::Reset),
SetAttribute(CAttribute::Reset)
))

@@ -153,7 +192,7 @@ impl ModifierDiff {
if removed.contains(Modifier::ITALIC) {
map_error(queue!(w, SetAttribute(CAttribute::NoItalic)))?;
}
if removed.contains(Modifier::UNDERLINED) {
if removed.intersects(Modifier::ANY_UNDERLINE) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that we have to emit NoUnderline followed by underline if the cell changes from one underline to a different type in the next one?

@archseer
Copy link
Member

archseer commented Sep 9, 2022

I tried this on wezterm: the underline color changes, but it's using the same underline (not undercurl). I did confirm that undercurl works normally via

printf "\x1b[58:2::255:0:0m\x1b[4:1msingle\x1b[4:2mdouble\x1b[4:3mcurly\x1b[4:4mdotted\x1b[4:5mdashed\x1b[0m\n"

@archseer
Copy link
Member

archseer commented Sep 9, 2022

has_extended_underlines is false for me. Manually overriding it to always be true works and I don't have the same rendering issue as above

@sudormrfbin
Copy link
Member Author

has_extended_underlines is false for me. Manually overriding it to always be true works and I don't have the same rendering issue as above

Seems like wezterm's terminfo file has to installed separately:

curl https://raw.githubusercontent.com/wez/wezterm/master/termwiz/data/wezterm.terminfo | tic -x -

There's also one more problem at play here even if you have the terminfo file but when it's installed in ~/.terminfo/: tic attempts to install the terminfo entry in /usr/share/terminfo first and if denied permission installs it in ~/.terminfo, so by default the above command will install it in your HOME. But there seems to be a bug with the path added in cxterminfo, adding ~/ instead of ~/.terminfo. It's a separate bug but overlaps with the wezterm issue.

So to check if wezterm works with undercurl:

curl https://raw.githubusercontent.com/wez/wezterm/master/termwiz/data/wezterm.terminfo | tic -x -
export TERM=wezterm
export TERMINFO=~/.terminfo
hx # or cargo run, etc

Fixing the cxterminfo bug should remove the export TERMINFO step, but the terminfo file will still have to be installed for wezterm (unless we also add a version check similar to checking $VTE_VERSION).

@EpocSquadron
Copy link
Contributor

Tested this out and noticed sometimes the way underlines work in wezterm it's going to be clipped (in other words, there's not enough room for the underlines so it looks funky). I brought it up in the wezterm matrix and there may be upcoming fixes to make it look better in more cases, but I think we should include up front in the documentation that font adjustments may be needed with any given terminal emulator for the fancy underlines to look good. For example, wezterm provides a way to increase the line height and adjust the positioning of the underline to be closer to the text. Any combination of that may be enough to resolve the issue.

See matrix discussion in wezterm room for more information: https://matrix.to/#/!PirwUBcuIlTXwNveYz:matrix.org/$muIHcpgbicXxPXLFuxvfXEMmPg-oqmGu0drVRc_UnA4?via=matrix.org&via=kde.org&via=mozilla.org

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2022
@sudormrfbin
Copy link
Member Author

Closing in favor of #4061.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colored and styled underlines for errors/warnings
6 participants