-
-
Notifications
You must be signed in to change notification settings - Fork 790
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
Finish up DEC Special Graphics character set #891
Conversation
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.
Thanks!
Would you mind adding a basic test for this?
I think you could get away with doing something like this:
https://github.com/wez/wezterm/blob/main/term/src/test/mod.rs#L632-L642
but printing the relevant sequences to get into the alt charset and print out the chars, and then assert that they show up as the intended unicode chars.
You can run the term
tests directly via cargo test -p wezterm-term
term/src/terminalstate.rs
Outdated
"`" => "◆", | ||
"a" => "▒", | ||
|
||
// AZL: I do not know why the next four Unicode glyphs are |
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.
Is that a local font problem? If I copy and paste the characters from your PR description into the terminal I see those glyphs in my terminal on this mac. If you do the same, do you see those glyphs on your system, or is the issue that the don't show up when used in the appropriate mode when rendered via the terminal?
wezterm will log information about glyphs that it failed to resolve. It can take several seconds to do that if you're in a debug build, because it has to parse and compute glyph coverage maps for a potentially large list of fallback fonts, and that's really slow in a debug build.
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 saw nothing logged to stderr on the glyphs. wezterm ls-fonts yields:
Primary font:
wezterm.font_with_fallback({
-- <built-in>, BuiltIn
"JetBrains Mono",
-- /usr/share/fonts/truetype/noto/NotoColorEmoji.ttf, FontConfig
"Noto Color Emoji",
-- <built-in>, BuiltIn
"Last Resort High-Efficiency",
})
The other fonts are all the same: JetBrains Mono, Noto, Last Resort.
When I paste " ␉ ␌ ␍ ␊ ␋" into cat I see these instead:
I've got the format fixed, and will add the test. (This is fun! :) )
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 think the glyphs you're seeing are coming from a fallback font; they seem like alternative pictorial versions of ␉ ␌ ␍ ␊ ␋
. I think I should add a thing to ls-fonts
to have it explain which fonts are used for a given text input to help understand cases like this!
I've got the format fixed, and will add the test. (This is fun! :) )
Great!
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.
wezterm ls-fonts --text "␉ ␌ ␍ ␊ ␋"
will explain which fonts were used now in main
OK, this update handles VT100 ASCII, UK, and DEC character sets. It passes the vttest VT100 character set test, and can support ncurses ACS chars. What do you think? |
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.
This is looking great thanks! I think the only thing outstanding is that there are couple of tests in the termwiz
crate that need adjusting to the EscCode
changes, but this otherwise looks ready to land.
Thanks!
/// Designate Character Set – DEC Line Drawing | ||
DecLineDrawing = esc!('(', '0'), | ||
/// Designate Character Set – US ASCII | ||
AsciiCharacterSet = esc!('(', 'B'), |
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.
There are a couple of tests in the termwiz crate that need to be adjusted for this change:
https://github.com/wez/wezterm/blob/main/termwiz/src/render/terminfo.rs#L891
You can run those tests using cargo test -p termwiz
(or just run all of the tests: cargo test
)
When I use Terminus font, the scan line glyphs (⎺⎻─⎼⎽) look good for one moment, and are then overwritten with wrongly-placed lines that look like (⎺⎻──⎼). I thought I saw some logic for wezterm to draw its own glyphs; is there a way to turn that feature off, so that I can ensure I am testing my font and not that drawing code? |
Hmm, that sounds like a fallback font is being loaded asynchronously. |
Hi! This is my first PR ever in Rust, so it likely needs work. But the point of it:
With these changes (and assuming you know how to fix the glyphs for ␉ ␌ ␍ ␊ ␋), this would finish out the screen features SAVE/RESTORE CURSOR screen, and get better on the VT100 character sets screen.