3 is italic (already implemented), 6 should be rapid blink according to http://en.wikipedia.org/wiki/ANSI_escape_code (which I know is not authoritative).

Are there any terminals supporting this, or would vte be the first (maybe only) one? The latter doesn't make much sense to me. It would be nice to have buy-in from the devs of a few more popular terminals and some apps. I'm personally supportive of ideas that hit a critical mass, but against emulations features that are specific to vte only.

Just for curiosity, what would be your use case for this? Spell checking, maybe? Perhaps you should consider simple underline, or strikethrough, or some foreground or background color.

That's a great resource.  http://invisible-island.net/xterm/ctlseqs/ctlseqs.html is my bible, I didn't realize it was incomplete.  I guess 110 and 111 are the first available codes.

There are not currently any terminals that support undercurl, you've got to start somewhere! :-)  VIM already supports it, the next step is the terminals.  I was talking to Behdad, since I was trying to implement it myself, and he's the one who suggested I file the feature report so that you might take a look at it.  Once VTE supports it, I'm going to add a patch to tmux, and to iterm as well.

One use is spell/grammar checking, but my primary personal use is displaying tool errors inside of VIM.  Underline already has a use (e.g. for links in HTML files).  Undercurl is pretty standard nowadays in  software development tools across the board, it just isn't supported in terminal emulators.

Sounds good.

I encourage you to ask xterm/ncurses developer Thomas Dickey about the 110/111 escape codes, he has a good overview of this area and might point out if it's problematic for some reason. He's a helpful guy and usually responds within a day. (I wonder if we should keep the tradition of difference of 20, and hence also align to multiples of 20, maybe on:121, off:141? Or maybe we can safely piggyback on rapid blink 6/26? I really don't know.) He's also the one who knows what should happen to terminfo/termcap files.

Please also add support to vim's terminal version so that it can actually use it (knows about the escape sequence), and (once settled on spec and implemented in vte/vim) please file feature requests for other popular terminals (xterm, konsole, putty, st, terminology, etc.), at least to bring this new feature into their developers' attention.

Thanks, good luck! :)

Would be really cool to undercurl compiler errors, etc, (say, an undefined variable name...).  But also would be cool to have vte do spell-checking itself.

(In reply to comment #4)
> But also would be cool to have vte do spell-checking itself.

I think it should belong to text editors. Vte's canvas contains plenty of stuff that's not proper text (editor's UI chrome, unix commands output, file names etc.) so you'd see too many things underlined, with probably no practical way to add them to a dictionary.

I mean, I don't mind if someone adds this feature into vte, as long as it can be disabled :D

Another generic thing with undercurl for spell checking purposes: Usually the text is black, while the undercurl is red. In vte the undercurl will be black (well, the same as foreground text color). Unless an exception is made to make it a hardcoded color, or if we introduce a 3rd color per charcell.

(In reply to comment #4)
> Would be really cool to undercurl compiler errors

Just FYI: gcc 4.9 will have built-in support for coloring its output (configurable to use any SGR, pretty much like grep).

As ChPe pointed out, one of the technical problems will be that VteCellAttr is already full (64 bits).

A couple of possible solutions:

- Get rid of blink. It's been broken for years (or maybe never implemented?). (Bug 579964)

- Get rid of invisible. Does anyone use this?

- Get rid of standout, half, reverse or invisible. Instead, adjust the color whenever the cell is written. (This was originally implemented in bug 616436 as approach #1 and then it would have been easy to do, but later with true colors it was changed to the cleaner #2 design.)

- Currently there can be N = approx 2^24+300 colors for fg, same for bg, N^2 < 2^49, so N*fg+bg could be stored in 49 bits. Slow and ugly.

- Currently "bold" and "half" are mutually exclusive. Could "invisible" also be made exclusive to these? (This would probably slightly change the handling of escape sequences, this might be a problem.) Then these could be stored in 2 bits instead of 3.

- Save on "fragment" and "columns":
-- "columns" can go from 1 to 8, correct? So why store in 4 bits instead of 3? (What about dynamic tab positions, and what if even 4 bits are not enough?)
-- "columns" is unused if "fragment" is true, right? These two could be squeezed into one field of 4 bits: value==0 means fragment cell, value>0 means starting cell.
-- "columns" can be greater than 2 only for the tab character, is it true? Then the bold, italic, blink, standout, half, invisible bits are irrelevant so they could be overloaded to encode additional information.

- Steal some bits from the character's Unicode code point. Works when the two go together (in memory most of the time), more problematic with the scrollback storage.

- Live with the growth of the structure size. Sigh. And is not trivial to implement because now it's unioned with an int64.

I'm standing by bug 616436 comment 18 where I've said that we shouldn't remove standout/half/reverse/invisible/blink :-)

Growing the cell by another guint32 seems to me the best solution.

What's print support? Is it about printing to physical paper (bug 441027)? How do you plan to print blinking text? :D Do applications actually use blink? Does anyone really want to see blinking text? (Note that even firefox removed <blink> support recently.)

I think we can at least temporarily remove blink, and defer the 64 bit squeezing problem until someone's actually about to implement it.

If you insist on keeping blink, I'd be in favor of squeezing "columns" and "fragment" together. Unless we can already foresee the next growth in the structure size.

Ok blink isn't important, but the others might be important for printing. By which I mean printin as in bug 441027 not as in bug 613854.

https://en.wikipedia.org/wiki/ANSI_escape_code#graphics lists some more attributes that we might want to support (double underline, overline, Xed-out, circled, framed) in addition to this request for squiggly-underline.

And yes, should talk to the xterm/ncurses maintainer before choosing which escape sequence to use.

We already have Xed-out. The others seem to be rarely supported and even the meaning of the escapes is sometimes ambiguous. I don't think anyone will spend time adding support because someone has already seen a terminal that supported them. It's more going to be demand-driven, like the current one. And they all need a critical mass in terminals and apps that support to make sense. Long story short: I don't think they'll be implemented any time soon. But we'll see.

So has this been discussed with ncurses/xterm maint?

I spoke with Thomas Dickey, and he had a few things to say:
 1) The wiki page is wrong, reserved codes stop at 107, not 109
 2) We shouldn't use spacing between the codes unless we have a specific plan to expand them
 3) Terminfo/cap documentation on new extensions is at http://invisible-island.net/ncurses/terminfo.src.html#toc-_N_C_U_R_S_E_S__U_S_E_R-_D_E_F_I_N_A_B_L_E__C_A_P_A_B_I_L_I_T_I_E_S

(As part of dropping ncurses dependency in bug 728900 the standout and reverse attributes were merged into one, so right now we have 1 free bit in VteCellAttr.)

kitty uses \e[6m for this (supposedly rapid blink, probably noone implements). See https://github.com/kovidgoyal/kitty/blob/master/protocol-extensions.asciidoc.

(They seem to have some other cool stuff as well.)

Kitty does not only add curly underline, it also allows it to have its own color (independently from the cell's foreground color). It's obviously useful for spell checking.

Following some recent refactoring around hyperlinks, VteCellAttr can now easily grow by as many bits as we want, that's no longer a bottleneck. 1 additional bit is no problem at all. However, would we want to dedicate 25 or so bits for this feature??

Is it okay to go with \e[6m (originally "rapid blink")??

I'd rather not re-use a sequence that's been standardised to mean something else, even if it is unused/not implemented in vte.

Undercurl feature request for

     vim: https://github.com/vim/vim/issues/1306
  neovim: https://github.com/neovim/neovim/issues/7479

I guess if either of them implements this, we should too.

(In reply to Egmont Koblinger from comment #17)
> Following some recent refactoring around hyperlinks, VteCellAttr can now
> easily grow by as many bits as we want, that's no longer a bottleneck. 1
> additional bit is no problem at all. However, would we want to dedicate 25
> or so bits for this feature??

Is it really necessary to have the under{line,curl,...} colour per cell, or is a one-for-all colour OSC enough? IMHO that would be good enough. So this would use just 1 bit per cell for the boolean.

(In reply to Christian Persch from comment #20)

> is a one-for-all colour OSC enough?

I'd hate to see that. (I actually hate OSC 4 as well.) No utility should be able to alter the colors previously output by another utility, and no utility should be required to emit such OSC numbers to achieve their desired colors.

There might also be valid cases for multiple colors. E.g. red underline for misspelled words, green underline for stylistic problems.

What I might consider is to do a bit of rounding, e.g. limit to 4 bits per color component. This wouldn't save us much, though. Due to alignment, in RAM we'd be at 12 bytes per cell anyway (as opposed to the current 8), 16 with the character (which is great for the pending SPARC alignment issue), and even without rounding we'd still have 7 or so bits for future features without expanding the size. (We could introduce some rounding if we ever just slightly run out of bits, which is unlikely to happen in the foreseeable future.)

For the scrollback, zeros should compress pretty well. On a side note, default fg/bg colors aren't denoted by the number 0, and following this implementation the default underline color wouldn't be 0 either. We could shuffle around the IDs (and somehow merge DEF_FG and DEF_BG so that they are both 0).

(In reply to Egmont Koblinger from comment #21)
> (In reply to Christian Persch from comment #20)
> 
> > is a one-for-all colour OSC enough?
> 
> I'd hate to see that. (I actually hate OSC 4 as well.) No utility should be
> able to alter the colors previously output by another utility, and no
> utility should be required to emit such OSC numbers to achieve their desired
> colors.

Do you think we should store 'resolved colours' in the cell instead of indices?

> There might also be valid cases for multiple colors. E.g. red underline for
> misspelled words, green underline for stylistic problems.

Ok. Just to clarify, you're proposing that this 3rd colour will be used for all 'line-y' effects like underline, overline, curly underline, strikethrough; not a new individual colour for *each* of these per cell, right? If so, I'm ok with that.

> What I might consider is to do a bit of rounding, e.g. limit to 4 bits per
> color component. This wouldn't save us much, though. Due to alignment, in
> RAM we'd be at 12 bytes per cell anyway (as opposed to the current 8), 16
> with the character (which is great for the pending SPARC alignment issue),
> and even without rounding we'd still have 7 or so bits for future features
> without expanding the size. (We could introduce some rounding if we ever
> just slightly run out of bits, which is unlikely to happen in the
> foreseeable future.)

Let's not do that, at least not until we really have to.
 
> For the scrollback, zeros should compress pretty well. On a side note,
> default fg/bg colors aren't denoted by the number 0, and following this
> implementation the default underline color wouldn't be 0 either. We could
> shuffle around the IDs (and somehow merge DEF_FG and DEF_BG so that they are
> both 0).

Yes, that would be nice (but not required for this bug here).

(In reply to Christian Persch from comment #22)

> Do you think we should store 'resolved colours' in the cell instead of
> indices?

Just as we do with normal fg/bg colors for each cell: If a 256-color index was used then store that index (so a subsequent palette change also effects this color), but if a RGB index was used then store that.

> Ok. Just to clarify, you're proposing that this 3rd colour will be used for
> all 'line-y' effects like underline, overline, curly underline,
> strikethrough; not a new individual colour for *each* of these per cell,
> right? If so, I'm ok with that.

Yes, definitely not one separate for each :-)

Kitty only uses this index for underlining (curly or straight, which are mutually exclusive). See https://github.com/kovidgoyal/kitty/issues/71. I'm tempted not to agree with them and maybe use it for strikethrough (and overline if we'll have it) too.

(In reply to Christian Persch from comment #18)

> I'd rather not re-use a sequence that's been standardised to mean something
> else, even if it is unused/not implemented in vte.

Kitty author is open to changing it, see https://github.com/neovim/neovim/issues/7479. I guess we should come to an agreement with him in the near future.

I don't have a firm opinion here. I wouldn't mind reusing 6 (rapid blink) too much. There are a few other ambiguous ones too (e.g. 3 italic vs standout, 21 not bold vs double underline). 6 is practically not used at all. Or 26 (proportional spacing) is even less used.

Or we could go for the next free slot which is apparently 56, or go for 57 instead because it's adjacent to 58 & 59 that'll change the color so that we pick 3 adjacent numbers.

While we're at it: if we add undercurl, we should probably also add SGR 21 double underline (as xterm has it). That would mean 4 different underline modes (including none), stored in 2 bits.

HTML (for copy-pasting) should go like
<span style="text-decoration: underline single|double|wavy #rrggbb">...</span>
or
<u style="text-decoration-style: ...; text-decoration-color: ...">...</u>

A tiny difference is that in xterm double underline seems to mean a single line of double width, whereas in html it's two lines. Meh.)

Created attachment 364871 [details] [review]
Undercurl, v1

Add undercurl ("\e[4:3m") and double wide underline ("\e[4:2m" or "\e[21m") support. (The concrete escape sequences are still subject to change.)

No coloring yet, that's gonna be another patch.

Created attachment 364875 [details] [review]
Colored underline, v1

This patch adds support for colored underlines (using SGR 58 and 59 for the time being). Goes on top of the previous patch.

I'm yet to double check everything, especially all weird combinations of reverse, concealed, mouse highlight, preedit etc... but at first glimpse it seems to be okay. Also not sure if it's okay to piggyback on VTE_DEFAULT_FG or should introduce a new constant.

TODO: HTML

Test with e.g.
echo -e '\e[4:3;58:5:200mfoo\e[36mbar\e[59mbaz\e[39mquux'

Created attachment 364954 [details] [review]
Undercurl, v2

Ported to current git (line spacing).
Supports 4:0.
Better vertical positioning of the undercurl and hopefully cleaner code computing that.

Created attachment 364955 [details] [review]
Colored underline, v2

Ported to current git (line spacing).
HTML colored underline done.
(Still goes on top of the corresponding Undercurl patch.)

*** Bug 731151 has been marked as a duplicate of this bug. ***

Created attachment 365066 [details] [review]
Undercurl, v3

Changed the double underline to be two separate underlines, rather than a thicker one.

This goes unlike xterm, but follows mlterm (as well as HTML).

I believe this approach is more usable, as it allows applications to assign different semantics for single and double underlines. (If double meant thick, the user probably couldn't be fully confident which thickness is being used without also seeing the other one somewhere on the screen.)

Created attachment 365072 [details] [review]
Colored underline, v3

Minor fixes.

Christian, could you please review the patches? I think they're pretty much okay.

The thing I dislike the most is the naming: "underline" for the mode and "undl" for the color.

For the latter I went for a four-letter one to match "fore" and "back" although this might be a stupid idea, and "undl" definitely looks silly. Could be perhaps "deco" which stands for _deco_ration color, or _de_coration _co_lor. This would match HTML's naming, although unlike HTML, we only apply it for the underline and not for strikethrough.

Another possibility is to go overly verbose, e.g. "underline_mode" and "underline_color", not sure if this would improve readability, probably not that much.

Yet another tiny but important detail:

When the text (letters with descent) crosses the underline of a different color (e.g. spellchecker's red undercurl), I believe legibility of the text is more important, hence its color should win. This is what LibreOffice Writer, FireFox and Kitty do too.

In draw_cells(), the call to _vte_draw_text() should be moved from the middle to the bottom, that is, after drawing the underline or other decoration.

Can you think of any negative side effects this change could introduce? (I can't.)

Created attachment 365099 [details]
Test script

Here's a hopefully useful test script, to be included under the (badly named) perf directory.

(See also bug 791303.)

Created attachment 365153 [details] [review]
Undercurl, v4

Minor cleanups. Add tracking comments to too many FALSEs passed to a method.

Created attachment 365154 [details] [review]
Colored underline, v4

Renamed undl to deco, I like it much more. Swapped the order of drawing the decoration and the text, so that the text's color wins.

Created attachment 365164 [details]
Test script, v2

Much improved version.

Time for me to stop before it becomes overengineered (if it hasn't already). :-)

First patch:

+const char *styles[] = {"", "single", "double", "wavy"};

Make that |static const char styles[][7] = { ... }|.

+m_double_underline_position = MIN (char_spacing.top + char_ascent + m_line_thickness, cell_height - 3 * m_line_thickness);
+m_double_underline_thickness = m_line_thickness;

I'd swap these two lines, and then refer to m_double_underline_thickness when computing m_double_underline_position.

So when there's not enough space, this will cause the double underline to be within the ascent. Could try shrinking m_double_underline_thickness until it fits, and if it still doesn't fit at 1px, fuse the underlines to get a 2px thick double-underline.  I'm not insiting to do this right now, I'm fine with adding just a short FIXME comment here for this :-)

+if (underline == 1 || underline == 2) {

Maybe use a |switch (underline)| here? And perhaps introduce an enum for the values?

+ for (int j = 0; j < underline; j++, position += 2 * thickness) {

There's a signed-unsigned compare warning on this line.

+_vte_draw_draw_undercurl(...

Not finding a fault with this :-) but this looks rather complicated and also kinda expensive to draw. LO does 'wavy' underline as a saw-tooth (albeit with rounded peaks it appears); could we just do a simple saw-tooth?  Just adding a comment is fine; we'd need to profile this to see if it's a significant problem. => defer to later :-)



Patch 2:

I like the choice of 'deco' for the colour variable :-)

+        /* Draw whatever SFX are required. Do this before drawing the letters,
+         * so that if the descent of a letter crosses an underline of a different color,
+         * it's the letter's color that wins. */

Hmm...

(In reply to Egmont Koblinger from comment #32)
> When the text (letters with descent) crosses the underline of a different
> color (e.g. spellchecker's red undercurl), I believe legibility of the text
> is more important, hence its color should win. This is what LibreOffice
> Writer, FireFox and Kitty do too.

I have only checked LO, but with my version at least, the underline is over the descenders, not under it. Having witnessed that effect, I do agree that we want to draw the underline below the text, not above it.

Same probably applies to overline. However IMHO for strikethrough, I think the 'not perfectly legible text' effect is actually what is wanted.  Luckily we use the same colour for strikethrough as for the text itself, so drawing order doesn't make a difference. Just mention that in the comment?


With these nits fixed (or argued away/against :-) this is fine to commit, not necessary to attach a new set of patches. Thanks!!

(In reply to Egmont Koblinger from comment #21)
> What I might consider is to do a bit of rounding, e.g. limit to 4 bits per
> color component. 

I'm acutally in favour of limiting this; giving a 16x16x16 colour cube (plus indexed colours) for this effect should be plenty. That means we use only 2x25 bits for the fg+bg, plus 13 bits for the deco colour; all in all 63 bits which fits neatly within a guint64 with 1 bit to spare. I'll try this in a follow-up patch, unless you want to cook it up yourself.

(In reply to Christian Persch from comment #37)

> +m_double_underline_position = MIN (char_spacing.top + char_ascent +
> m_line_thickness, cell_height - 3 * m_line_thickness);
> +m_double_underline_thickness = m_line_thickness;
> 
> I'd swap these two lines, and then refer to m_double_underline_thickness
> when computing m_double_underline_position.

I was thinking about it too, not just for this but for all kinds of underlines.

> So when there's not enough space, this will cause the double underline to be
> within the ascent. Could try shrinking m_double_underline_thickness until it
> fits, and if it still doesn't fit at 1px, fuse the underlines to get a 2px
> thick double-underline.  I'm not insiting to do this right now, I'm fine
> with adding just a short FIXME comment here for this :-)

... And in the mean time take care to adjust the single underline's thickness as well, lest the double one becomes just as thin, or who knows, maybe even thinner :-) It's pretty cumbersome. Double underline looks quite nice on my font size, so it's low prio for me. I'll add a todo.

> +if (underline == 1 || underline == 2) {
> 
> Maybe use a |switch (underline)| here? And perhaps introduce an enum for the
> values?

A switch sounds okay. Could even move case 1 and case 2 into two separate branches, that'd be a better design for the fused double underline mentioned above, and case 2 would unroll the loop, ...

> + for (int j = 0; j < underline; j++, position += 2 * thickness) {
> 
> There's a signed-unsigned compare warning on this line.

... eliminating this warning.

> +_vte_draw_draw_undercurl(...
> 
> Not finding a fault with this :-) but this looks rather complicated and also
> kinda expensive to draw. LO does 'wavy' underline as a saw-tooth (albeit
> with rounded peaks it appears); could we just do a simple saw-tooth?  Just
> adding a comment is fine; we'd need to profile this to see if it's a
> significant problem. => defer to later :-)

I find this curly underline so beautiful that I wouldn't want to switch to a saw-tooth. Let alone that one would be taller, interfering more with the letters.

Performance is indeed somewhat an issue. If I make my shell prompt wavy and hold Enter, sometimes it skips a line (which doesn't happen in normal circumstances). Luckily it's not terrible, and probably good enough for forthcoming practical uses (e.g. spell checker underlining a small percentage of the onscreen text).

What I had in mind instead: Somehow cache the rendered version of this underline (as a bitmap). Maybe in the last 3 or so different colors seen, or (not sure if it's possible) cache in a color-independent way that can quickly be colored on demand. In the latter case the cached glyph would remain valid until the font changes. I'll add a TODO / file a bug.

> I have only checked LO, but with my version at least, the underline is over
> the descenders, not under it. Having witnessed that effect, I do agree that
> we want to draw the underline below the text, not above it.

An even nicer version would be if the underline stopped slightly before it's about to cross a letter. See "text-decoration-skip: ink". I vaguely recall having seen an iTerm2 screenshot doing this.

(In reply to Christian Persch from comment #38)

> I'm acutally in favour of limiting this; giving a 16x16x16 colour cube (plus
> indexed colours) for this effect should be plenty. That means we use only
> 2x25 bits for the fg+bg, plus 13 bits for the deco colour; all in all 63
> bits which fits neatly within a guint64 with 1 bit to spare. I'll try this
> in a follow-up patch, unless you want to cook it up yourself.

I don't see too much point in this _right now_.

This could squeeze the 16 in-memory bytes to 14 with attribute packed, probably resulting in a more significant slowdown than win in memory footprint. (We had some rough numbers in the sparc bug 781499.)

Or maybe back to 12 bytes if you introduce a low limit on the hyperlinks, but then it's prone to overflow in case of malicious software producing malicious data (all cells having a different target). Assuming we'll implement overline, we'd have 18 bits (262144) for hyperlink and then no spare bit left. Implement italic vs oblique and it's 17. Implement dotted/dashed underline and it's 16. Implement double/wavy/dotted/dashed strikethrough/overline...

Currently the structure is chosen so that no field crosses a 4-byte boundary, we believe it's good for performance (not sure if we've ever tested). With the aforementioned packing this couldn't be possible. It'd also probably require heavy refactorings to read the underline color (which could the one crossing that boundary) only when needed.

Bitpacking from RRGGBB to RGB and vice versa would be easy and fun, though :)

I personally feel safe and calm with the current nicely aligned 16-byte structure having quite a few spare bits, and would only start doing this whenever we're running out of these bits.

(In reply to Christian Persch from comment #37)

> this looks [...] kinda expensive to draw

Creating the two arcs and doing everything else except for the final cairo_stroke() takes (depending on the font size) 6-10 µs on average. (This is then repeated for each undercurled cell.)

The final step, cairo_stroke() takes an average of about 35-100 µs. (The ballpark of 100 µs is with huge fonts where the entire fullscreen window is around 80x24 characters. For "sane" font sizes it's more like in the 35-50 range.)

---

After about 2 minutes of studying cairo docs, I think this is what we should do:

Instead of painting the wave on our actual canvas, paint it on some in-memory surface or pattern (not clear yet which of these two) where the rest of the surface/pattern is transparent. Then, when it gets to actually painting a colored wave, just paint the solid color using this cached surface/pattern as cairo_mask.

Sounds like a good exercise.

(For measuring the time spent in cairo_stroke(), I omitted low values (below 10 µs) from the averate calculation. They occur e.g. when a cell above a curly one is invalidated, and hence so is the top part of the undercurled. The entire undercurled cell is repainted, but the curl itself is outside of the clip region.)

(Timings measured on Intel Core i5-6200U @ 2.3GHz)

Created attachment 365216 [details] [review]
Undercurl caching, 1

On top of the v4 patches: This one caches the shape of the undercurl.

Note to my future self which will forget cairo's basics: After a bit of preparation work (the caching itself), painting basically follows the same method both without and with caching. The desired shape is the "mask", and the source is a solid color. The difference is that without caching the mask is a computational-heavy set of arcs + stroke width, whereas with caching involved it's a bitmap (actually alpha channels only).

The time it takes to print an undercurl is now in the ballpark of 20 µs. Holding Enter with my prompt being undercurled still skips a line every now and then, but much less frequently than before.

For some weird reason, the poor man's timing I had (diffing g_get_monotonic_time() before and after) gives huge values (thousands of µs's) significantly more often than without caching, but only if a big region is invalidated/repainted at once. I'm almost certain that it's the kernel's context switch or something like this kicking in, and not CPU burnt by cairo. Would need to set up some more proper profiling, so currently the numbers I provided are very rough.

The real core business with caching the look and then using this mask is pretty simple straightforward cairo code.

The rest is quite cumbersome and I don't really like it. I'll think on possibilities to clean up a bit, and ideas are also welcome.

Whose responsibility should it be to cache the look (e.g. cache upfront when a font is set, or only when actually used)? Who should invalidate the cache (should it happen when a font is set, or only when an undercurl is about to get printed again)? In the latter case (as I did now), the drawing method needs to check quite a few parameters to see if they changed.

There's also an x offset necessary since the undercurl overflows horizontally from the cell which shouldn't be cropped, that'd look ugly. This offset makes the code harder to read. I've also introduced the concept of y offset (with opposite sign, ouch) so that the mask is as small as possible, only includes the undercurl itself and not the vertical space above it. Plus there's doubles, floor/ceil, whatnot, ouch...

Just clearing the undercurl cache in apply_font_metrics() (if any of the relevant parameters has changed), and on unrealize, is the right place, I think, and in _vte_draw_draw_undercurl() create on first use as you already do in this patch.

+        cairo_surface_t *undercurl_surface;

Should make that a cairo_pattern_t* undercurl_pattern.

+        cairo_t *undercurl_cr;

You don't need to cache the cairo_t here.

+        gint undercurl_width, undercurl_thickness;
+        gdouble undercurl_pos;

I don't see why you need these other parameters? The only thing you need to know when to draw the undercurl is the offset between m_undercurl_position and the origin of the cached surface, which is fixed? I see m_undercurl_position is a double, but you could make that an integer, and only computing the exact value when drawing the undercurl to the cache surface pattern.

+        if (G_UNLIKELY (width != draw->undercurl_width ||
+                        !_vte_double_equal(pos, draw->undercurl_pos) ||
+                        line_width != draw->undercurl_thickness)) {

...and then you also only need to check for G_UNLIKELY(draw->undercurl_pattern == nullptr) here, since the other parameters are only used to draw the cache pattern and the cache is cleared when they change in apply_font_metrics().

@@ -788,6 +796,9 @@ _vte_draw_free (struct _vte_draw *draw)
+        cairo_surface_destroy (draw->undercurl_surface);

Needs a nullptr check.

+        cairo_destroy (draw->undercurl_cr);

Move that into _vte_draw_draw_undercurl() at the end of the if() { } block drawing the cache surface.

@@ -1677,28 +1688,58 @@ _vte_draw_get_undercurl_height(gint width, int line_width)
+                cairo_surface_destroy (draw->undercurl_surface);

nullptr check.

+                draw->undercurl_surface = cairo_image_surface_create (CAIRO_FORMAT_A8,

Should gdk_window_create_similar_surface() to create a surface for the gtk_widget_window() of the terminal. (Needs to pass that down to this function as param.)

When drawing two adjacent cells with undercurl using the cache, is this continuous? You could make the surface contain just 1 (or a small N) times the fundamental unit of the pattern, and set extend to CAIRO_EXTEND_REPEAT, and draw using a clip.

        cairo_set_operator (draw->cr, CAIRO_OPERATOR_OVER);
        cairo_new_sub_path(draw->cr);

I don't think you need that new_sub_path() here anymore.

+        cairo_mask_surface (draw->cr, draw->undercurl_surface, x - line_width, y + pos_int);

Here you'd cairo_save(), clip, cairo_mask(cr, pattern,...); cairo_restore().

Hmm that was a lot comments for such a small patch. Sorry! :-) :-)

(In reply to Christian Persch from comment #44)

> Hmm that was a lot comments for such a small patch. Sorry! :-) :-)

No worries, I knew it would be a problematic patch, at least in its current state :-) I was just happy to get it working at the first place.

I'll submit the non-caching version first, and then continue working on this on top of that, if that sounds good. (I would've combined them to a single patch if caching was clean and straightforward, which apparently it isn't.)

Support for curly and colored underlines, and the test script are now submitted.

Optimization is yet to be finished.

Created attachment 365421 [details]
screenshot: without new_sub_path()

> I don't think you need that new_sub_path() here anymore.

I actually do need it in the current state of things, see the screenshot. (Will check if it's still needed after all your propsed modifications.)

Just wondering: isn't it a bug sowewhere else (e.g. where Pango draws that box with hex characters) that it doesn't close its path?

That's weird... could be just a missing cairo_move_to() in draw_undercurl? (Could still be a pango problem *too*, but _pango_cairo_renderer_draw_unknown_glyph does save/restore, so...)

(Does this look like the same problem as in https://bugzilla.redhat.com/show_bug.cgi?id=1523007 (pre-undercurl)?)

(In reply to Christian Persch from comment #48)

> (Does this look like the same problem as in
> https://bugzilla.redhat.com/show_bug.cgi?id=1523007 (pre-undercurl)?)

I've been thinking a lot about that bug. I don't think it's relevant. Posting a comment over there soon.

(In reply to Christian Persch from comment #48)

> could be just a missing cairo_move_to() in draw_undercurl?

Apparently you're right, this change:
-        cairo_new_sub_path(draw->cr);
+        cairo_move_to (draw->cr, x, yc);
also works correctly, and is probably the right way.

(In reply to Christian Persch from comment #44)

Attaching "Undercurl caching, v2" shortly.

> +        cairo_surface_t *undercurl_surface;
> 
> Should make that a cairo_pattern_t* undercurl_pattern.

Not yet done; just wondering: why is a pattern better than a surface for this purpose?

> Should gdk_window_create_similar_surface() to create a surface for the
> gtk_widget_window() of the terminal. (Needs to pass that down to this
> function as param.)

Actually I've found a gdk-unaware cairo_surface_create_similar() so that I can create one similar to draw->cr without passing a new param. I assume draw->cr was created as "similar" to the main window.

> When drawing two adjacent cells with undercurl using the cache, is this
> continuous? [...]

Yup, now it is continuous. That's because the line ends at 45°, a bit of its tip overflows to the next cell, but just as much of the current cell remains unpainted (the next cell overflows here). This requires that the surface is a bit wider than cell_width, to contain this overflow too.

> [...] You could make the surface contain just 1 (or a small N) times
> the fundamental unit of the pattern, and set extend to CAIRO_EXTEND_REPEAT,
> and draw using a clip.

Clipping would clip vertically which presumably would look uglier. (This is also the main reason why I'm not sure how going for a "pattern" rather than a surface could help.)

> Here you'd cairo_save(), clip, cairo_mask(cr, pattern,...); cairo_restore().

The save/restore pair seems to be unnecessary, but I've added it (cannot hurt).

I believe all your other comments are addressed. The patch is much nicer than the previous one :-) but I'll do another round of tiny improvements, and look at the surface vs pattern story.

Also, I changed undercurl_thickness from long to double because why not.

It's quite fast now; I haven't measured, but tapped on Enter with an undercurled prompt, and could no longer cause skipping lines.

---

Truly insignificant nitpicking, but technically interesting:

The previous patch passed along two separate parameters: the y coordinate of the cell (int), and the y offset of the undercurl within the cell (double). The new one simplifies this to just one y parameter (the sum of these two). I've added comments explaining that the caching mechanism assumes that the fractional part of y is always the same (we don't re-antialias for sub-pixel vertical movements), only the integer part of y changes.

Technically, strictly speaking this is not exactly true, since double loses from its precision at every power-of-two pixels from the top. So if someone examines each and every pixel's RGB in the undercurl, they might (with a very small chance) find that there's a bit of difference depending whether the first undercurl was painted near the top or near the bottom of the canvas. We might just totally ignore it (this wouldn't be compatible with the previous patch comparing doubles to check for cache validity, which I no longer do so we're fine) or might revert to passing 2 separate parameters.

Created attachment 365605 [details] [review]
Undercurl caching, v2

I thought a bit more about the probablity of a difference in the binary fractional digit #fourty-something causing a different rendering... let's ignore it :)

---

I looked at the cairo_pattern docs.

If we wanted to change the look so that the undercurl ends in a vertical cut exactly at cell boundary (which I don't want to, I believe our current look is nicer) then we could reasonably use cairo's pattern feature with its built-in repetition CAIRO_EXTEND_REPEAT. We'd need to create the surface just as we do now, then create a pattern for it, set it to be repeated, set up a transformation matrix, set up the clipping rectangle when applying, etc.

On the other hand, we could rework our internal API so that _vte_draw_draw_undercurl() paints the entire undercurl run in a single cairo step, rather than being invoked over and over again for each cell.

This would result in a bit more complex code, that's not what I'm primarly worried about, but the uglier look at the end of the lines.

If we wish to keep the current, overflown ending for lines (I do) then cairo's pattern repetition feature isn't useful for us (unless we handle the left and right edges separately which is too cumbersome). All the hassle of creating a pattern and setting up an offset in the transformation matrix is conveniently done by cairo_mask_surface() so that we don't have to explicitly refer to a pattern anywhere (this is what I do now), see https://cgit.freedesktop.org/cairo/tree/src/cairo.c?h=1.14.12#n2072.

(We could still modify _vte_draw_draw_undercurl()'s API so that it draws a long run of undercurl, moving the for-loop from outside of calling this function to inside. That'll save us a couple of cairo calls and give a more consistent API with the other similar methods. I guess it's a good idea.)

If you paint the undercurl for each cell individually, you don't need to bother with the pattern; it would just simplify doing longer runs (with the extend setting). So let's just keep using the surface.

BTW, have you checked the undercurl works correctly on wide cells (2 columns) and tab cells (8 columns) ?

(In reply to Egmont Koblinger from comment #53)
> (We could still modify _vte_draw_draw_undercurl()'s API so that it draws a
> long run of undercurl, moving the for-loop from outside of calling this
> function to inside. That'll save us a couple of cairo calls and give a more
> consistent API with the other similar methods. I guess it's a good idea.)

Yes.

Created attachment 365608 [details] [review]
Undercurl caching, v3

Moved the loop as discussed.
Removed the width parameter as it's already available as draw->cell_width.
Fixed a leftover "line_width" instead of "x_padding" (not sure if maybe x_margin or x_overflow or whatever would be a better name).

Perhaps the assignment to y_bottom, y_center, y_int, y_bottom_int could be clarified a bit, I don't really like that small part; otherwise I'm pretty happy with the current state.

> BTW, have you checked the undercurl works correctly on wide cells (2 columns) and tab cells (8 columns) ?

Yup. CJK receives two periods.

A TAB cannot have underline/strikethru/etc. properties, only bgcolor. TAB is a control character to move the cursor, only under special circumstances (there's nothing yet beyond the cursor position when the TAB is encountered) can we special-case it for copy-paste purposes. In this case it might receive a nondefault background (due to bce), but not other nondefault attributes. If TAB skips over existing characters, their properties (incl, undercurl) are preserved.

Created attachment 365646 [details] [review]
Undercurl caching, v3.1

> Perhaps the assignment to y_bottom, y_center, y_int, y_bottom_int could be
> clarified a bit, I don't really like that small part

Minor changes here.

Christian, please take a look.

Comment on attachment 365646 [details] [review]
Undercurl caching, v3.1

Looks good now, thanks!

Everything's finished here I think, so closing.

For future reference: As per https://github.com/kovidgoyal/kitty/issues/226#issuecomment-381665023, mintty decided that 4:4 is dotted underline, and 4:5 is dashed.

That's also the exact same what hterm is using. 

Maybe we should have a shared document somewhere that lists these common extensions...

Apologies for necro.

> Maybe we should have a shared document somewhere that lists these common extensions...

mintty has some documentation for what it supports on that note.[0] VTE probably can use a text file in docs/ too.

  [0]: https://github.com/mintty/mintty/wiki/Tips#text-attributes-and-rendering