Add proper underline and strikethrough support

[chrisduerr]

Support for strikethrough has been added by inserting and removing a
STRIKE_THROUGH flag on the cell.

Now all strikethrough and underline drawing is also done through the
rectangle renderer. So no glyphs are used to render underlines and
strikethrough.
The position is taken from the font metrics and should be accurate for
linux, however is not yet tested on macos.

It works by checking the underline state for each cell and then drawing
from the start until the last position whenever an underline ended. This
adds a few checks even if no underline is rendered but I was not able to
measure any significant performance impact.

Fixes #806.
Fixes #31.

Demo:

TODO:

• Account for font and glyph offsets
• Account for window padding
• Test (and potentially fix) DPI
• Refactor code and extract it into separate files
• Add Windows support

[chrisduerr]

Here are the results of the benchmarks I've run:

Master

Benchmark #1: alt-screen-random-write
  Time (mean ± σ):      5.433 s ±  0.032 s    [User: 5.200 s, System: 1.888 s]
  Range (min … max):    5.381 s …  5.476 s
Benchmark #1: scrolling
  Time (mean ± σ):      5.562 s ±  0.087 s    [User: 5.419 s, System: 5.623 s]
  Range (min … max):    5.415 s …  5.714 s
Benchmark #1: scrolling-in-region
  Time (mean ± σ):      5.533 s ±  0.062 s    [User: 5.359 s, System: 5.638 s]
  Range (min … max):    5.399 s …  5.615 s
Benchmark #1: startup
  Time (mean ± σ):      85.7 ms ±   2.2 ms    [User: 73.7 ms, System: 15.0 ms]
  Range (min … max):    83.0 ms …  91.0 ms
Benchmark #1: unicode-random-write
  Time (mean ± σ):      5.494 s ±  0.649 s    [User: 5.469 s, System: 0.155 s]
  Range (min … max):    4.476 s …  6.671 s

Underline + Strikethrough

Benchmark #1: alt-screen-random-write
  Time (mean ± σ):      5.409 s ±  0.015 s    [User: 5.164 s, System: 1.930 s]
  Range (min … max):    5.382 s …  5.431 s
Benchmark #1: scrolling
  Time (mean ± σ):      5.516 s ±  0.064 s    [User: 5.350 s, System: 5.623 s]
  Range (min … max):    5.432 s …  5.615 s
Benchmark #1: scrolling-in-region
  Time (mean ± σ):      5.528 s ±  0.072 s    [User: 5.364 s, System: 5.614 s]
  Range (min … max):    5.432 s …  5.665 s
Benchmark #1: startup
  Time (mean ± σ):      85.3 ms ±   1.9 ms    [User: 72.8 ms, System: 16.0 ms]
  Range (min … max):    82.0 ms …  89.6 ms
Benchmark #1: unicode-random-write
  Time (mean ± σ):      5.836 s ±  0.454 s    [User: 5.804 s, System: 0.175 s]
  Range (min … max):    5.032 s …  6.367 s

Benchmark Script

#!/bin/bash

# Read the command line arguments
if [[ "$#" -lt 2 ]]; then
    echo "Usage: bench.sh <TERM> <OUT_DIR>"
    exit 1
fi
term="$1"
dir="$2"
echo "Benchmarking '$1' and writing results to '$2'"
echo ""

# Install hyperfine benchmarker
echo "Searching for hyperfine"
if hash hyperfine 2>/dev/null; then
    echo "Found existing hyperfine install"
else
    echo "Installing hyperfine"
    cargo install hyperfine
fi
echo ""

# Create directory for the benchmarks
if [ -d "$dir" ] || [ -f "$dir" ]; then
    echo "Removing existing output directory"
    rm -rf "$dir"
fi
echo "Creating new output directory"
mkdir "$dir"
echo ""

# Generate the benchmarks, aiming for ~5 secs
echo "Generating benchmarks"
echo "Generating alt-screen-random-write benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 599999999 alt-screen-random-write > "$dir/alt-screen-random-write.vte"
echo "Generating scrolling benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 50000000 scrolling > "$dir/scrolling.vte"
echo "Generating scrolling-in-region benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 50000000 scrolling-in-region > "$dir/scrolling-in-region.vte"
echo "Generating unicode-random-write benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 800000 unicode-random-write > "$dir/unicode-random-write.vte"
echo ""

# Run the benchmarks
echo "Running benchmarks"
echo "Running startup benchmark"
hyperfine --style 'basic' "$term -e true" > "$dir/startup.bench"
echo "Running alt-screen-random-write benchmark"
hyperfine --style 'basic' "$term -e cat $dir/alt-screen-random-write.vte" > "$dir/alt-screen-random-write.bench"
echo "Running scrolling benchmark"
hyperfine --style 'basic' "$term -e cat $dir/scrolling.vte" > "$dir/scrolling.bench"
echo "Running scrolling-in-region benchmark"
hyperfine --style 'basic' "$term -e cat $dir/scrolling-in-region.vte" > "$dir/scrolling-in-region.bench"
echo "Running unicode-random-write benchmark"
hyperfine --style 'basic' "$term -e cat $dir/unicode-random-write.vte" > "$dir/unicode-random-write.bench"
echo ""

echo "All benchmarks completed successfully"
echo "You can find the results in $dir/*.bench"

All benchmarks with color support used a small patch which randomly generates SGR sequences instead of just changing color so underline and strikethrough is actually tested.

[maximbaz]

As usual for your PRs, super awesome!

I tested my font (Consolas) as well as another one that I like (Source Code Pro), I have two comments/questions:

1. Underline seems to "jump vertically" depending on a font, wouldn't it be visually nicer to have it exactly in line with the bottom part of block cursor? Have a look at "Consolas - PR", the underline is so much away from the text that it even feels closer to the letters "ikth" of the word "strikethrough" below.

2. The strikethrough like feels too high in "Source Code Pro - PR" example, especially comparing to "Consolas - PR" or the image you showed in your demo.

IMO LibreOffice renders both fonts more beautifully, underline is closer to the text in Consolas and strikethrough is centered in Source Code Pro.

Closer comparison:

[chrisduerr]

For the underline I'm taking the metrics that are provided by the font itself, so those should be okay in theory.

However with the strikethrough I'm just taking the cell height and applying strike-through half way through the cell. It might be a better approach to take the font baseline and the ascent and drawing the strikethrough half way through there.

Thanks for the quick feedback. :)

EDIT: What I haven't thought about is the actual alignment of the underline with a width that is not 1. It looks to me like the Consolas underline is more than one pixel wide. Right now I just assumed that the position of the underline is the top corner of it. However if it would be the middle or even the bottom the underline of Consolas would be higher. I'll look into that again, it's probably somewhere in the freetype documentation.

[chrisduerr]

According to the docs:
When displaying or rendering underlined text, this value corresponds to the vertical position, relative to the baseline, of the underline bar's center.

So I'm currently still doing it wrong because I'm positioning the underline relative to the baseline, of the underline bar's top. Not relative to the baseline, of the underline bar's center.
This should help moving the underline in the Consolas example from @maximbaz a bit up.

I can't find any specifics on strikethrough though.

[chrisduerr]

After looking a bit more into it, it seems like there is no "standard" way for rendering the strikethrough. So where exactly the strikethrough line is placed seems to vary significantly from terminal to terminal. From the ones I've looked at none seems perfect.

Another thing I've noticed is that the underline position and thickness are always the same, no matter what font size. I'm not entirely sure if that's correct, because fonts with a huge underline offset (Hack has -4) will lead to the underline leaving the glyph box at small sizes.

[maximbaz]

Huh, interesting discovery about the underline position - to illustrate (Consolas):

[maximbaz]

The underline raised a little higher with the latest commit, do you have some ideas about how to fix the underline being so low when the font size is very small? I'm just curious, given the benefits and that it works for usual font sizes I personally think it's fine to merge this even in the current state.

This is font size of 4, Consolas:

10 (my regular font size):

12 (too large for me, but look how the position of underline perfectly aligns with the cursor):

[chrisduerr]

I'll be honest here, the state this PR is in right now is pretty much completely unusable with some font/size combinations. And I have no idea how exactly freetype underline and position is supposed to work.

I'll have to look more into drawing underline with freetype, maybe check out how some other applications do it. But as long as underline position/width is always the same with all font sizes, this will not work properly.

[chrisduerr]

Apparently the undreline position and thickness are specified in font units, not device pixels, so they need to be converted first.

There also is an exact specification for strikethrough position and thickness using the freetype::tt_os2::TrueTypeOS2Table struct. This gives us access to the y_strikeout_size and y_strikeout_position methods.

I'll update this PR with a reworked version of this as soon as possible, so others can test it with their font setup.

[chrisduerr]

Underline has been reworked, however there's still one small caveat:

To make sure the underline doesn't disappear with small fonts, the
minimum underline thickness has been set to `1`. This decision was made
because the `Hack` font has an underline thickness under `0.5` with
relatively big fonts.

This edge-case can lead to some odd behavior when making the font size
super tiny (smaller than readable) so just not rendering any underline
might be a viable alternative.

I'd love some feedback on the issue if underline should always be rendered.

Strikethrough will hopefully be up tomorrow. :)

[maximbaz]

Very cool, now the position of the underline stays relatively on the same place regardless of the font size.

Also, a nice accident, using Consolas with my regular font size of 10 the underline perfectly matches the bottom of the box cursor, just like I wanted 🙂 This isn't necessarily true for all fonts, but it's a pleasant surprise.

In fact, the perfect match is for font sizes from 1 to 12, and starting from font size 13 it goes out of sync by one pixel... but it doesn't matter.

And this is Source Code Pro:

[chrisduerr]

I'd classify the first GIF as a bug. It might be up to freetype standards (not sure, I'd have to look into it), but the underline should never be even a single pixel below the cell height.

Since the cell height is the full height of the line, any pixel below that is part of the next line and should never contain content from the previous one. It's probably just a rounding/edge case thing, thanks for always being so quick to test things. :)

[chrisduerr]

The current breakage on macos is intentional. I will need some time to test this with a VM.

[chrisduerr]

@maximbaz To check if the font specifies an incorrect underline position/thickness or if there's some kind of off-by-one error, it would really help if you would give me the metrics for the situation where the underline goes one pixel (or more) beyond the block cursor's bottom line.

Here's a patch you can apply to this PR (a0270d9) for printing the metrics to stdout:
https://hastebin.com/kafalerami

[maximbaz]

Happy to help 🙂

The underline is pixel-perfect on font sizes [5-12, 18, 21] and is off on font sizes [13-17, 19-20].

Click

5
-----------
CELL_HEIGHT: 17
DESCENT: -4
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 0.45117188
CALC POS: -3
CALC THI: 1
-----------

6
-----------
CELL_HEIGHT: 20
DESCENT: -5
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 0.5415039
CALC POS: -4
CALC THI: 1
-----------

7
-----------
CELL_HEIGHT: 24
DESCENT: -6
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 0.63183594
CALC POS: -5
CALC THI: 1
-----------

8
-----------
CELL_HEIGHT: 27
DESCENT: -6
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 0.72216797
CALC POS: -5
CALC THI: 2
-----------

9
-----------
CELL_HEIGHT: 30
DESCENT: -7
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 0.8125
CALC POS: -6
CALC THI: 2
-----------

10
-----------
CELL_HEIGHT: 34
DESCENT: -8
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 0.90234375
CALC POS: -7
CALC THI: 2
-----------

11
-----------
CELL_HEIGHT: 37
DESCENT: -8
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 0.9926758
CALC POS: -7
CALC THI: 2
-----------

12
-----------
CELL_HEIGHT: 41
DESCENT: -9
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.0830078
CALC POS: -8
CALC THI: 2
-----------

13
-----------
CELL_HEIGHT: 44
DESCENT: -10
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.1733398
CALC POS: -9
CALC THI: 3
-----------

14
-----------
CELL_HEIGHT: 47
DESCENT: -11
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.2636719
CALC POS: -10
CALC THI: 3
-----------

15
-----------
CELL_HEIGHT: 51
DESCENT: -11
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.3540039
CALC POS: -10
CALC THI: 3
-----------

16
-----------
CELL_HEIGHT: 54
DESCENT: -12
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.4443359
CALC POS: -11
CALC THI: 3
-----------

17
-----------
CELL_HEIGHT: 58
DESCENT: -13
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.534668
CALC POS: -12
CALC THI: 3
-----------

18
-----------
CELL_HEIGHT: 61
DESCENT: -14
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.625
CALC POS: -12
CALC THI: 4
-----------

19
-----------
CELL_HEIGHT: 64
DESCENT: -14
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.7148438
CALC POS: -13
CALC THI: 4
-----------

20
-----------
CELL_HEIGHT: 68
DESCENT: -15
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.8051758
CALC POS: -14
CALC THI: 4
-----------

21
-----------
CELL_HEIGHT: 71
DESCENT: -16
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.8955078
CALC POS: -14
CALC THI: 4
-----------

[maximbaz]

By the way, the position of strikeout is very good now, on a few fonts that I tested and on a wide range of font sizes 👍

[chrisduerr]

Hmm that's odd. If I calculate the position myself from the metrics you've provided, I get a result that's still within the cell bounds.

Unfortunately I'm also not able to reproduce it on my system which kinda makes this a bit hard to troubleshoot for me. When using consolas myself (size 20) it looks like this:

It seems to be well within bounds.

[maximbaz]

Just out of curiosity I replaced .round() with .floor() here and got these results:

• pixel perfect underline on font sizes [8-18, 21]
• underline beyond cell height on font sizes [5-7, 19-20]

This is obviously not a solution, but it got me wondering: metrics.* properties are all rounded in font/src/ft/mod.rs, here we have some calculations based on already rounded values and then round again, can't this introduce some increasing calculation error? What if metrics.* were not rounded in font/src/ft/mod.rs, but left as f32, and here we had rounding only once, in the end of all calculations?

[maximbaz]

In case there are multiple Consolas fonts with different metrics, I use this one - try, maybe you can reproduce with it 🙂

[chrisduerr]

I think rounding multiple times should be fine because these metrics should never be float values. The underline position for example shouldn't be 11.2 pixels below the baseline because the concept of 0.2 pixels doesn't exist. And after we make use of the metrics we have to round again because we need u32s, so it's either floor, ceil or round to get valid integers.

I think that approach is correct, however if someone else knows more, please correct me.

Reproducing your issue will probably be hard for me because of potential DPI differences. I've used the consolas-fonts package to test though, so they might be different.

The question is just if my "algorithm" is correct really. I can't see anything wrong with it and it should be fine with the values you've supplied. If the algorithm is correct, but the font still specifies an offset that is out of bounds, I think alacritty should handle it.

I'm gonna go ahead and implement a bounds check which will make sure that whenever the underline is out of bounds, the underline will be moved up so it's exactly within bounds (using the thickness provided with the font). This should fix the issue and also help with potentially broken font metrics.

However if my approach to this calculation is wrong and maybe rounding shouldn't take place in the metrics, we should still fix this instead of just relying on a bounds check to catch off-by-one errors.

[maximbaz]

I added this to your patch:

println!("CALC POS F32: {}", f32::from(face.ft_face.underline_position()) * x_scale / 64.);
println!("CALC THI F32: {}", f32::from(face.ft_face.underline_thickness()) * x_scale / 64.);

Let's look at a random output:

-----------
CELL_HEIGHT: 44
DESCENT: -10
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.1733398
CALC POS F32: -8.836716
CALC THI F32: 2.6400146
CALC POS: -9
CALC THI: 3
-----------

What I was saying is that because F32 values can have any fractional part, the following hypothetical example can happen:

CALC_POS_F32 = 7.49
CALC_THI_F32 = 2.49

(CALC_POS_F32.round() + CALC_THI_F32.round() / 2.0).round() == (7 + 2 / 2).round() == 8
(CALC_POS_F32 + CALC_THI_F32 / 2.0).round() == (8.735).round() == 9

Let me know if this makes sense, I'll try the new bounds check in meantime.

[chrisduerr]

Yeah I understand what you mean. But in my opinion it doesn't make any sense to have a fractional underline position/thickness. It should always be an integer offset.

In fact the .round() and .floor() should not make any difference because every part of the calculation (descent, line_height, underline pos/thickness) should all be integers.

[maximbaz]

I don't see any problem with your algorithm either, and just out of curiosity I verified that simply removing .round() when calculating CALC POS and CALC THI doesn't solve my particular problem, so I agree, let's keep the code as it is. Bounds check on the other hand is solving the problem well 🙂

[maximbaz]

positon -> position

[chrisduerr]

Thanks :)

[chrisduerr]

I've added the bounds check, would be interested to see if that fixes it for you @maximbaz. I'd still love to know why it's rendered outside in the first place.

[maximbaz]

With the last commit the underline is never exceeding the cell and thus always matches the box cursor perfectly 👍

By the way, my dpi value is 210, so you can also try xrandr --dpi 210 and then starting alacritty to see if you can reproduce what I was showing 🙂

[chrisduerr]

I've tried to reproduce it with 210 DPI but that didn't help either. Not sure what could be going on but would you mind sharing the y position of the calculated underline for a size that's broken? Maybe 20?

It's calculated in line 556 of src/display.rs (you'll have to disable the if check).

[maximbaz]

For font size 20:

-----------
CELL_HEIGHT: 68
DESCENT: -15
UNDERLINE POS: -482
UNDERLINE THI: 144
X SCALE: 1.8051758
CALC POS F32: -13.59523
CALC THI F32: 4.0616455
CALC POS: -14
CALC THI: 4
-----------

===========
Y: 201
UNDERLINE_HEIGHT: 4
===========

[chrisduerr]

I forgot to ask you what the line number is but based on the Y I'd guess it's 3.

That 201 seems like a very odd number to me though. Because if Y would really be 3, the bottom line of the cell in line 3 should be at 68 * 3 = 204. Now if we take the calculated underline thickness, divide it by two and add it to the reported y = 201, we're at 203. Instead of being one pixel below, this should be one pixel above.

If the line number is only 2, the bottom of the cell would be 136, so the underline would be off by a ton.

These numbers seem a bit odd to me.

[maximbaz]

The line number is 3. Just following the code, the math is:

3 (line number) * 68 (line_height) + (-15) (descent) - (-14) (underline position) - (4 / 2) (underline thickness)

3 * 68 - 15 + 14 - 2 = 201

[chrisduerr]

Yeah so that doesn't make any sense at all then. Clearly if the line number is 3 and the line height is 68, then 201 should be accurate and fit well within the cell.

If you have another line below one with underline it actually does intersect with it, right? It's not like the block cursor is just not big enough?

[maximbaz]

If you have another line below one with underline it actually does intersect with it, right? It's not like the block cursor is just not big enough?

I believe yes, but it's hard to tell, which symbol will fill the entire cell besides block cursor?

And with the latest commit in this PR:

It is curious to investigate, but I feel we should leave this with the approach taken in the last commit 😄

[chrisduerr]

The best character to render for filling the whole block is the box cursor. The unicode character for it is defined here:
https://github.com/jwilm/alacritty/blob/master/font/src/lib.rs#L74 (echo "\U10a3e4")

[chrisduerr]

Looking at it closely, it definitely seems to intersect:

The problem is not the case you have, but if it's not an edge case. It would be possible that a single pixel underline is rendered one pixel too far down for example. Assuming that it is an issue with the way I calculate things.

However I'm fine with letting it go the way it is right now. The math seems accurate from my point of view and I don't think there's gonna be any configuration that would look bad. And if there is one that looks really out of place, we can just act upon it when we find it.

[maximbaz]

BOX_CURSOR_CHAR is intersecting just as well. I agree, with bounds check it looks very good and let's leave it as it is for now 🙂

[chrisduerr]

With a bit of work I was able to rebase this branch back on master and make it work again. It seems to be performing fairly well in the minimal tests I've done, so I don't expect any regressions from the state it was in when it was put on hold.

[dm1try]

osx(10.14.2)
Source Code Pro 13.0
DPI/padding/offsets are default

terminal & iTerm2 & alacritty:

[chrisduerr]

@dm1try That's not with an offset of 0, a glyph offset of 0, a padding of 0 and 1.0 DPI though, right?

[dm1try]

That's not with an offset of 0, a glyph offset of 0, a padding of 0

I used default settings(offsets were commented) and I've tried with explicit set to 0

# Configuration for Alacritty, the GPU enhanced terminal emulator
#env:
  # TERM env customization. Default is xterm-256color
  # Note: the default TERM value `xterm-256color` does not
  # specify all features alacritty supports. This does pose
  # a few issues with programs relying on terminfo and the
  # `tput` command
  # TERM: alacritty

window:
  # Window dimensions in character columns and lines
  # (changes require restart)
  #  dimensions:
  #  columns: 0
  #  lines: 0

  # Adds this many blank pixels of padding around the window
  # Units are physical pixels; this is not DPI aware.
  # (change requires restart)
  padding:
    x: 0
    y: 0

  # Window decorations
  # Setting this to false will result in window without borders and title bar.
  decorations: full
  fullscreen: true
  start_maximized: false

# Display tabs using this many cells (changes require restart)
tabspaces: 8

# When true, bold text is drawn using the bright variant of colors.
draw_bold_text_with_bright_colors: true

# Font configuration (changes require restart)
font:
  # The normal (roman) font face to use.
  normal:
    family: "Source Code Pro" # should be "Source Code Pro" or something on macOS.
    # Style can be specified to pick a specific face.

  # The bold font face
  bold:
    family: "Source Code Pro" # should be "Source Code Pro" or something on macOS.
    # Style can be specified to pick a specific face.
    # style: Bold

  # The italic font face
  italic:
    family: "Source Code Pro" # should be "Source Code Pro" or something on macOS.
    # Style can be specified to pick a specific face.
    # style: Italic
  offset:
    x: 0
    y: 0

  # Glyph offset determines the locations of the glyphs within their cells with
  # the default being at the bottom. Increasing `x` moves the glyph to the right,
  # increasing `y` moves the glyph upwards.
  glyph_offset:
    x: 0
    y: 0

  # Point size of the font
  size: 13.0
  # Offset is the extra space around each character. offset.y can be thought of
  # as modifying the linespacing, and offset.x as modifying the letter spacing.
  # OS X only: use thin stroke font rendering. Thin strokes are suitable
  # for retina displays, but for non-retina you probably want this set to
  # false.
  use_thin_strokes: false
...

according to DPI, I use default to device scaling. though DPR is 2 on retina display:

reated log file at "/var/folders/8t/0phq2zg12js4d9ycstdlbfkr0000gn/T/Alacritty-57922.log"
[2018-12-20 16:46] [INFO] Welcome to Alacritty.
[2018-12-20 16:46] [INFO] Configuration loaded from /Users/dmitrydedov/.alacritty.yml
[2018-12-20 16:46] [INFO] device_pixel_ratio: 2
[2018-12-20 16:46] [INFO] width: 2048, height: 1536
[2018-12-20 16:46] [INFO] Initializing glyph cache
[2018-12-20 16:46] [INFO] Finished initializing glyph cache in 0.058081714
[2018-12-20 16:46] [INFO] width: 1200, height: 792
[2018-12-20 16:46] [INFO] Cell Size: (15 x 33)
[2018-12-20 16:46] [INFO] Padding: (0 x 0)
[2018-12-20 16:46] [INFO] PTY Dimensions: Line(24) x Column(80)
[2018-12-20 16:46] [INFO] Initialisation complete
[2018-12-20 16:46] [INFO] Font size changed: Size(26) [DPR: 2]
[2018-12-20 16:46] [INFO] width: 1200, height: 792

on external display the problem is similar:

[chrisduerr]

I haven't considered DPI at all yet because back then it was a bit different. I'll update this PR asap with the remaining issues resolved and refactored code, hopefully that should fix it.

[dm1try]

btw, unrelated to this issue, but as you can see on the screenshot above the default letter spacing in alacritty is different from terminal/iTerm2. setting offset.x to 1 make them equal, not sure if it's a bug or expected behavior

[chrisduerr]

As far as I'm concerned, I've addressed all remaining issues except for Windows support.

So in theory it should work on macOS now if the metrics are calculated correctly. Would be appreciated if you could test again @dm1try, hopefully that resolves your problem.

[chrisduerr]

Also it looks like rusttype provides neither underline, nor strikeout metrics. As a result of that, I'm not sure if it makes sense at this point to support underline/strikeout at all on Windows at this point.

@zacps What do you think about keeping the old hack in place for Window until fontkit has been landed? It seems like fontkit at least offers the underline metrics, maybe we can even upstream strikeout support. Otherwise our custom strikeout which I've had to implement for macOS would do just fine, the important part is mainly the underline.

[dm1try]

@chrisduerr thanks, it works now!

(checked on both displays)

[zacps]

Fontkit for Windows is basically done, we could ship it for Windows while I work on Linux fixes. I guess it depends how keen you are to get this through? I'd like to say fontkit would be done in a couple of weeks but unfortunately life doesn't work like that.

[chrisduerr]

@zacps I'd like to get this merged, then have conPTY as the next significant PR and work my way torwards fontkit. With some fixes to URL launching and similar stuff in between.

I'm totally fine with fixing up fontkit myself on linux, that shouldn't be any problem. I can help out at least a little bit to speed things up. My idea was to get this PR into 0.2.5 and potentially release 0.2.6 with fontkit (at least as an option on macOS/linux, probably the default on Windows).

So for me it doesn't make much sense to try and spin up some mediocre underline/strikeout support for Windows now, when the next release is going to have proper support for it anyways. Just so we don't have to do things twice. The old and hacky way to do URLs can still stay in place for Windows, that shouldn't be a problem, so there's not gonna be any regression.

[chrisduerr]

So I started writing the code for using the old fallback behavior just for Windows and then I realized that I'm just making life hard on myself. Coming up with some okay-ish metrics for underline and strikeout really isn't difficult, so the much, much easier solution was just to implement these metrics. No need to make it unnecessarily hard and ugly.

Would appreciate some testing though @zacps, since I don't have access to Windows right now. There's no scientific background for the way I made these metrics up, other than them looking decent on linux. So let me know if that works out for you too!

[zacps]

Unfortunately I also don't have access to Windows until the new year, but I can take a look at it then.

[cole-h]

I copied the echo command that @dm1try used above to test. Strikethrough doesn't seem to be working, but underline is. Adding a reset escape code \e[0m fixes the underline issue present in the image -- I left as is in case this is unexpected. Here's a screenshot of the output in both Powershell and Bash under Ubuntu in WSL, respectively:

Let me know if there's any other information or testing I can provide.

[chrisduerr]

@cole-h Thanks a ton for testing this. The issue with resetting is 'intentional'. I simply didn't add the reset code at the end because my prompt adds it automatically.

The only question left really is if strikeout doesn't work because Window is messing with things or if the metrics are wrong.

Could you try applying this patch?

diff --git a/src/renderer/lines.rs b/src/renderer/lines.rs
index 556dcb0..f90972b 100644
--- a/src/renderer/lines.rs
+++ b/src/renderer/lines.rs
@@ -118,9 +118,7 @@ fn create_rect(
     let width = end_x - start_x;
 
     let (position, height) = match flag {
-        Flags::UNDERLINE => (metrics.underline_position, metrics.underline_thickness),
-        Flags::STRIKEOUT => (metrics.strikeout_position, metrics.strikeout_thickness),
-        _ => unimplemented!("Invalid flag for cell line drawing specified"),
+        _ => (metrics.underline_position, metrics.underline_thickness),
     };
 
     let cell_bottom = (start.line.0 as f32 + 1.) * size.cell_height;

What it does is simply ignore the strikeout metrics and always render underlines for both strikeout and underlines. So if this patch doesn't underline the STRIKEyTROUGH text, it's likely an issue that will be resolved with the conPTY.

[cole-h]

Patch applied, added reset escapes for cleaner screenshot: no underlines on the STRIKEyTHROUGH.

Let me know if there is anything else!

[chrisduerr]

Thanks, so I think this is good to do then. I'll do a final code review to make sure everything's clean and working properly and then it'll hopefully be merged.

Thanks a ton for the help again @cole-h!

[FichteFoll]

Thanks for implementing and updating.

However, when testing this, I have to say the underline position is a bit unfortunate with the terminus font.

The upper terminal shows an older master version (cf9d94e) with a patch I applied as per #31 (comment) and the lower terminal is from current master unpatched:

Here's my config.

[chrisduerr]

@FichteFoll I believe you're absolutely correct and there is actually a bug in the computation of the line metrics. I've created a PR here, please let me know if that fixes it.

Also feel free to just open a new issue in the future if you're suspecting that there might be a bug. Feedback is always appreciated!

[FichteFoll]

Wasn't sure about it being a bug or a problem on my part, so I presumed commenting on the PR that introduced the change was the most appropriate.