Thanks for the patch!

A few questions / comments:

* Do you require this for a specific application (if so, which one) ? 

* Does xterm implement this?

* Your patch treats italic and bold as exclusive properties, but I don't see why they couldn't be combined (so a cell could be bold *and* italic).

From ECMA 48:

8.3.117
SGR - SELECT GRAPHIC RENDITION
Notation: (Ps...)
Representation: CSI Ps... 06/13
Parameter default value: Ps = 0
SGR is used to establish one or more graphic rendition aspects for subsequent 
                         ^^^^^^^^^^^
text. The established
aspects remain in effect until the next occurrence of SGR in the data stream, depending on the setting of
the GRAPHIC RENDITION COMBINATION MODE (GRCM). Each graphic rendition aspect is
specified by a parameter value:

1 bold or increased intensity
2 faint, decreased intensity or second colour
3 italicized
[...]

So in this code:
+		case 3:
+			terminal->pvt->screen->defaults.attr.italic = 1;
+			terminal->pvt->screen->defaults.attr.bold   = 0;
+			terminal->pvt->screen->defaults.attr.half   = 0;

the last 2 lines shouldn't be there, I think, and the font code should handle bold+italic too...

* As for the other code that's now passing around cell->attr.bold and cell->attr.italic, maybe would be better to pass around the full cell->attr instead?

Hey,

I actually just wanted to have italics support in vim like with rxvt-unicode.

The reason why I didn't allow both italic and bold, was that it would have required a third parameter or a heavy modification of the code. (and really italic-bold is a corner case for terminals) But you are right, passing the attr instead of bold/italic would really be the clean way to do it.

Actually I just discovered a strange bug. It seems the order of the attributes in VteCellAttr matters. Naturally I put italic right under bold, but that breaks the color palette. Does some code touch this struct with raw bit-access?

Anyway, I'm going to restructure the patch to address the issues you mentioned.

Best Regards,
Patrick

I don't think the order should matter; maybe a build problem?

You can add the guint italic:1 at the end of the struct, luckily there is exactly 1 bit free until it overflows 32 bits :-)

(In reply to comment #2)
> Actually I just discovered a strange bug. It seems the order of the attributes
> in VteCellAttr matters. Naturally I put italic right under bold, but that
> breaks the color palette. Does some code touch this struct with raw bit-access?

Did you update basic_cell to match?

Created attachment 225724 [details] [review]
The updated patch.

(In reply to comment #4)
> 
> Did you update basic_cell to match?

Thank you, this was causing the problem. :-)

I updated my patch to also support bold italic text. I had to do some larger changed in vtedraw.c and a few text style flags to vtedraw.h VTE_DRAW_(NORMAL|BOLD|ITALIC). The patch should now work as intended.

Cheers, Patrick

Created attachment 225727 [details] [review]
The updated patch.

Just realized that I accidentally changed the threshold for the rejection of bold fonts.

There are a few code style problems with the patch (e.g. we don't use declarations in the middle of code blocks, since that doesn't work with C89), but that's minor. I worry a bit about doubling the number of fonts used unconditionally even if no italic is actually used... Behdad, is that a significant memory problem?

The most important problem however is that it simply doesn't work with our default TERM setting. I have to use TERM=rxvt-unicode for it to work; neither xterm nor xterm-256color works. However, we do have to figure out our TERM story anyway for the 256 color problem (fedora patching this downstream).

Doubling fonts shouldn't be a problem.  Fonts are fairly light-weight these days.

Created attachment 225854 [details] [review]
New version.

Fixed some coding style problems, double-striking italic text should now also work better.

Review of attachment 225854 [details] [review]:

Thanks for the updated patch! Looking mostly good; just a few comments below.

First a general question: the bold fonts are checked against the non-bold variants to see if they're too wide; should we also check the italic non-bold font against the normal non-bold font to see if italic is too wide? And then implement fallback faux-italic by slanting?

::: src/vte.c
@@ +10117,3 @@
 		if (clear && (draw_default_bg || bg != defbg)) {
+			gint bold_offset = _vte_draw_has_bold(terminal->pvt->draw,
+									VTE_DRAW_BOLD) ? 0 : bold;

In case we decide to do fallback italic, this should use the style (ie potentially italic and/or bold) instead of just BOLD, I think:

gint bold_offset = _vte_draw_has_font_for_style(terminal->pvt->draw, style) ? 0 : (bold || italic)

(and compute style beforehand since it's now used twice in this func) ?

@@ +10524,3 @@
 				if (back != VTE_DEF_BG) {
+					gint bold_offset = _vte_draw_has_bold(terminal->pvt->draw,
+											VTE_DRAW_BOLD) ? 0 : bold;

Same comment here.

::: src/vterowdata.h
@@ +54,3 @@
 	guint32 fore: 9;	/* Index into color palette */
 	guint32 back: 9;	/* Index into color palette. */
 

Since the 32 bits are now full, delete the 

       /* unused; bug 499893
        guint32 protect: 1;
         */

        /* 30 bits */

comments at the end.

::: src/vteseq.c
@@ +2299,3 @@
+		case 3:
+			terminal->pvt->screen->defaults.attr.italic = 1;
+			break;

Need to also implement the sequence to turn italic off again, which is case 23 (as per ECMA 48).

I fixed the last two comments and pushed to vte-0-34 and vte-next branch.

The question from comment 11 remains however.

Also I noticed sth odd: when I set sth italic in vim (running under TERM=rxvt-unicode), it looks like it turns bold+italic instead of just italic...

(In reply to comment #12)
> I fixed the last two comments and pushed to vte-0-34 and vte-next branch.
> 

Thank you very much. :-)

> The question from comment 11 remains however.
>

Personally I don't think a terminal should emulate every style of a font. Bold is a rather special case, because it can be easily emulated, most people don't notice the difference (compared to the real bold variant) and it is widely used.
That said, I have tested fonts with more than 10% size difference and up to 15% it still looks better IMHO to force the bold font in the normal-sized cell grid. (Observe for example Inconsolata with 14%)

> Also I noticed sth odd: when I set sth italic in vim (running under
> TERM=rxvt-unicode), it looks like it turns bold+italic instead of just
> italic...

I have tested vte-0-34 with gnome-terminal gnome-3-6 and can't observe this effect. What font are you using? With Inconsolata vim displays italic text correctly.

You're right, it works with Inconsolata but not with (default) "Monospace". Weird, but not a vte bug then.

Now we need to get the ncurses terminfo for 'vte' updated upstream so it includes the italic capability.

https://lists.gnu.org/archive/html/bug-ncurses/2012-10/msg00001.html