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

Add support for dotted/dashed underline styles #4529

Merged
merged 6 commits into from
Jan 18, 2022

Conversation

jcla1
Copy link
Contributor

@jcla1 jcla1 commented Jan 17, 2022

As has been a proposal for a long time the SGR escape codes \e[4:4 and \e[4:5 could be used to denote dotted and dashed underline styles respectively. (Previously, using these escape codes lead to undercurled text.)

This patch adds support for both. The dashed style looks good regardless of conditions (font-size), but the dotted style only looks entirely correct on certain font-sizes. Here's an example

Screenshot from 2022-01-17 13-24-06

The reason the dotted underline only looks good on certain font-sizes is: the underline styles are rendered on a per-cell basis (and then just copy-repeated if multiple letters next to each other should be underlined). This leads to an inability to fill the space under the letters with an equal spaced sequence of dots of thickness width. Here's an example screenshot where it doesn't look ideal:

Screenshot from 2022-01-17 13-35-49

(Notice that, sometimes, between two letters the dots are joined to one wide dot.)

In `Colored and styled underlines` it's proposed that the SGR codes
\e[4:4m and \e[4:5m are used to add a dotted or dashed underline to the
rendering context respectively. This commit prepares the necessary
changes to add the two additional underline style, while still rendering
them as a normal underline and curly underline.
Dashed underline looks pretty good regardless of conditions, but the
dotted underline only looks good/correct on certain font-sizes. This is
due to the underline being rendered on a per cell/glyph basis (so one
can not place a dot directly between two letters, say. Could be remedied
by pulling the rendering of the underlines into the shader, but that is
more work.
@kovidgoyal
Copy link
Owner

Looks good in general, some comments:

  1. It should be possible to distribute the dots evenly, you can see an example of doing that with braille dots in the function distribute_dots() in box_drawing.py. The idea is to place the dots evenly initially, if that leaves extra space after the last dot, distribute that space among all the inter dot gaps to minimize the effect. The final distribution of the dots will be

gap/2 dot gap dot gap dot gap dot gap/2

  1. Edit docs/underlines.rst to remove the note that kitty does not support dashed and dotted underlines.

@kovidgoyal
Copy link
Owner

And of course you need to modify some tests.

@jcla1
Copy link
Contributor Author

jcla1 commented Jan 17, 2022

Thanks for the feedback! Yeah, I was expecting some tests to fail (it was hard enough to get the build working on my machine, so I didn't bother with the tests at the time).

I'll fix the tests & docu and also have a look at distribute_dots(), but I'm skeptical that what I'm imagining is possible.

Previously, because of the new underline styles a couple of tests were
failing due to an unexpected number of sprites being returned from the
test-set-up. No new tests were added.
Also make more clear *what* exactly is rendered in the cell (i.e. a
strikethrough).
@jcla1
Copy link
Contributor Author

jcla1 commented Jan 18, 2022

So I've edited the test and none of them should fail now. Beware though that I'm not entirely sure that the two test-cases I've added are actually testing what I think they should be.

I've also updated the documentation.

Lastly, I've had a look at distribute_dots() and, as was to be expected, it does not solve the problem -- only change it to a different one:

Screenshot from 2022-01-18 13-24-02

(Notice how now the dots, instead of overlapping, are spaced too far apart.)
The reason equal spacing is impossible, with the current cell-based drawing of the underlines, is this: Say the cell_width is a prime number, like 23. There is no way to evenly space out dots of size 1, 2 or 3, because that would need a divisor for the cell_width (of which there are none)!

Below is the patch with which I've generated the above image...but as I've said above: the problem still persists.

diff --git a/kitty/fonts/render.py b/kitty/fonts/render.py
index cee46aa9..2e3aa776 100644
--- a/kitty/fonts/render.py
+++ b/kitty/fonts/render.py
@@ -17,7 +17,7 @@
     test_render_line, test_shape
 )
 from kitty.fonts.box_drawing import (
-    BufType, render_box_char, render_missing_glyph
+    BufType, distribute_dots, render_box_char, render_missing_glyph
 )
 from kitty.options.types import Options, defaults
 from kitty.typing import CoreTextFont, FontConfigPattern
@@ -280,10 +280,12 @@ def add_intensity(x: int, y: int, val: int) -> None:
 
 
 def add_dots(buf: CBufType, cell_width: int, position: int, thickness: int, cell_height: int) -> None:
+    spacing, size = distribute_dots(cell_width, cell_width // (2 * thickness))
+
     y = 1 + position - thickness // 2
     for i in range(y, min(y + thickness, cell_height)):
-        for j in range(0, cell_width, 2 * thickness):
-            buf[cell_width * i + j:cell_width * i + min(j + thickness, cell_width)] = [255] * min(thickness, cell_width - j)
+        for j, s in enumerate(spacing):
+            buf[cell_width * i + j * size + s: cell_width * i + (j + 1) * size + s] = [255] * size
 
 
 def add_dashes(buf: CBufType, cell_width: int, position: int, thickness: int, cell_height: int) -> None:

@kovidgoyal
Copy link
Owner

I didnt say you could distribute space equally, I said you could minimize the disruption by distributing the extra space between all the gaps.

@jcla1
Copy link
Contributor Author

jcla1 commented Jan 18, 2022

So would you rather I include the patch to use distribute_dots()? (I didn't previously as I felt there was no gain over the 'constant width & spacing at the cost of regular overlap' method.)

Besides that, is there anything else that needs to be done before merging?

@kovidgoyal
Copy link
Owner

Yes include the patch and no there is nothing else to be done.

@kovidgoyal kovidgoyal merged commit 17a3be8 into kovidgoyal:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants