Humm. I'm really sorry to disappoint. But I was wrong about the ceil() being significant.  We divide by 1024 afterwards and round...

  info->width  = PANGO_PIXELS (howmany (logical.width, strlen(VTE_DRAW_SINGLE_WIDE_CHARACTERS))); 

I'm investigating.

Humm.  I'm out of ideas.  I'm getting already-rounded numbers there, since cairo + freetype hinting do the rounding.  Perhaps I need a different default hinting mode to reproduce...

I don't think the width is the problem, but the height. The issue people have is a change in the space between rows. See 
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c29
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c30
https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c34

(In reply to Adam Goode from comment #3)
> I don't think the width is the problem, but the height. The issue people
> have is a change in the space between rows. See 
> https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c29
> https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c30
> https://bugzilla.redhat.com/show_bug.cgi?id=1437999#c34

Yes, correct. The whole terminal becomes bigger vertically. Which then messes with positioning, how many of them fit onto the desktop etc.

What happens if PANGO_PIXELS_CEIL is replaced with PANGO_PIXELS for setting info->height and info->ascent?

Ah ok.  Then yeah, rounding height sounds like might do it.

So I looked at vte and played with the font measurement code. It's really just trusting pango to do all the metrics, based on the string:
 !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~

Putting that string several times into a file "abc.txt" and running pango-view with and without the freetype patch gives identical results to vte:
 pango-view --font "monospace 12" ~/abc.txt --pixels

Note that without --pixels, the width changes but the height is the same, and with --pixels, the height changes but the width is the same.

Created attachment 350043 [details]
pango-view --font "monospace 8" ~/abc.txt

Created attachment 350044 [details]
pango-view --font "monospace 8" ~/abc.txt --pixels

Created attachment 350045 [details]
pango-view --font "monospace 8" ~/abc.txt
(with freetype patch)

Created attachment 350047 [details]
pango-view --font "monospace 8" ~/abc.txt --pixels (with freetype patch)

It doesn't look like vte is wrong here. At least it doesn't differ from pango directly. And really all --pixels is doing is setting --dpi=72.

Note: VTE's font rendering changed (the width increased) in Ubuntu about a year ago: https://bugs.launchpad.net/ubuntu/+source/fontconfig/+bug/1547374. No clue which component was responsible for that, filing under fontconfig was just a guess.

Eventually I just got used to this change, after a few months it no longer bothered me. I guess it's going to be the same with this one, too.

Yeah I'm also thinking now that there's no bug in vte or pango.  Though, vte can be changed to round instead of ceil, the line height.  But that won't change the results here since pango is already passing down to vte whole numbers.

In the mean time, we could consider adding an API and a hidden g-t setting to add/subtract a constant pixel value from the cell size computed by pango. That'd address bug 738781 as well.

Just an enquiry as to whether there's been any further thoughts or development on this? 

I found this bug whilst looking to see if my terminal emulator (termite) can alter line spacing. In the course of that search, I found several alternative vte-based emulators asking the same question -- there's definitely a call for configurable line spacing!

Nope, nothing happened. (You would've seen it here or in the other linked bug.)

Can't this be solved somehow on fontconfig or pango level? I really don't know. Some stanzas added to fonts.conf maybe? If there's a way to configure there, I wouldn't want to complicate VTE's codebase.

If it's confirmed that there's no possible external workaround, and if we decide to address this feature: shall we add a new API, or probably to be more consistent with the widget's padding go with some CSS? E.g.

vte-terminal {
  padding: t r b l /* these values apply to the entire widget */
}
vte-terminal cell {
  padding: t r b l /* these values apply to each cell */
}

Do we need custom line spacing only, or custom column spacing too? How would the latter co-exist with complex text rendering, if we ever implement that?

Also, is there a need for negative adjustment, i.e. squeezing the rows (or even columns), risking glyphs getting cropped or overflowing to the lines above/below?

I've seen fontconfig suggested as a possible avenue for this, but I haven't yet found either an apparent solution in the fontconfig documentation or anyone else that has been able to suggest a way to do it. I can't claim to be a deep expert on fontconfig, but I've drawn a blank in a fairly extensive search. (Whilst coming up with quite a significant number of people requesting this capability.)

From my perspective, this is desirable from the perspective of accessibility -- for people with visual impairments that benefit from increased line spacing -- and for improving usability of some writing modes. I wouldn't personally see a need for changing column spacing.

Similarly, I wouldn't personally see a need for negative adjustment, and that does seem to open up a whole range of problems with cropping and overflowing. (I know that rxvt-unicode supported negative values in its lineSpacing parameter, but I think that that was largely to overcome some odd spacing calculations elsewhere anyway.)

(In reply to Joss Wright from comment #19)

> I've drawn a blank in a fairly extensive search.

Okay, let's say fontconfig most likely doesn't support it and hence we should move forward.

> From my perspective, this is desirable from the perspective of accessibility

I've increased column spacing (probably "letter spacing" is the right term) by 1px and it actually made it more easily readable for me. (Maybe I'm just using a too small font to begin with.) Would be great to get input from folks relying on accessibility. Or, I'm just tempted to add this as well for symmetry and completeness, plus because it's just as easy as line spacing.

I was looking for the right term for this horizontal offset (to be used in the source code for variable names), similarly to "ascent" / "descent", I'm not sure if "left/right side bearing" is the right one.

> Similarly, I wouldn't personally see a need for negative adjustment

Obviously not from accessibility point of view... I'm curious to hear about folks who prefer a more dense look, more terminals / more data to fit on the screen.

Nitpicking: VTE draws the line drawing characters (see e.g. mc, alsamixer, ncdu) manually rather than taking from the font. I guess they should be extended to be drawn even in this newly introduced padding, so that the lines are still continuous. Correct?

Another use can I can foresee for letter (column) spacing is if someone is extensively using special Unicode glyphs which are single-width, yet somewhat wider than the English letters (e.g. bug 781676).

I'm experimenting with negative spacing now. With small fonts it's hardly usable, but with large ones it might be okay to save a little bit (e.g. have room for more charcells at someone's preferred big fontsize).

The CSS approach seems to be implementable (there are Gtk+ methods around widget_path which allow to get the padding of an imaginary vte-terminal-charcell widget), but negative paddings aren't allowed, plus it forces us to handle left and right, as well as top and bottom paddings separately which I really don't want to do (I think a single value per axis, split evenly on the two sides of charcells, is good enough for all purposes. Plus, nitpicking, we'd have troubles centering the manually drawn glyphs.)

So I'm voting for a new API (two setters and getters), for extra line spacing and letter spacing in pixels.

I was also thinking a bit that maybe it should scale with the font, e.g. have a multiplier instead so that the spacing also increases along with the font size. I think it's overkill, or if really required, the logic could go to the app (e.g. gnome-terminal) rather than VTE. Also, for whom this feature is the most important (accessibility) I don't think changing the font size is a typical use case, I would guess that people with visual impairment would much rather once find their preferred font and later stay with that (correct me please if I guessed it wrong).

Thinking about it even a bit more :)

- I'd still like to add this feature along both axes.

- I'd still allow negative values.

- I'd still go for some new API (a pair of setters/getters, or perhaps a single setter/getter, the single getter placing the two values via pointers? The latter approach is probably problematic around PROP_* changes).

- I'd have these take a number that'd added to the charcell's size (or negative value to shrink). (That is, I've rejected my previous approach of taking a multiplier relative to the font size, but...)

- ... I'd apply the font_scale multiplier on top of the received number (and round it to the nearest integer), because I believe that's the natural behavior people would expect, and hence I'd prefer this code to go to VTE once as opposed to each VTE-based apps.

- I'm wondering then if the APIs should take int or double. Given that VTE multiplies by the scale factor, certain values would be unreachable if we take ints. Althouogh it doesn't really matter.

- For g-t I'd only do a hidden pref for the time being.

Christian, would you be okay with this design?

The implementation is boring boilerplate code only, not a single bit of fun, so I'd only do it if I have your design approval :)

(It's unfortunate that the good posts are on this unrelated bug instead of bug 738781, so let's reopen this one and retitle it.)

(In reply to Egmont Koblinger from comment #20)
> Nitpicking: VTE draws the line drawing characters (see e.g. mc, alsamixer,
> ncdu) manually rather than taking from the font. I guess they should be
> extended to be drawn even in this newly introduced padding, so that the
> lines are still continuous. Correct?

Correct. This extra spacing *enlarges* the cell, and does and should not introduce any spacing *between* the cells either horizontally or vertically. The only change is that the origin point of where the font is drawn is now somewhere inside the cell, not on the border.

(In reply to Egmont Koblinger from comment #21)
> Another use can I can foresee for letter (column) spacing is if someone is
> extensively using special Unicode glyphs which are single-width, yet
> somewhat wider than the English letters (e.g. bug 781676).
> 
> I'm experimenting with negative spacing now. With small fonts it's hardly
> usable, but with large ones it might be okay to save a little bit (e.g. have
> room for more charcells at someone's preferred big fontsize).

I don't think negative spacing is ok. This will make it more likely for text to overrun its cell, which will make the invalidated area too small on changes, and thus cause garbage/artifacts. If someone thinks their font is too large, they can get a Narrow variant (those certainly exist for proportional fonts, and I don't see anything stopping someone from doing the same to monospace fonts).

> The CSS approach seems to be implementable (there are Gtk+ methods around
> widget_path which allow to get the padding of an imaginary
> vte-terminal-charcell widget), but negative paddings aren't allowed, plus it
> forces us to handle left and right, as well as top and bottom paddings
> separately which I really don't want to do (I think a single value per axis,
> split evenly on the two sides of charcells, is good enough for all purposes.
> Plus, nitpicking, we'd have troubles centering the manually drawn glyphs.)
> 
> So I'm voting for a new API (two setters and getters), for extra line
> spacing and letter spacing in pixels.

I agree that this should be API not CSS. Not pixels though, since:

> I was also thinking a bit that maybe it should scale with the font, e.g.
> have a multiplier instead so that the spacing also increases along with the
> font size. I think it's overkill, or if really required, the logic could go
> to the app (e.g. gnome-terminal) rather than VTE. Also, for whom this
> feature is the most important (accessibility) I don't think changing the
> font size is a typical use case, I would guess that people with visual
> impairment would much rather once find their preferred font and later stay
> with that (correct me please if I guessed it wrong).

So IMHO this extra space should be specified in em/ex (well, an integer value specifying a multiple of some fraction of an em/ex), so it scales with the font size / font scale, and the DPI (so moving windows from a high DPI to a low DPI screen doesn't need the user to change the pref!).

(In reply to Egmont Koblinger from comment #22)
> - I'd still like to add this feature along both axes.

WRT the 'complex text' question in comment 17, if it did turn out that we cannot implement both of these, we can just remove the 'extra column spacing' again *at that time*.

> - I'd still allow negative values.

I'd rather not, see above.

> - I'd still go for some new API (a pair of setters/getters, or perhaps a
> single setter/getter, the single getter placing the two values via pointers?
> The latter approach is probably problematic around PROP_* changes).

Yes, separate setters/getters for each direction. And I do agree with comment 21 that the spacing should be distributed evenly on top/bottom and left/right.
 
> - I'd have these take a number that'd added to the charcell's size (or
> negative value to shrink). (That is, I've rejected my previous approach of
> taking a multiplier relative to the font size, but...)

Hmm... see above for my POV.
 
> - ... I'd apply the font_scale multiplier on top of the received number (and
> round it to the nearest integer), because I believe that's the natural
> behavior people would expect, and hence I'd prefer this code to go to VTE
> once as opposed to each VTE-based apps.
 
> - I'm wondering then if the APIs should take int or double. Given that VTE
> multiplies by the scale factor, certain values would be unreachable if we
> take ints. Althouogh it doesn't really matter.

If for example we say the API sets the extra spacing as multiple of (say) 1/100th of an em/ex (for column/row spacing), then we can just use ints. 

> - For g-t I'd only do a hidden pref for the time being.

Sure.
 
> Christian, would you be okay with this design?

Given that at least the row spacing appears to be an a11y concern (bug 738781), I'm fine with implementing this, modulo the differences to be resolved as per the above :-)

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

(In reply to Christian Persch from comment #23)

> (It's unfortunate that the good posts are on this unrelated bug instead of
> bug 738781, so let's reopen this one and retitle it.)

It's indeed unfortunate.

> I don't think negative spacing is ok. This will make it more likely for text
> to overrun its cell [..]

Yup, as you said it makes overruns, display corruptions etc. "more likely", but such things already happen now sometimes.

Is it fair to allow negative values, but state in the API docs to "use at your own risk"? To not offer negative values on g-t's UI (if we ever add one) but silently accept those from gsettings and forward to VTE? Or even refuse in g-t, just let other VTE-based apps use them?

Okay, I really don't insist, I'm fine with disallowing them entirely.

> So IMHO this extra space should be specified in em/ex (well, an integer
> value specifying a multiple of some fraction of an em/ex), so it scales with
> the font size / font scale, and the DPI (so moving windows from a high DPI
> to a low DPI screen doesn't need the user to change the pref!).

I'm okay with the scaling factor. But em/ex seems to be complicated to get and aren't used anywhere else. So how about just a multiplier to the cell width/height?

Should it be the factor for the addition only (0 = unchanged) or an overall factor (1 = unchanged)?

By the way, I'm not familiar with the high/low DPI stuff, or DPI in general at all. For me it's just pixels all the time. I know of course the world isn't as simple as that, I'm yet to learn this topic :)

> If for example we say the API sets the extra spacing as multiple of (say)
> 1/100th of an em/ex (for column/row spacing), then we can just use ints.

I'm personally not a big fan of arbitrary constants, like 100 in that case, although it's of course a faily common choice in the human world. So I'm just quietly wondering if we should still go with doubles? E.g. gtk.css also takes "-GtkWidget-cursor-aspect-ratio: <double>;" for the I-beam cursor's width.

(In reply to Egmont Koblinger from comment #25)
> > So IMHO this extra space should be specified in em/ex (well, an integer
> > value specifying a multiple of some fraction of an em/ex), so it scales with
> > the font size / font scale, and the DPI (so moving windows from a high DPI
> > to a low DPI screen doesn't need the user to change the pref!).
> 
> I'm okay with the scaling factor. But em/ex seems to be complicated to get
> and aren't used anywhere else. So how about just a multiplier to the cell
> width/height?
> 
> Should it be the factor for the addition only (0 = unchanged) or an overall
> factor (1 = unchanged)?

em is basically the cell width and ex (well, 2ex) the cell height, so making this a scaling factor for the width/height seems ok. With that, I guess 1-based makes sense, but no strong preference for either from me.

> By the way, I'm not familiar with the high/low DPI stuff, or DPI in general
> at all. For me it's just pixels all the time. I know of course the world
> isn't as simple as that, I'm yet to learn this topic :)
> 
> > If for example we say the API sets the extra spacing as multiple of (say)
> > 1/100th of an em/ex (for column/row spacing), then we can just use ints.
> 
> I'm personally not a big fan of arbitrary constants, like 100 in that case,
> although it's of course a faily common choice in the human world. So I'm
> just quietly wondering if we should still go with doubles? E.g. gtk.css also
> takes "-GtkWidget-cursor-aspect-ratio: <double>;" for the I-beam cursor's
> width.

Yes, doubles should be ok here.

I guess we've pretty much agreed, right?

I also tend to vote for 1-based factor, although mine is also a weak vote. I guess I'll have a safety clamp between 1.0 and 2.0. That would disallow "negative" extra spacing, at least until someone comes up with a convincing reason to have that.

I guess only the property/function names are left to decide, that can wait until the first demo patch.

I'll go ahead and do the first round (at some point).

I just wanted to say thank you for being so responsive!

Joss, thanks but let's wait until it's actually implemented :D

The funny thing is that the standard way of using these numbers (in word processors, html, etc.) is different along the two axes.

For "letter spacing" a.k.a. "character spacing" a.k.a. "tracking", the default value is 0, anything larger than that denotes extra space, negative values stand for squeezing.

This is probably the reasonable way to do this (generally, with non-monospaced fonts) since the width changes from letter to letter, yet there's a constant addition. For the same reason, percentage values probably don't make sense here (or can they be relative to a particular letter's width?).

On the other hand, for "line spacing", a.k.a. "line-height" in CSS, 1.0 or 100% or the font's absolute height stands for the default. Extra spacing is achieved using absolute numbers larger than the font height, or factors larger than 1.0.

I'd guess this is partially because of historical reasons (maybe originating from the English term "double spacing", it would be silly for 1.0 to denote this in software), partially because noone thought of being consistent with letter spacing. It's a bit different story anyways since normally this value is constant from line to line.

Of course with fixed grid it's a somewhat different world.

Anyway, what is means for us is that we cannot use the standard terms "line spacing" and "letter spacing" in our API names and yet have consistent semantics in the two methods. I guess consistent behavior between our two methods is the most important. That means we'll have to go with nonstandard names such as "cell_width/height_factor" or so.

There's a bit of ugliness around underlines (escape sequence as well as regex matching) and strikethrough.

I thought the extra spacing can be the private business of one single component as close to the font as possible (that is, either the font_info or the vte_draw stuff in vtedraw.cc), the rest wouldn't need to be modified.

Now it turns out that this causes the underlines and strikethrough (drawn by vte.cc) to be mispositioned. I really don't feel like refactoring the code and moving these to vtedraw.cc for the sake of this feature. Hence, the computed spacing values will have to be explicitly communicated to vte.cc in _vte_draw_get_text_metrics() and handled there appropriately. That is, unfortunately, more places than I anticipated will have to be aware of the existence of line/letter spacing.

Also not sure what to do with the underline cursor. Being at the bottom of the spacing-enlarged cell (way below the letters' baseline) looks stupid. I guess it should be positioned relatively to the letters as if there was no extra spacing. In the mean time the block cursor occupies the entire enlarged height (which we could also change by the way, but then it looked stupid in relation with cells with background colors). So I guess we'll just go with an underline cursor that isn't placed as low as the bottom of the rectangle cursor.

So many stupid details in such an easy feature request :)

Created attachment 361960 [details] [review]
vte

vte, version 0.

Underline etc. are not yet addressed, and there's also a fixme around invalidation.

Created attachment 361961 [details] [review]
gnome-terminal

gnome-terminal, version 0.

Use something like this to set a value (once set, it can also be modified in dconf-editor):

dconf write /org/gnome/terminal/legacy/profiles:/:b1dcc9dd-5262-4d8d-a863-c897e6d979b9/cell-width-scale 1.5

and similarly for cell-height-scale.

(In reply to Egmont Koblinger from comment #31)
> There's a bit of ugliness around underlines (escape sequence as well as
> regex matching) and strikethrough.
[...]
> Also not sure what to do with the underline cursor. Being at the bottom of
> the spacing-enlarged cell (way below the letters' baseline) looks stupid. I
> guess it should be positioned relatively to the letters as if there was no
> extra spacing. In the mean time the block cursor occupies the entire
> enlarged height (which we could also change by the way, but then it looked
> stupid in relation with cells with background colors). So I guess we'll just
> go with an underline cursor that isn't placed as low as the bottom of the
> rectangle cursor.

IMHO underline and underline cursor should be just below the font's baseline, which with this patch won't just be at the very bottom of the cell.

(In reply to Christian Persch from comment #34)

> IMHO underline and underline cursor should be just below the font's
> baseline, which with this patch won't just be at the very bottom of the cell.

Seems we agree here as well. The next version will do so.

(In reply to Christian Persch from comment #34)

> IMHO underline and underline cursor should be just below the font's
> baseline, which with this patch won't just be at the very bottom of the cell.

On a somewhat related note: Shouldn't we use pango_font_metrics_get_{underline,strikethrough}_{position,thickness} for these?

Created attachment 362059 [details] [review]
vte, v1

Underline, strikethrough, cursor etc. position should be as expected.

I'll do another round of self-review to possibly clean up the code, but any comments are already appreciated. Most importantly: What shall the API names be?

Low prio TODO: Push CJK to the right by yet another half letter spacing.

Created attachment 362061 [details] [review]
vte, v2

CJK centered. Rounding fixes. Testapp.

The API bits and names look ok; haven't looked too closely at the other bits yet other than that they seem complicated ;-)

One nit: use double not gdouble, int not gint, etc., in new code.

To make the rest of the changes easier to follow, you could commit separately (now!) the chunks changing from one underline thickness/position to one each for underline and regex underline. (If you want to.)

Trying it out, I did notice that the block cursor looks beyond ugly; maybe we should restrict it to filling only the inner cell area excluding the extra borders, even given the background colour problem...

Boxes draw fine, but in perf/UTF-8-demo.txt I noticed that the maths are getting disconnected; maybe we should also special-draw (selected) characters from the '2300..23FF Miscellaneous Technical' block?

(In reply to Christian Persch from comment #39)

> they seem complicated ;-)

The feature itself is much more complicated than we anticipated. So many nontrivial questions we didn't think of.

> One nit: use double not gdouble, int not gint, etc., in new code.

Happy to, just curious: why?

> Trying it out, I did notice that the block cursor looks beyond ugly; maybe
> we should restrict it to filling only the inner cell area excluding the
> extra borders, even given the background colour problem...

I'll take a look.

(I've tried the I-beam cursor at the left of the cell vs. the left of the glyph; the left of the cell looked much better to me.)

> Boxes draw fine, but in perf/UTF-8-demo.txt I noticed that the maths are
> getting disconnected; maybe we should also special-draw (selected)
> characters from the '2300..23FF Miscellaneous Technical' block?

Nice catch. No, I don't want to hand-draw them :P

(In reply to Egmont Koblinger from comment #40)

> > Trying it out, I did notice that the block cursor looks beyond ugly

Still haven't tried, just thinking... LibreOffice Writer (and presumably all other similar software) don't increase the cursor height either when line spacing is increased. So it sounds a reasonable request, and probably should also apply to the I-beam height. And then to the underscore cursor width too I guess.

> Trying it out, I did notice that the block cursor looks beyond ugly

Thinking about it and looking at our code (still haven't started doing it):

This would seriously further complicate everything. That's because painting the block cursor requires that we re-paint (with swapped or otherwise different colors) the underlying letter, using whatever method we use to paint a letter.

That is, a special casing to exclude the spacing needs to be added to this method, just for the sake of the cursor (and if it's not cumbersome enough already, underline positioning etc. sure make it that).

Alternatively, draw the entire glyph and then revert the spacing only. Ouch.

I'm inclined to say that at least for now, until a counter request arrives, let's leave it this way for the sake of code simplicity for a presumably rarely used feature.

(By the way, xterm, urxvt and konsole all support line spacing, and all increase the cursor height. And yes it's ugly.)

... Oh, and forgot to say, if we wanted the cursor not to extend to the spacing then how should the cursor over CJKs look?

(In reply to Egmont Koblinger from comment #42)
> I'm inclined to say that at least for now, until a counter request arrives,
> let's leave it this way for the sake of code simplicity for a presumably
> rarely used feature.

OK.

Created attachment 362347 [details] [review]
vte, v3 part 1

Created attachment 362348 [details] [review]
vte, v3 part 2

+        if (ascent != m_char_x_offset) {
+                resize = true;
+                m_char_x_offset = x_offset;
+        }
+        if (ascent != m_char_y_offset) {
+                resize = true;
+                m_char_y_offset = y_offset;
+        }

Do you mean to compare [xy]_offset  with m_char_[xy]_offset here?

-       m_line_thickness = MAX (MIN ((height - ascent) / 2, height / 14), 1);
+        m_line_thickness = MAX (MIN (descent / 2, (ascent + descent) / 14), 1);
[and more instances]

You replace height with ascent + descent in a bunch of places... probably because m_char_heigth will now include the extra padding? Maybe rename (possibly as a follow-up patch) m_char_height/width to m_cell_height/width?

Unnecessary, since you just initialised them with the 'double foo{1.0}' in the class definition.

+                int x = requests[i].x + (requests[i].columns == 2 ? draw->x_spacing : draw->x_offset);

... + request[i].columns * draw->x_spacing / 2 ?

+        /* Apply letter spacing and line spacing. */
+        draw->width = draw->fonts[VTE_DRAW_NORMAL]->width * cell_width_scale;
+        draw->x_spacing = draw->width - draw->fonts[VTE_DRAW_NORMAL]->width;

I wouldn't do it like that, since that can lead to [xy]_spacing being odd; IMHO we should ensure it's always even, so that the extra padding is evenly distributed around the cell's border.

(In reply to Christian Persch from comment #47)
> Unnecessary, since you just initialised them with the 'double foo{1.0}' in
> the class definition.

Hmm...I meant to say that you can just put the m_cell_*_scale initialisation in the class definition like you already do in app.cc. (I know existing code doesn't, but this is new code and that's the style (C++11) I want to move to :-)

Having tried this patch, I think the new line thickness calculation is wrong; it makes the lines (see doc/boxes.txt and perf/UTF8-demo.txt) way, way, way too heavy (see yourself with scale = 2.0 in width or both directions). (That also exposes a bug in some of the line drawing characters, which I'll file separately.)

(In reply to Christian Persch from comment #49)
> [...]I think the new line thickness calculation is wrong

It's actually not the new line thickness calculation, it's just that the calculations in _vte_draw_terminal_draw_graphic also need to be adjusted for column_width/row_height now including the padding.

(In reply to Egmont Koblinger from comment #40)
> > One nit: use double not gdouble, int not gint, etc., in new code.
> 
> Happy to, just curious: why?

Just trying to get rid of extraneous glib dependencies :-) plus it's actually shorter.

(In reply to Christian Persch from comment #47)

> +        if (ascent != m_char_x_offset) {
>
> Do you mean to compare [xy]_offset  with m_char_[xy]_offset here?

Definitely, looks like an ugly copy-n-paste bug. Thanks for catching!

> You replace height with ascent + descent in a bunch of places... probably
> because m_char_heigth will now include the extra padding? Maybe rename
> (possibly as a follow-up patch) m_char_height/width to m_cell_height/width?

Was thinking about that, but I didn't want to change _that_ much in the code. But now I guess you're right. Instead of a followup, I'd rather do that in a preceding change.

> +        draw->x_spacing = draw->width - draw->fonts[VTE_DRAW_NORMAL]->width;
> 
> I wouldn't do it like that, since that can lead to [xy]_spacing being odd;
> IMHO we should ensure it's always even, so that the extra padding is evenly
> distributed around the cell's border.

I think going with increment of 2 pixels of spacing is too much. I don't think anyone would mind us arbitrarily chosing which way the rounding goes in case of odd pixels, people wouldn't even bother checking it. I'd like to retain this finer granularity for spacing.

(In reply to Christian Persch from comment #49)

> Having tried this patch, I think the new line thickness calculation is
> wrong; it makes the lines (see doc/boxes.txt and perf/UTF8-demo.txt) way,
> way, way too heavy

Looks like another nice catch (I haven't caught this being wrong), will check (hopefully tomorrow), thanks!

(In reply to Egmont Koblinger from comment #52)

> Maybe rename (possibly as a follow-up patch)
> m_char_height/width to m_cell_height/width?

Okay to go as far as renaming the public API vte_terminal_get_char_height/width? (Keeping the old one as deprecated, of course.)

I don't think that's necessary, just amend the documentation of these functions.

Created attachment 362458 [details] [review]
vte, v4 part 1

part 1: separate variables for underline/strikethrough positions/thicknesses.

Just a tiny cosmetic change from "v3 part 1".

Created attachment 362459 [details] [review]
vte, v4 part 2

part2: Rename pretty much all char_{width,height} to cell_{width,height} throughout the code.

Created attachment 362461 [details] [review]
vte, v4 part 3

part 3: The main bits.

Changes: Added "cell_" or "char_" prefix to some variables to make them more obvious. Width of line/box drawing fixed.

Comment on attachment 362459 [details] [review]
vte, v4 part 2

Didn't check this at all, assuming it's just a sed job :-)

Thanks! Just a few nits remaining:

(In reply to Egmont Koblinger from comment #41)
> (In reply to Egmont Koblinger from comment #40)
> 
> > > Trying it out, I did notice that the block cursor looks beyond ugly
> 
> Still haven't tried, just thinking... LibreOffice Writer (and presumably all
> other similar software) don't increase the cursor height either when line
> spacing is increased. So it sounds a reasonable request, and probably should
> also apply to the I-beam height. And then to the underscore cursor width too
> I guess.

+ stem_width = (int) (((float) (m_char_ascent + m_char_descent)) * m_cursor_aspect_ratio + 0.

You're calculating the stem width from the font height, but painting the ibeam with the full cell height, meaning the aspect isn't correct. Maybe the ibeam cursor should only be painted as high as the font, not the cell? (And the underline cursor maybe should only extend the font width, not the whole cell width.) (IMHO that's independent of that we decided to make the block cursor take the whole cell.)

@ _vte_draw_get_char_width
-       return uinfo->width;
+        return uinfo->width + draw->cell_x_spacing * columns;

_vte_draw_get_char_width is only used in vte.cc to check if the faux-bold overruns the cell size. Now, if it does overrun the char width but the overrun is contained inside the extra spacing at the right, no actual overrun to the next cell occurs. Not sure this is worth fixing though, since I want to remove faux bold anyway (bug 756010).

-                radius = (column_width + 2) / 3;
+                radius = (draw->cell_width + 2) / 3;

Shouldn't this be calculated from the font width, not the cell width? IMHO the radius should be independent of the cell spacing.

> You're calculating the stem width from the font height, but painting the
> ibeam with the full cell height, meaning the aspect isn't correct. Maybe the
> ibeam cursor should only be painted as high as the font, not the cell? (And
> the underline cursor maybe should only extend the font width, not the whole
> cell width.) (IMHO that's independent of that we decided to make the block
> cursor take the whole cell.)

Similarly to the line width in box drawing chars, I think that just increasing the spacing shouldn't change the cursor width.

Making ibeam just as high as the font, not as the cell: it sounds reasonable, will try (and indeed it's independent of the block cursor story).

Making underline just as wide as the font: I'm more afraid of this, especially underneath CJK. I don't think it could look good and consistent there.

> _vte_draw_get_char_width is only used in vte.cc to check if the faux-bold
> overruns the cell size. Now, if it does overrun the char width but the
> overrun is contained inside the extra spacing at the right, no actual
> overrun to the next cell occurs. Not sure this is worth fixing though, since
> I want to remove faux bold anyway (bug 756010).

It's not only faux bold. Such overrun even happens with regular letters like 'm' with antialiasing (bug 734740) and tons of Unicode symbols (e.g. bug 781676).

With this in mind, I've modified _vte_daw_get_char_width() to become a weird hybrid: it returns the width of the particular character plus the spacing. I've documented that above that method; not sure if I should emphasize it more, or even rename the method to make it more obvious.

With that in mind, is there still a newly introduced bug? I believe there's a missed optimization (something that overruns without spacing might not overrun with spacing), does it make sense to further optimize it? Will see if it's reasonably doable.

Or I can return the old behavior of this method, and the caller add x_offset.

> -                radius = (column_width + 2) / 3;
> +                radius = (draw->cell_width + 2) / 3;
> 
> Shouldn't this be calculated from the font width, not the cell width? IMHO
> the radius should be independent of the cell spacing.

Haha, indeed. Will fix. I was actually playing with these (checked what happens with ambiguous=wide when these are double wide chars - radius isn't increased compared to the narrow mode, which is what I'd expect), and apparently ended up with the wrong variant.

(In reply to Egmont Koblinger from comment #60)
> Making underline just as wide as the font: I'm more afraid of this,
> especially underneath CJK. I don't think it could look good and consistent
> there.

Well, by that I meant (columns * cell_width - x_spacing) wide, starting at x_offset. 
 
> > _vte_draw_get_char_width is only used in vte.cc to check if the faux-bold
> > overruns the cell size. Now, if it does overrun the char width but the
> > overrun is contained inside the extra spacing at the right, no actual
> > overrun to the next cell occurs. Not sure this is worth fixing though, since
> > I want to remove faux bold anyway (bug 756010).
> 
> It's not only faux bold. Such overrun even happens with regular letters like
> 'm' with antialiasing (bug 734740) and tons of Unicode symbols (e.g. bug
> 781676).

Hmm ok. So let's keep this function alive for now.
 
> With this in mind, I've modified _vte_daw_get_char_width() to become a weird
> hybrid: it returns the width of the particular character plus the spacing.
> I've documented that above that method; not sure if I should emphasize it
> more, or even rename the method to make it more obvious.
> 
> With that in mind, is there still a newly introduced bug? I believe there's
> a missed optimization (something that overruns without spacing might not
> overrun with spacing), does it make sense to further optimize it? Will see
> if it's reasonably doable.
> 
> Or I can return the old behavior of this method, and the caller add x_offset.

Just returning (uinfo->width + x_offset) should do that optimisation, not? The callers check the returned for "> columns*cell_width", so if the 'overrun' fits into the extra spacing available at the right of the cell(s), then no overrun occurs.

(In reply to Christian Persch from comment #61)

> Well, by that I meant (columns * cell_width - x_spacing) wide, starting at
> x_offset.

... whereas x_spacing is currently unknown by VteTerminal. I can of course make it known (and y_spacing is known indirectly as cell_height - char_ascent - char_descent, so why not).

> Just returning (uinfo->width + x_offset) should do that optimisation, not?

That sounds like a terrible API. 

Btw, the same method is used to figure out the block/underline cursor width (larger than char/cell width if required to accomodate for the entire glyph), so we do need some reasonable API, we really shouldn't go with ugly hacks to please that particular optimisation.

/me crying loud, this is probably the most complex VTE change so far relative to the initial assumption, and the two-liner proof of concept in bug 738781 comment 1.

(In reply to Egmont Koblinger from comment #62)
> (In reply to Christian Persch from comment #61)
> 
> > Well, by that I meant (columns * cell_width - x_spacing) wide, starting at
> > x_offset.
> 
> ... whereas x_spacing is currently unknown by VteTerminal. I can of course
> make it known (and y_spacing is known indirectly as cell_height -
> char_ascent - char_descent, so why not).
> 
> > Just returning (uinfo->width + x_offset) should do that optimisation, not?
> 
> That sounds like a terrible API. 
> 
> Btw, the same method is used to figure out the block/underline cursor width
> (larger than char/cell width if required to accomodate for the entire
> glyph), so we do need some reasonable API, we really shouldn't go with ugly
> hacks to please that particular optimisation.

Hmm ok, let's go with the original return value then. It's not incorrect after all, just not optimal.

> /me crying loud, this is probably the most complex VTE change so far
> relative to the initial assumption, and the two-liner proof of concept in
> bug 738781 comment 1.

It's almost there :-)

> It's almost there :-)

I know, I know. :-) I'll do it (not today).

Seriously though, if it wasn't for accessibility, I'd consider to ditch all this work, remain with the simpler codebase and close as wontfix.

(In reply to Christian Persch from comment #63)

> Hmm ok, let's go with the original return value then. It's not incorrect
> after all, just not optimal.

Or we could make it return two values: the left and right edges of the glyph (and rename the function accordingly). This would allow to push wider glyphs slightly to the left (to occupy the left spacing of the current glyph, rather than the left spacing of the next one). Not sure if we should do this, though.

Created attachment 363026 [details] [review]
vte, v5 (part 3 only)

I think we're getting there, at least I quite like the current version :-)

For painting the cursor, we need to (at least indirectly) know the right and bottom offsets (spacings, paddings, call it whatever you want) as well. (Or move the painting of the cursor into vtedraw, I don't feel like doing such big refactorings.) I deliberately do not want to store char_{width_height} as that would potentially lead us to accidentally use that instead of cell_{width_height} in code added later. So I decided to store the amount of extra space in all four directions.

Similarly to the overall padding, I went for using a GtkBorder rather than 4 separate values, I think it's more readable this way. I also dared to use the name "m_char_padding" which is a flashback from the earlier idea of declaring the desired values in CSS. It's not strictly always a padding in its CSS meaning (wider glyphs might paint on it, CJK is even more complicated), but with ASCII codepoints it is the same, and I believe this naming helps mentally imagine what's going on.

I've made variables names more verbose at a few other places, hoping it improves readability. Added many new comments. Radius fixed. An additional cleanup: don't look up TAB's width in the font (mostly because in an intermediate version of the patch I broke the cursor's look over a TAB character; it should be fixed now).

I-beam and underline cursor fixed as discussed earlier. (Note: With giant horizontal spacing, underlining excluding the spacing looks way too narrow to me, whereas underlining the entire cell looks way too wide. I don't wish to further complicate the code by going for something in between [and properly take care of all those rounding off-by-ones]. Underline is the stupidest of all cursors, probably only exists in order to emulate VGA text mode, and especially people who need accessibility hopefully don't use it.)

Bug: When switching between different profiles having the same font but different spacing, geometry hints (resize increments) are not updated.

TerminalWindow needs to update its geometry on VteTerminal::char-size-changed for that to work, I think.

+    <key name="cell-height-scale" type="d">
+      <default>1.0</default>

You should also add minimum and maximum for these keys, with <range min="1.0" max="2.0" /> .

The bug also occurs if I change the spacing without leaving the windodw (geom hints are apparently reinstalled on focus in/out), that is, execute the corresponding dconf command from there.

I guess I forgot to emit char-size-changed on spacing change. But I'll double check your guess too.

Review of attachment 363026 [details] [review]:

::: src/vte.cc
@@ +7561,2 @@
 	}
+        if (memcmp(&char_spacing, &m_char_padding, sizeof(GtkBorder)) != 0) {

Should s/char_spacing/char_padding/ throughout the change.

(I changed my mind wrt the name, and mistakenly search-n-replaced the m_ prefixed ones only.)

(In reply to Egmont Koblinger from comment #67)
> Bug: When switching between different profiles having the same font but
> different spacing, geometry hints (resize increments) are not updated.

(In reply to Christian Persch from comment #68)
> TerminalWindow needs to update its geometry on
> VteTerminal::char-size-changed for that to work, I think.

I'm afraid it's going to be more complicated. Hooking up to char-size-changed might be a good workaround, but we're facing some deeper issue.

Let's talk about the case where you right-click and there select a new profile. This way it's known (well, initiated) by gnome-terminal, and even without extra spacing, the font size might change. Whether the overall cell size changes due to font change or due to spacing change should be irrelevant to gnome-terminal.

When I change profile, pretty often vte_terminal_get_char_{width,height}() are called twice, once with the old profile and once with the new. In these cases updating the geometry goes as expected.

Sometimes though, the infamous warning
(gnome-terminal-server:18455): Gtk-WARNING **: Allocating size to GtkScrollbar 0x56284e892230 without calling gtk_widget_get_preferred_width/height(). How does the code know the size to allocate?
gets printed, and vte_terminal_get_char_{width,height}() are not called at all. This is when resize increments go wrong.

Suggests to me that this is not only an innocent warning, but stops GTK+ from executing all pending things it needs to execute (or the other way around: something else failing triggers this warning).

I'm pretty sure this issue can also be triggered without extra spacings.

I think we should really get to the bottom of these warnings.

I'm wondering if as per this random finding: https://github.com/budgie-desktop/budgie-desktop/commit/ff3d7c9c it would be good enough just to call the requested methods and ignore their return values?

Just these minor comments:

+ _vte_draw_get_char_edges(m_draw, cell->c, columns, style, NULL, &right);
+ if (right > m_cell_width * columns) {
+   columns++;
+ }

I know the current code is the same, but shouldn't this be simply 

 columns = right/m_cell_width ?

(Ie in case columns==1 and char is a non-BMP unknown char whose hexbox is almost 3 columns wide.)

+_vte_draw_get_char_edges (m_draw, cell->c, cell->attr.columns, style, NULL, &r);
+cursor_width = MAX(cursor_width, r);

That can make the cursor_width a non-integral multiple of cell width, no? IMHO should round up to the nearest full cell.

(In reply to Egmont Koblinger from comment #72)
> Sometimes though, the infamous warning
> (gnome-terminal-server:18455): Gtk-WARNING **: Allocating size to
> GtkScrollbar 0x56284e892230 without calling
> gtk_widget_get_preferred_width/height(). How does the code know the size to
> allocate?
> gets printed, and vte_terminal_get_char_{width,height}() are not called at
> all. This is when resize increments go wrong.
> 
> Suggests to me that this is not only an innocent warning, but stops GTK+
> from executing all pending things it needs to execute (or the other way
> around: something else failing triggers this warning).
> 
> I'm pretty sure this issue can also be triggered without extra spacings.
> 
> I think we should really get to the bottom of these warnings.
> 
> I'm wondering if as per this random finding:
> https://github.com/budgie-desktop/budgie-desktop/commit/ff3d7c9c it would be
> good enough just to call the requested methods and ignore their return
> values?

I don't see how that patch could fix anything, not even make the warning go away (since it's just chaining up to the parent classes vfunc...) ?

Also in vte-2.91/g-t's case it's a stock GtkScrollbar that's spewing the warning, nothing we can change from outside...

I too would like to track these down, but let's move that to a new bug.

(In reply to Christian Persch from comment #73)
> I know the current code is the same, but shouldn't this be simply 
> 
>  columns = right/m_cell_width ?

Well, rounded *up*, of course.

Comment on attachment 361961 [details] [review]
gnome-terminal

With <range> added, ok to commit.

(In reply to Christian Persch from comment #73)

> + _vte_draw_get_char_edges(m_draw, cell->c, columns, style, NULL, &right);
> + if (right > m_cell_width * columns) {
> +   columns++;
> + }
> 
> I know the current code is the same, but shouldn't this be simply 
> 
>  columns = right/m_cell_width ?
>
> Well, rounded *up*, of course.

At one point I changed that "if" to "while" which is the lazy man's implementation of rounding up :) Then I reverted, not sure why. (I guess it was related to the cursor over the tab character being broken in that work-in-progress version and me hunting down that bug.)

Will do it properly.

> +cursor_width = MAX(cursor_width, r);
> 
> That can make the cursor_width a non-integral multiple of cell width, no?
> IMHO should round up to the nearest full cell.

That would be a noticeable user-visible change, and not to the better. In many fonts, many single-wide characters are a bit wider than a cell. Currently the cursor is extended to cover the entire glyph. This "bit" wider could even be as small as a single additional pixel, hardly noticeable. Suddenly turning them to double-wide would look ugly (as it would overlap another glyph) but would also be quite misleading (users could think it's logically double-wide).

> I don't see how that patch could fix anything, not even make the warning go
> away (since it's just chaining up to the parent classes vfunc...) ?
> 
> Also in vte-2.91/g-t's case it's a stock GtkScrollbar that's spewing the
> warning, nothing we can change from outside...

Well, according to the warning's wording, GTK keeps track of whether we call that function or not. And then it might do other nasty things as well.

Anyway, I agree that this should go to another bug.

Sorry for the long delay.

> > I know the current code is the same, but shouldn't this be simply 
> > 
> >  columns = right/m_cell_width ?
> >
> > Well, rounded *up*, of course.

This was the only pending issue, right? (Apart from the geometry update one, which we'll fork to a new bug.) If so, I'm not attaching a new patch, but I'll do this (twice in the code):

-       if (right > m_cell_width * columns) {
-               columns++;
-       }
+       columns = MAX(columns, howmany(right, m_cell_width));

Most likely a

+       columns = howmany(right, m_cell_width);

would also be correct, I'm just not brave enough for that :)

(In reply to Egmont Koblinger from comment #77)

> Most likely a
> 
> +       columns = howmany(right, m_cell_width);
> 
> would also be correct, I'm just not brave enough for that :)

Actually I'm not even sure about it. It might not properly invalidate a nondefault background color, or underline/strikethrough. Let's remain safe and go with MAX().

Another tiny fix I'd like to add:

-        m_underline_position = char_spacing.top + MIN (char_ascent + m_line_thickness, char_height - m_line_thickness);
+        m_underline_position = MIN (char_spacing.top + char_ascent + m_line_thickness, cell_height - m_line_thickness);

This allows the underline to occupy the extra spacing below the character's descent area, rather than being too close to the letters. This will be important for curly underline (which gets taller as you increase letter spacing). I'd like the simple underline to follow the same pattern we'll have for the curly one, and I don't want to modify this line of code in that patch :)

(In reply to Egmont Koblinger from comment #77)
> Sorry for the long delay.
> 
> > > I know the current code is the same, but shouldn't this be simply 
> > > 
> > >  columns = right/m_cell_width ?
> > >
> > > Well, rounded *up*, of course.
> 
> This was the only pending issue, right? (Apart from the geometry update one,
> which we'll fork to a new bug.) 

IIRC yes, that's it.

(In reply to Egmont Koblinger from comment #78)
> (In reply to Egmont Koblinger from comment #77)
> 
> > Most likely a
> > 
> > +       columns = howmany(right, m_cell_width);
> > 
> > would also be correct, I'm just not brave enough for that :)
> 
> Actually I'm not even sure about it. It might not properly invalidate a
> nondefault background color, or underline/strikethrough. Let's remain safe
> and go with MAX().

Ok.

(In reply to Egmont Koblinger from comment #79)
> Another tiny fix I'd like to add:
> 
> -        m_underline_position = char_spacing.top + MIN (char_ascent +
> m_line_thickness, char_height - m_line_thickness);
> +        m_underline_position = MIN (char_spacing.top + char_ascent +
> m_line_thickness, cell_height - m_line_thickness);
> 
> This allows the underline to occupy the extra spacing below the character's
> descent area, rather than being too close to the letters. This will be
> important for curly underline (which gets taller as you increase letter
> spacing). I'd like the simple underline to follow the same pattern we'll
> have for the curly one, and I don't want to modify this line of code in that
> patch :)

Ok with that.

Thanks!

Submitted.

> I too would like to track these down, but let's move that to a new bug.

Filed bug 791226.

For reference, here's how to increase line spacing or character spacing, as there's no UI for that (yet):

dconf write /org/gnome/terminal/legacy/profiles:/:b1dcc9dd-5262-4d8d-a863-c897e6d979b9/cell-height-scale 1.5

dconf write /org/gnome/terminal/legacy/profiles:/:b1dcc9dd-5262-4d8d-a863-c897e6d979b9/cell-width-scale 1.2

Replace the Profile ID with your actual value which you might take e.g. from the Profile Preferences dialog. (Chances are it'll be the same as the one above.)

Valid floating point values go from 1.0 (the default) to 2.0 (double spacing).

Filed bug 791968 for adding a UI.