-
Notifications
You must be signed in to change notification settings - Fork 8.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for the "italic" graphic rendition attribute #8580
Conversation
d5e2966
to
966f748
Compare
I've had it on my TODO list for a long time to implement "cascading FontInfo", where a Renderer consumer could set up a group of fontinfos and have attribute-specific overrides only to meaningful properties (like, "italic" is "override nothing except this flag in the font lookup"; bold is "set face name to Xxx Bold, keep size and slant"). It's always conceptually been a blocker for me to have actual styling... but look, here it is and working today, and I'm not going to stand in the way of progress and hold out for some "ideal world" where our designs are perfect and community-ready at all times. That's a lot of words for me to say to explain why I'm 100%, or maybe 150%, okay with this... so that folks on the team who know how I felt about FontInfo shouldn't be worried about how I'll feel. 😀 |
I'm was just going to say this is probably not a great approach, and I don't really know what I'm doing on the DX side of things, but I thought it was at least worth offering as a PR. If nothing else it could serve as a temporary solution until you have something better. |
The DX side of things is close with one observation that wouldn't necessarily be immediately apparent: A I would greatly prefer if both reg/italic font/formats were pushed into the Whether or not it's italic at the moment could be stored in the |
OK. That sounds good. I'll give it a try. |
_hfont((HFONT)INVALID_HANDLE_VALUE) | ||
_hfont(nullptr), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this was a deliberate change - the correct error value for a font is null rather than INVALID_HANDLE_VALUE
. This was previously generating error logs when the font got deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woopsie. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
The more I look at the DX renderer, the less I feel I know what's going on, but I think I've got something working now with a single My one concern is that I'm not sure the block characters are handled correctly when italic. On the plus side, they have less render artifacts than the non-italic variant, but I think they're more likely to have gaps between blocks in some situations. That said, I wouldn't expect people to be using block characters in italics anyway, so I don't think it's a big deal. But that brings me to another issue: quite a few fonts that I've tested actually render the block characters at an angle when italic, which doesn't really make any sense. The line characters don't connect vertically, and the blocks get sort of chopped off at the edges. Technically I think this is the font's responsibility, but it is probably something we could fix (at least in the DX renderer) if we really wanted to. In practice I think it's unlikely to be a problem though. |
I'll take a look.
I agree.
Thanks. It looks close enough for now. We can revisit block drawing in a different bug, not as a part of implementing italics if it becomes a big enough problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's more or less what I meant by the comment a few days ago. Looks pretty fine to me. Just the one outstanding discussion point on the bare vs. Com pointer. I think @DHowett will have an opinion or an insight that will help clear that one up and then we can approve and get this in.
src/renderer/dx/CustomTextLayout.cpp
Outdated
@@ -1226,7 +1242,7 @@ CATCH_RETURN(); | |||
{ | |||
// Get the font fallback first | |||
::Microsoft::WRL::ComPtr<IDWriteTextFormat1> format1; | |||
if (FAILED(_format.As(&format1))) | |||
if (FAILED(::Microsoft::WRL::ComPtr<IDWriteTextFormat>(_formatInUse).As(&format1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this strange, then I realized you're storing the InUse
ones as a bare pointer, instead of as a ComPtr
. Given the ComPtr
is just a refcount anyway... should we maybe just store it that way so there are 2 refs outstanding for holding it twice in our class? I don't think that should be THAT harmful to performance overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I went back and forth with this a few times, but settled on the bare pointer in the end because it felt a bit wasteful having that extra ref counting for something that's entirely internal to the class. I wouldn't object to going back to a ComPtr
if that's what you prefer though. Just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lifetime is held by other member variables. I am comfortable with a bare pointer here.
Anyway, it will be instantly promoted to an owning reference (of TextFormat1) here as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to make this a little "clearer" (clearer to a COM person; instead of using a constructor call with _formatInUse
), you can do this:
if (FAILED(_formatInUse->QueryInterface(IID_PPV_ARGS(&format1))))
IID_PPV_ARGS
is a macro that takes a pointer to a COM thing and transforms it into an interface ID for that pointed-to type + the type as an argument pack.
the &
operator on ComPtr acts roughly as expected of COM things.
QueryInterface returns a positive reference counted object into the pointed-to location so it's consistent with what WRL needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I'm good with what Dustin says then. Bare pointer is fine with me and you can optionally make it "clearer to a COM person" per his suggestion.
_hfont((HFONT)INVALID_HANDLE_VALUE) | ||
_hfont(nullptr), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woopsie. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally comfortable with this. Minor readability nit (though whether you consider IID_PPV_ARGS
more or less readable is rather up to you!)
src/renderer/dx/CustomTextLayout.cpp
Outdated
@@ -1226,7 +1242,7 @@ CATCH_RETURN(); | |||
{ | |||
// Get the font fallback first | |||
::Microsoft::WRL::ComPtr<IDWriteTextFormat1> format1; | |||
if (FAILED(_format.As(&format1))) | |||
if (FAILED(::Microsoft::WRL::ComPtr<IDWriteTextFormat>(_formatInUse).As(&format1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to make this a little "clearer" (clearer to a COM person; instead of using a constructor call with _formatInUse
), you can do this:
if (FAILED(_formatInUse->QueryInterface(IID_PPV_ARGS(&format1))))
IID_PPV_ARGS
is a macro that takes a pointer to a COM thing and transforms it into an interface ID for that pointed-to type + the type as an argument pack.
the &
operator on ComPtr acts roughly as expected of COM things.
QueryInterface returns a positive reference counted object into the pointed-to location so it's consistent with what WRL needs.
_hfont((HFONT)INVALID_HANDLE_VALUE) | ||
_hfont(nullptr), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
@@ -300,6 +322,16 @@ GdiEngine::~GdiEngine() | |||
// Save the font. | |||
_hfont = hFont.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is hilarious that we used the smart pointer types for all of the 8 seconds required to ... decapsulate them and store them as raw handles in our class. LOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I mean. I'm sure I had good intentions like "it'll be released properly if one of the interim steps fails and the RETURN_IF*
macro fires. But it is pretty silly. The class should probably have some smart pointers too... future fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good. James, if you want to fix the readability nit that Dustin called out, go for it. I'll wait a bit to merge this in case you want that chance.
I was planning to make the change, but I'm still at work so won't be able to do anything until a little later tonight. I don't mind if you want to merge though. |
Will there be a Faux Italic as well as a Faux Bold option when the chosen font does not include those varients? |
@mdtauk This is not intended to be configurable right now. If DirectWrite or GDI chooses to make a faux-italic variant of a font, that is its right. Cascadia doesn't have an italic variant, so the one pictured above is faux-italic. |
@j4james if that's the only change, I'll make it on your branch & merge. 😄 Thanks |
So this is something handled by DirectWrite then, fair enough. |
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Hmm. Our agents are on the floor. |
) This PR adds support for the ANSI _italic_ graphic rendition attribute, which is enabled by the `SGR 3` escape sequence. For the GDI renderer, I've just created an additional italic variant of the font, and then the `UpdateDrawingBrushes` method selects the appropriate font variant into the device context based on the requested text attributes. It's a bit more complicated in the DX renderer, because we need both an italic variant of the font, and a variant of the text format object. The `CustomTextLayout` class also had to be updated to hold the two font and format instances, and decide which of the variants to use based on a `useItalicFont` property in the drawing context, initially set in the `UpdateDrawingBrushes` method. ## Validation Steps Performed I've created some test content using a range of different character sets (e.g. CJK, block characters, emoji, etc.), then applied the italic attribute mixed with various other SGR attributes to see how they interact. The output isn't always perfect, but I think it seems reasonable given the constraints of a cell-based terminal renderer. Closes microsoft#5461
This is my attempt to isolate all the dwrite font related thing by introducing a new layer - `DxFontRenderData`. This will free `DxRenderer` & `CustomTextLayout` from the burden of handling fonts & box effects. The logic is more simplified & streamlined. In short I just moved everything fonts-related into `DxFontRenderData` and started from there. There's no modification to code logic. Just pure structural stuff. SGR support tracking issue: #6879 Initial Italic support PR: #8580
This PR Introduces `DxFontInfo` to simplify the logic in `DxFontRenderData`. `DxFontInfo` aims to be the DWrite equivalent of `FontInfo` & `FontInfoBase` in GDI. It encapsulates the needed information to represent a displayable font face. It also provides the ability to resolve a font face based on the available fonts on the system. ## References This is a follow-up of #9096. Initial Italic support was introduced by #8580. The motivation behind this is to support bold & bold-italic text in Windows Terminal.
This PR adds support for the ANSI italic graphic rendition attribute,
which is enabled by the
SGR 3
escape sequence.For the GDI renderer, I've just created an additional italic variant of
the font, and then the
UpdateDrawingBrushes
method selects theappropriate font variant into the device context based on the requested
text attributes.
It's a bit more complicated in the DX renderer, because we need both an
italic variant of the font, and a variant of the text format object. The
CustomTextLayout
class also had to be updated to hold the two font andformat instances, and decide which of the variants to use based on a
useItalicFont
property in the drawing context, initially set in theUpdateDrawingBrushes
method.Validation Steps Performed
I've created some test content using a range of different character sets
(e.g. CJK, block characters, emoji, etc.), then applied the italic
attribute mixed with various other SGR attributes to see how they
interact. The output isn't always perfect, but I think it seems
reasonable given the constraints of a cell-based terminal renderer.
Closes #5461