[vim/vim] Add support for alternate font highlighting (PR #13537)
Peter Munch-Ellingsen
This adds support for alternate font highlighting using CSI SGR 10-19. Few terminals currently support this, but with added tool support this should improve over time. The change here is more or less taken from how colors are configured and applied, but there might be some parts I missed while implementing it. Changing fonts is done through the new ctermfont attribute which takes a number, 0 is the normal font, and the numbers 1-9 select an "alternative" font. Which fonts are in use is up to the terminal.
An example of what the result looks like can be found in my patched version of pangoterm which I used to implement this feature:
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/13537
Commit Summary
- 36465d1 Add support for alternate font highlighting
File Changes
(5 files)
- M src/highlight.c (59)
- M src/proto/term.pro (1)
- M src/screen.c (2)
- M src/structs.h (1)
- M src/term.c (9)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
oh wow nice.
However, this is missing documentation. And tests, you can possibly use synIDattr() for this. Of course we would need support for retrieving those special highlighting attributes there as well. 😬
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch pushed 1 commit.
- 8d7fda4 Remove unused parameter
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
Yes, I assumed there where things I had missed. But before I put more work into it I wanted to get some feedback. I'll look into synIDattr() and write some documentation.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch pushed 2 commits.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
Added a testcase based on the one for underline color, and it uses synIDattr. Also threw in a short section in the docs explaining the new option.
Not exactly sure why the CI failed, make test passes for me locally. If you have any info please share so I can get it fixed.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch pushed 1 commit.
- 3218aa4 Remove superfluous >= 0 check
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch pushed 1 commit.
- df68be7 Add ctermfont to tags list
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
thanks appreciate the quick changes. Not sure about the test failures, could it be, that the tests now get an additional highlighting attribute they did not expect?
I think however it would make sense to add a special termcap entry, like t_f8, check e.g. this commit e023e88 (see the changes in term.c and term.h and also the included doc changes).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
The tests are looking better now. One test times out, the other fails for what appears to be completely unrelated issues (maybe I chose a bad commit to branch out from?).
I'll look into the termcap entry thing tomorrow, thanks for taking the time to reply.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
@benknoble commented on this pull request.
> @@ -5269,6 +5269,13 @@ ctermul={color-nr} *highlight-ctermul*
command is given. If the Normal group colors are changed later, the
"fg" and "bg" colors will not be adjusted.
+ctermfont={font-nr} *highlight-ctermfont*
+ This gives the alternative font number to use in the terminal. The
+ available fonts depend on the terminal, and if the terminal is not set
+ up for alternative fonts this simply won't do anything. The range of
+ {font-nr} is 0-9 where 0 resets the font to the default font and 1-9
+ selects one of the 8 alternate fonts. For more information see your
+ terminals handling of SGR parameters 10-19.
⬇️ Suggested change
- terminals handling of SGR parameters 10-19. + terminal's handling of SGR parameters 10-19.
This is possessive.
In src/highlight.c:
> @@ -1034,6 +1035,38 @@ highlight_set_ctermul(int idx, int color, int is_normal_group) hl_set_ctermul_normal_group(color); } +/* + * Set the cterm font for the highlight group at 'idx'. + * 'arg' is the color name or the numeric value as a string. + * 'is_normal_group' is set if the highlight group is 'NORMAL'
This parameter was removed.
In src/highlight.c:
> @@ -1144,7 +1177,6 @@ highlight_set_cterm_color(
return TRUE;
}
-
I think this was unintended.
In src/highlight.c:
> @@ -2948,6 +2995,9 @@ highlight_list_one(int id)
sgp->sg_cterm_bg, NULL, "ctermbg");
didh = highlight_list_arg(id, didh, LIST_INT,
sgp->sg_cterm_ul, NULL, "ctermul");
+ if (sgp->sg_cterm_font != 0)
+ didh = highlight_list_arg(id, didh, LIST_INT,
+ sgp->sg_cterm_font + 1, NULL, "ctermfont");
Since it's different from all the rest, why + 1? Upon closer inspection, it seems related to the tests, but I don't see the relation.
In src/highlight.c:
> return NULL;
if (modec == 'c')
{
if (fg)
n = HL_TABLE()[id - 1].sg_cterm_fg - 1;
else if (ul)
n = HL_TABLE()[id - 1].sg_cterm_ul - 1;
+ else if (font)
+ n = HL_TABLE()[id - 1].sg_cterm_font;
And then no -1 here? I don't understand the representations, so this is mostly curiosity. Probably also test-related, however.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch commented on this pull request.
> @@ -5269,6 +5269,13 @@ ctermul={color-nr} *highlight-ctermul*
command is given. If the Normal group colors are changed later, the
"fg" and "bg" colors will not be adjusted.
+ctermfont={font-nr} *highlight-ctermfont*
+ This gives the alternative font number to use in the terminal. The
+ available fonts depend on the terminal, and if the terminal is not set
+ up for alternative fonts this simply won't do anything. The range of
+ {font-nr} is 0-9 where 0 resets the font to the default font and 1-9
+ selects one of the 8 alternate fonts. For more information see your
+ terminals handling of SGR parameters 10-19.
Whoops, bit of quick writing there
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch commented on this pull request.
In src/highlight.c:
> @@ -2948,6 +2995,9 @@ highlight_list_one(int id)
sgp->sg_cterm_bg, NULL, "ctermbg");
didh = highlight_list_arg(id, didh, LIST_INT,
sgp->sg_cterm_ul, NULL, "ctermul");
+ if (sgp->sg_cterm_font != 0)
+ didh = highlight_list_arg(id, didh, LIST_INT,
+ sgp->sg_cterm_font + 1, NULL, "ctermfont");
The others use 0 as a sentinel value as far as I can tell, while ctermfont has 0 as a valid value. All the others therefore add one to bring the valid values into a certain range, and then subtract it later. I could probably do a similar thing for ctermfont come to think of it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch pushed 2 commits.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
Pushed some changes now based on the comments from @benknoble. Tested it in my patched pangoterm and everything seems to still be working as expected. All the related tests seemed fine, but I got some issues with the UI tests that I think is just down to this machine really needing a complete reinstall. So I decided to push it here and see what the actual complete tests suite has to say.
Also looked into the termcap entry thing, not sure how relevant it is in this case since so very few terminals even implement this feature. As far as I could tell termcap is more about bridging the gap between terminals with different behaviours, and I haven't found any terminal which implements this in a different way from the SGR codes used here (and even that is only properly supported by my patched Pangoterm it seems).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Well, as far as I know, t_f8 (and the similar setting for the background color) is not part of termcap either. But it's good to be able to manually override it in case one needs it anyhow.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shad
I've tested theses changes with mintty and noticed that if I try to assign a ctermfont without any color it does't work. As soon as I assign ctermfont along a ctermul, guisp, ctermfg or ctermbg the font is correctly modified
Can the number of alternate fonts can be extended from 9 to 10 so the SRG 20 [Fraktur] (Gothic) can be also used? I've tested it and it works fine on mintty (where I only have 5 alternate font configured and the gothic one is automaticaly found on the system by mintty)
On the right, the file without fonts modification and on the left, the applied fonts modification with my experiment of SRG20 on line 10
as you can see in the file, ctermfont is not recognised as correct option, I don't know if the runtime should be modified along this patch or in a separate process.
If you want to reproduce my tests, here is my minttyrc configuration:
Font=FiraCode Nerd Font Mono Font1=Monaspace Neon Var Font2=Monaspace Argon Var Font3=Monaspace Xenon Var Font4=Monaspace Radon Var Font5=Monaspace Krypton Var #Font10 is not configured and mintty search for Fraktur or Blackletter font (ECMA-48 "Gothic") FontHeight=12
and the test.vim file you just need to source to apply font modification
hi String ctermfont=1 hi Identifier ctermfont=2 hi Type ctermfont=3 hi Comment ctermfont=4 hi Error ctermfont=5 " ctermfont let g:ctermfont \ = 'ctermfont' syn keyword ctermfont \ __ctermfont__ hi ctermfont ctermfg=7 \ ctermfont=10
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shad
@shadowwa commented on this pull request.
> + {font-nr} is 0-9 where 0 resets the font to the default font and 1-9
+ selects one of the 8 alternate fonts. For more information see your
if alternate fonts are from 1 to 9 there should be 9 of them not 8
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Shad
@shadowwa commented on this pull request.
In src/screen.c:
> @@ -1667,6 +1667,8 @@ screen_start_highlight(int attr)
*/
if (aep != NULL)
{
+ if (aep->ae_u.cterm.font > 0 && aep->ae_u.cterm.font < 11)
I've successfully tested the SRG20 (Gothic font) by extending the font limit by 1
⬇️ Suggested change- if (aep->ae_u.cterm.font > 0 && aep->ae_u.cterm.font < 11) + if (aep->ae_u.cterm.font > 0 && aep->ae_u.cterm.font < 12)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
Good catch about the Fraktur font, wasn't aware that it was implemented anywhere!
I'll have a look at the issue you mentioned about it not applying unless you also have a color tomorrow.
And I'll implement the termcap thing and write some more tests as well if I find the time.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch pushed 3 commits.
- 2b81cfd Add support for Fraktur font
- 0eeab54 More consistency, fix bug where ctermfont had to be combined with other properties to render
- 6805481 Implement reset by setting to NONE
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@PMunch pushed 1 commit.
- ef4245e Add highlighting of ctermfont to runtime
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
@shadowwa, I managed to reproduce the bug where you had to combine ctermfont with other styles to make it render. I also fixed an issue where you couldn't use ctermfont=NONE to reset the font back to normal. I also added support for the SGR 20 Fraktur option.
Also added the highlighting, so you you point your testing instance to the repo-local version it will highlight properly.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
Not entirely sure why it says I requested a review. Is this an automatic thing, or did I hit something on accident? If it's the latter I'm very sorry for troubling you.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Peter Munch-Ellingsen
Anything left on this which you'd want me to fix apart from more tests? If not could you run through the coverage workflow again so I can see the results?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
I think this comment of mine still applies: #13537 (comment)
In general, I think this is quite nice. It's on my list to include after the Vim9.1 release.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
Can we think about how gVim part of this works? At least the interface of how it's supposed to work. In highlighting, we have ctermfg and guifg and I think we should have something similar. One issue is I'm not sure if using font 1-9 makes sense in GUI, and it depends if we want to use a syntax guifont=5 or guifont=FiraCode (which would allow any number of fonts to be used).
FWIW I have been thinking about this topic a bit as I was going to propose a couple modifications to the guifont specification which is why I want to bring it up now. For reference, the modifications is:
- we can change
guifont=Font1,Font2,Font3to be a list of fallback fonts, instead of just "pick the first font that exists". This allows us to have multiple fonts at once instead of needing to rely on patched fonts. Some terminals already allow this. - Specify different fonts for bold/italics/etc. For example, this allows you to use a different weight variant of a font when bolding instead of just letting the OS font system pick it for you.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
I haven't forgotten about it. But I haven't had yet enough time to go over the implementation yet. It's still on my list however.
Regarding @ychin comment:
we can change guifont=Font1,Font2,Font3 to be a list of fallback fonts, instead of just "pick the first font that exists". This allows us to have multiple fonts at once instead of needing to rely on patched fonts. Some terminals already allow this.
Are you talking about the guifont option? Or is this about a potential new argument for the :hi ex command? Currently it already has the font attribute, which according to the documentation should be used for the GUI.
Specify different fonts for bold/italics/etc. For example, this allows you to use a different weight variant of a font when bolding instead of just letting the OS font system pick it for you.
I believe this will cause troubles with the current font rendering and the fixed width cells, that is used by the GUI. We will probably see rendering artifacts or strange spacing issues. I don't think this is worth the trouble
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
Currently it already has the font attribute, which according to the documentation should be used for the GUI. Oh and how does this relate to terminals?
Oh wait you are right! I forgot this already existed. One thing I noticed is that it doesn't work for GTK build right now (see https://github.com/vim/vim/blob/93197fde0f1db09b1e495cf3eb14a8f42c318b80/src/gui.c#L2363-L2370). Seems like MacVim never supported this. I'll investigate on my side.
Are you talking about the
guifontoption? Or is this about a potential new argument for the:hiex command?
I'm talking about the original guifont option, not the new proposal. I was mostly mentioning it since it's in the vicinity. I may take a look both my proposal and GTK's lack of supporting of highlight fonts and see if they could unified in one change.
I believe this will cause troubles with the current font rendering and the fixed width cells, that is used by the GUI. We will probably see rendering artifacts or strange spacing issues. I don't think this is worth the trouble
I guess this is platform dependent but we already have to deal with this for guifontwide, or this specific PR itself (using different fonts for certain highlighted texts). But each platform can selectively support it. I think some platforms (i think the Windows gVim) has tighter requirement on the fixed width. What I'm proposing is to provide a syntax to specify such information itself. Gui fonts right now has always been a somewhat platform-dependent feature.
But I probably care about this less than the ability to set fallback fonts, which I think is very useful (the existence and whole idea of needing patched fonts still annoys me).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.