dnkl/foot
41
2.0k
Fork
You've already forked foot
243

Styled + colored underlines #1747

Manually merged
dnkl merged 24 commits from curly-underlines into master 2024年07月01日 10:10:04 +02:00
Owner
Copy link

This implements extended underlines; all alternative styles as well as custom underline colors, as defined in https://sw.kovidgoyal.net/kitty/underlines/

Underline style+color is stored in the extra row member. This is additional data that is only allocated when necessary. This is also where OSC-8 hyperlinks are stored. Just like OSC-8, the styled underlines are stored as ranges, covering one or more cells.

We only use the extra row data when we actually need to; legacy underlines do not require it. We only need it when a custom style, or a custom color has been set.

TODO

  • More testing, especially text reflow
  • Measure memory usage
  • vtebench
  • See if we can share more code between OSC-8 and underlines
  • Additional terminfo capabilities (we now define Su, Smulx, Setulc)

This PR re-uses the logic for rendering the underlines from #1099.

This PR replaces #1099
Closes #828

Memory usage

I haven't measured it, but analyzing it, I think the current implementation is acceptable.

  • The cell struct has not been changed, which means no additional memory usage unless the application actually emits styled underlines (or OSC-8 hyperlinks).
  • The extra row member is still (i.e. like before, with OSC-8) allocated on-demand, which means no additional row memory usage until the application emits styled underlines (or OSC-8 hyperlinks)
  • The extra row member has grown in size - it now has two "range" arrays rather than one. Each range array is 8+4+4 bytes. This is the only place where we've increased the size of an already existing data structure. So, in an application that only emits OSC-8, memory usage has gone up by 12 bytes per row.
  • Each range (for styled underlines) is 4+4+16 = 24 bytes. They would be 20 bytes, except there's a union in there, and the OSC-8 part of that is 16 bytes vs. 12 bytes for styled underlines. We could compress the underline part even further, but it's hard to do anything about the OSC-8 part. In any case, since underlines are rarely single characters only, we quickly "save" the extra space used, compared to growing the cell struct and using it to store underline data.

Vtebench

Only affected benchmark is the "dense cells". This isn't unsurprising, since it applies underlines to all cells.

Branch Time
master 42.78ms avg (90% < 50ms) +-6.84ms
curly-underlines 46.89ms avg (90% < 57ms) +-7.74ms

I'll try to spend some time to see if we can improve it. My guess is it's the VT parser that's slower (rendering is also slower, due to the curly range lookup, but I don't think that's causing the slowdown we see here).

Terminfo

There's no real standard here... we define the same capabilities kitty does:

  • Su - a boolean, indicates support for styled underlines
  • Smulx - a string capability, for emitting styled underlines (not color)
  • Setulc - a string capability, for emitting colored underlines (color only, not style).

From what I can tell, upstream (ncurses) allows Smulx (I see it defined for a couple of entries), but not Su and not Setulc. This is something we should document (non-standard terminfo capabilities defined in "our" terminfo, but not ncurses', is currently documented in the wiki, but perhaps we should document it in the README too).

This implements extended underlines; all alternative styles as well as custom underline colors, as defined in https://sw.kovidgoyal.net/kitty/underlines/ Underline style+color is stored in the `extra` row member. This is additional data that is only allocated when necessary. This is also where OSC-8 hyperlinks are stored. Just like OSC-8, the styled underlines are stored as ranges, covering one or more cells. We only use the `extra` row data when we actually need to; legacy underlines do not require it. We only need it when a custom style, or a custom color has been set. **TODO** * [x] More testing, especially text reflow * [x] Measure memory usage * [x] vtebench * [x] See if we can share more code between OSC-8 and underlines * [x] Additional terminfo capabilities (we now define `Su`, `Smulx`, `Setulc`) This PR re-uses the logic for rendering the underlines from #1099. This PR replaces #1099 Closes #828 ## Memory usage I haven't measured it, but analyzing it, I think the current implementation is acceptable. * The cell struct has not been changed, which means no additional memory usage unless the application actually emits styled underlines (or OSC-8 hyperlinks). * The `extra` row member is still (i.e. like before, with OSC-8) allocated on-demand, which means no additional row memory usage until the application emits styled underlines (or OSC-8 hyperlinks) * The `extra` row member **has** grown in size - it now has **two** "range" arrays rather than one. Each range array is 8+4+4 bytes. This is the **only** place where we've increased the size of an already existing data structure. So, in an application that **only** emits OSC-8, memory usage has gone up by 12 bytes per row. * Each range (for styled underlines) is 4+4+16 = 24 bytes. They would be 20 bytes, except there's a union in there, and the OSC-8 part of that is 16 bytes vs. 12 bytes for styled underlines. We could compress the underline part even further, but it's hard to do anything about the OSC-8 part. In any case, since underlines are rarely single characters only, we quickly "save" the extra space used, compared to growing the cell struct and using it to store underline data. ## Vtebench Only affected benchmark is the "dense cells". This isn't unsurprising, since it applies underlines to all cells. | Branch | Time | |----------|--------| | master | 42.78ms avg (90% < 50ms) +-6.84ms | | curly-underlines | 46.89ms avg (90% < 57ms) +-7.74ms | I'll try to spend some time to see if we can improve it. My guess is it's the VT parser that's slower (rendering is also slower, due to the curly range lookup, but I don't think that's causing the slowdown we see here). ## Terminfo There's no real standard here... we define the same capabilities kitty does: * `Su` - a boolean, indicates support for styled underlines * `Smulx` - a string capability, for emitting styled underlines (**not** color) * `Setulc` - a string capability, for emitting colored underlines (color only, **not** style). From what I can tell, upstream (ncurses) allows `Smulx` (I see it defined for a couple of entries), but not `Su` and not `Setulc`. This is something we should document (non-standard terminfo capabilities defined in "our" terminfo, but not ncurses', is currently documented in the wiki, but perhaps we should document it in the README too).
dnkl force-pushed curly-underlines from 50ae470483
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
to accbe60e64
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
2024年06月25日 08:08:47 +02:00
Compare
dnkl force-pushed curly-underlines from accbe60e64
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
to df75afc7ba
Some checks failed
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline failed
2024年06月25日 08:14:17 +02:00
Compare
dnkl force-pushed curly-underlines from df75afc7ba
Some checks failed
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline failed
to 6a8a7d8783
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
2024年06月25日 08:20:19 +02:00
Compare
First-time contributor
Copy link

Encountered a crash when running LSP format on a buffer. It doesn't happen every time, but it happens quickly repeating 'format, undo, format, etc.' bt full attached... I compiled with the debug buildtype for this, but let me know if this isn't quite right.

Encountered a crash when running LSP format on a buffer. It doesn't happen every time, but it happens quickly repeating 'format, undo, format, etc.' `bt full` attached... I compiled with the debug buildtype for this, but let me know if this isn't quite right.
14 KiB
Author
Owner
Copy link

@rhask thanks! Can you please pull the latest updates and test again? 26fcab4a47 should fix your crash.

@rhask thanks! Can you please pull the latest updates and test again? https://codeberg.org/dnkl/foot/commit/26fcab4a477a84dbfe2dceef1036dfab970c4dea should fix your crash.
First-time contributor
Copy link

That seems to have fixed it!

That seems to have fixed it!
dnkl force-pushed curly-underlines from 26fcab4a47
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
to cb4a74e10b
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
2024年06月26日 18:39:29 +02:00
Compare
grid: refactor: replace {uri,curly}_range_append() with range_append()
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
0d3f2f27e3
Also add range_append_by_ref(), with replaces
uri_range_append_no_strdup().
render: underlines: improve the appearance of the 'dotted' style
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
19bf558e6c
Try to make the 'dotted' style appear more even, and less like each
cell is rendered separately (even though they are).
Algorithm:
Each dot is a square; it's sides are that of the font's line
thickness.
The spacing (gaps) between the dots is initially the same width as the
dots themselves.
This means the number of dots per cell is the cell width divided by
the dots' length/width, divided by two.
At this point, there may be "left-over" pixels.I.e. the widths of the
dots and the gaps between them may not add up to the width of the
cell.
These pixels are evenly (as possible) across the gaps.
There are still visual inaccuracies at small font sizes. This is
impossible to fix without changing the way underlines are rendered, to
render an entire line in one go. This is not something we want to do,
since it'll make styled underlines, for a specific cell/character,
look differently, depending on the surrounding context.
dnkl force-pushed curly-underlines from d791dbabf4
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
to 73f6982cbe
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
2024年06月27日 19:22:06 +02:00
Compare
reaadme: add styled and colored underlines to the feature list
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
ci/woodpecker/pr/woodpecker Pipeline was successful
ci/woodpecker/pull_request_closed/woodpecker Pipeline was successful
b503c0d6d9
dnkl changed title from (削除) WIP: styled + colored underlines (削除ここまで) to Styled + colored underlines 2024年07月01日 10:08:32 +02:00
dnkl manually merged commit 94b9014f95 into master 2024年07月01日 10:10:04 +02:00
@ -98,7 +132,13 @@ csi_sgr(struct terminal *term)
case 21: break; /* double-underline, not implemented */
Collaborator
Copy link

I guess SGR 21 could be supported, now that double underlines are implemented?

I guess SGR 21 could be supported, now that double underlines are implemented?
Author
Owner
Copy link

I considered it, but according to Wikipedia (I know, I know, not the best source), it's ambiguous (double underline, or not bold).

But sure, ECMA-48 says it's double underline, so yeah, we could support it.

I considered it, but according to Wikipedia (I know, I know, not the best source), it's ambiguous (double underline, or not bold). But sure, ECMA-48 says it's double underline, so yeah, we could support it.
Collaborator
Copy link

Ah yeah, I forgot about that detail. I guess it may make sense to just ignore SGR 21 then.

Related kitty issue

Ah yeah, I forgot about that detail. I guess it may make sense to just ignore SGR 21 then. [Related kitty issue](https://github.com/kovidgoyal/kitty/issues/226)
Author
Owner
Copy link

I read the kitty issue, and there were much more consensus than I had expected; except for kitty, all terminal devs that participated in the discussion were in favor of SGR 21 meaning double underline.

And, since it is in ECMA-48, I thought, why not, let's implement it: #1756

I read the kitty issue, and there were much more consensus than I had expected; except for kitty, all terminal devs that participated in the discussion were in favor of SGR 21 meaning double underline. And, since it _is_ in ECMA-48, I thought, why not, let's implement it: https://codeberg.org/dnkl/foot/pulls/1756
craigbarnes marked this conversation as resolved
@ -107,2 +105,4 @@
};
enum curly_style {
CURLY_NONE,
Collaborator
Copy link

It's only a minor nit, but the naming here seems odd to me. Only one of these styles is "curly" and the rest are just underlines. I think enum underline_style, struct underline_range_data and UNDERLINE_* would be somewhat less confusing.

It's only a minor nit, but the naming here seems odd to me. Only one of these styles is "curly" and the rest are just underlines. I think `enum underline_style`, `struct underline_range_data` and `UNDERLINE_*` would be somewhat less confusing.
Author
Owner
Copy link

Yeah, I considered it. Went with CURLY since a) it's shorter, and b) the only thing people ever use styled underlines for is curly underlines...

Let me think it over one more time; perhaps it is better to change is. Or, if you feel strongly about it, feel free to push+merge a PR.

Yeah, I considered it. Went with CURLY since a) it's shorter, and b) the only thing people ever use styled underlines for is curly underlines... Let me think it over one more time; perhaps it is better to change is. Or, if you feel strongly about it, feel free to push+merge a PR.
Collaborator
Copy link

the only thing people ever use styled underlines for is curly underlines...

Yeah, I figured that was probably why.

if you feel strongly about it

I don't feel especially strongly about it to be honest. I was just reading through the code and the first thing that came to mind was "wait, are they really all curly? 😅".

feel free to push+merge a PR.

#1755.

> the only thing people ever use styled underlines for is curly underlines... Yeah, I figured that was probably why. > if you feel strongly about it I don't feel especially strongly about it to be honest. I was just reading through the code and the first thing that came to mind was "wait, are they really _all_ curly? 😅". > feel free to push+merge a PR. #1755.
craigbarnes marked this conversation as resolved
First-time contributor
Copy link

It would be nice if this PR could also address the issue of the previous PR regarding undercurl having the lows at the characters instead of the highs.

It would be nice if this PR could also address the [issue](https://codeberg.org/dnkl/foot/pulls/1099#issuecomment-2033409) of the previous PR regarding undercurl having the lows at the characters instead of the highs.
First-time contributor
Copy link

I've noticed since foot 1.18.0 the underline-offset option does nothing, despite still being in the foot.ini manpage.
obrazek

I've noticed since foot 1.18.0 the `underline-offset` option does nothing, despite still being in the `foot.ini` manpage. ![obrazek](/attachments/941c2690-d189-40dd-8f5a-2d19e2b4c353)
Author
Owner
Copy link

No, it still works. But curlies take more space, and the offset is clamped to ensure it doesn't extend below the row. Try setting a larger line-height.

As for where the curls should have their lows, that's something people will have different opinions on, regardless of which option we go with. Foot currently renders them like kitty. Alacritty does the opposite. Neither allows you to configure it, as far as I can tell? I don't think this is a detail that's important enough to add a configuration option for.

Now, for any further comments, please either ask for support on our IRC channel, or open a new bug/feature report. This PR has already been merged.

No, it still works. But curlies take more space, and the offset is clamped to ensure it doesn't extend below the row. Try setting a larger `line-height`. As for where the curls should have their lows, that's something people will have different opinions on, regardless of which option we go with. Foot currently renders them like kitty. Alacritty does the opposite. Neither allows you to configure it, as far as I can tell? I don't think this is a detail that's important enough to add a configuration option for. Now, for _any_ further comments, please either ask for support on our IRC channel, or open a new bug/feature report. This PR has already been merged.
cljoly referenced this pull request from a commit 2025年06月30日 21:46:44 +02:00
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
dnkl/foot!1747
Reference in a new issue
dnkl/foot
No description provided.
Delete branch "curly-underlines"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?