-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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 custom box drawing and powerline glyphs #16729
Changes from 2 commits
ad2391c
3be7d11
710eb1b
ac1c8b6
7fc081a
5606691
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,21 +10,28 @@ namespace til | |
inline constexpr wchar_t UNICODE_REPLACEMENT = 0xFFFD; | ||
} | ||
|
||
static constexpr bool is_surrogate(const wchar_t wch) noexcept | ||
constexpr bool is_surrogate(const auto wch) noexcept | ||
{ | ||
return (wch & 0xF800) == 0xD800; | ||
} | ||
|
||
static constexpr bool is_leading_surrogate(const wchar_t wch) noexcept | ||
constexpr bool is_leading_surrogate(const auto wch) noexcept | ||
{ | ||
return (wch & 0xFC00) == 0xD800; | ||
} | ||
|
||
static constexpr bool is_trailing_surrogate(const wchar_t wch) noexcept | ||
constexpr bool is_trailing_surrogate(const auto wch) noexcept | ||
{ | ||
return (wch & 0xFC00) == 0xDC00; | ||
} | ||
|
||
constexpr char32_t combine_surrogates(const auto lead, const auto trail) | ||
{ | ||
// Ah, I love these bracketed C-style casts. I use them in C all the time. Yep. | ||
#pragma warning(suppress : 26493) // Don't use C-style casts (type.4). | ||
return (char32_t{ lead } << 10) - 0x35FDC00 + char32_t{ trail }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. horrifying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but i love it as long as it works There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not too bad actually if you search for |
||
} | ||
|
||
// Verifies the beginning of the given UTF16 string and returns the first UTF16 sequence | ||
// or U+FFFD otherwise. It's not really useful and at the time of writing only a | ||
// single caller uses this. It's best to delete this if you read this comment. | ||
|
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.
We may want to call this
compatibility.integratedDrawingGlyphs
or something - IMO and of course open to discussion!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.
Alternatively we could call them "integrated pseudographics" or "builtin semigraphics". apparently those (pseudo/semigraphics) are names for these glyphs
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.
Feature label: Built-in Glyphs
Help text: "Use built-in glyphs for BoxDrawing, BlockElement and Powerline characters"
I think this fits well with the future plan where we would draw them even when the font itself may not come with any of those glyphs (Eg. unpatched fonts)
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 mostly worried about the JSON name - where there is no help text and "glyphs" is awfully broad (there are a looooottt of glyphs in a font!)
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 opposed to renaming this setting for 2 reasons:
custom_block_glyphs
terminal.integrated.customGlyphs
So while I agree that we should call it "integrated glyphs" I think consistency with other applications is more important to make it easier for users to find related settings in related applications.
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.
FWIW that's for "vscode's integrated terminal", not for "integrated custom glyphs"
That's fair, however.
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.
In case you're still considering other names, I think both Alacritty and Contour use something like
builtin_box_drawing
for this option (as a sub setting of thefont
configuration).