Skip to content
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

Detect likely Powerline fonts, add a special powerline preview #15365

Merged
merged 4 commits into from Jun 9, 2023

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented May 16, 2023

When we detect a font that has a glyph for U+E0B6, we will switch the preview connection text to contain a special powerline prompt. This will allow people to see how different settings might impact their real-world environment.

When we don't detect such support, we fall back to the CMD-style C:\> prompt.

Pros:

  • It's beautiful.

Cons:

  • More code

Risks:

  • U+E0B6 is part of the private use area, and fonts that have symbols there (such as Cirth as sub-allocated by the ConScript Unicode Registry) will result in something unexpected.
  • Actually, E0B6 isn't part of base powerline... but I think this specific set of characters looks too good to pass up.

@github-actions

This comment has been minimized.

Base automatically changed from dev/duhowett/preview-text to main May 16, 2023 19:59
@DHowett DHowett force-pushed the dev/duhowett/preview-text-powerline branch from a5529a9 to 37a1f0f Compare May 17, 2023 19:49
@github-actions

This comment has been minimized.

Comment on lines +32 to +34
// We're actually checking for the "Extended" PowerLine glyph set.
// They're more fun.
THROW_IF_FAILED(font->HasCharacter(0xE0B6, &exists));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that you aren't really checking for PowerLine glyphs here. This is a Private Use Area, and this particular range has been used for Cirth characters in the ConScript Unicode Registry for the past couple of decades. Admittedly there probably aren't a lot of terminal users with Cirth fonts, but it would be nice if there was a safer way to detect PowerLine fonts specifically. Could you not guesstimate that from the name or something?

Copy link
Member Author

@DHowett DHowett May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you not guesstimate that from the name or something?

That was my first approach! Man, the code was a lot shorter then.

I profiled a bunch of fonts and found that there's:

  • fonts with "PL" in them
    • at the end
    • in the middle ("Xxx PL Mono")
      • ... which don't have PowerLine glyphs either ("IBM Plex Mono")
  • NerdFonts
    • with "NF" at the end or in the middle ("Hack NF")
    • with "Nerd Font" at the end or in the middle ("Xxx Nerd Font Propo")
    • with "NerdFont" in them somewhere
  • A hypothetical category of fonts that don't include a designator in the name, but do have somewhat-correct glyphs there

I know that it'll always be "best effort", and that powerline has coöpted part of the PUA that has already been used, but... it sucks all-up no matter how you slice it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly there probably aren't a lot of terminal users with Cirth fonts

image

Copy link
Collaborator

@j4james j4james May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that it wouldn't matter that much if you failed to detect some nerd fonts, because they'd just get the regular preview, which isn't the end of the world. Showing the nerd font preview to someone that's using another glyph set seems like a bigger deal.

That said, I've just been looking for a Cirth font to test with, and I couldn't find any monospace variants that could reasonably be used in a terminal (I'm sure I had one in the past, but I might be misremembering that). So feel free to leave it as is. I just wanted to make sure we at least considered other options here.

Edit: @zadjii-msft I just saw your Cirth screenshot now, but I'm assuming that's not a "standard" CSUR font - it's just remapping ASCII characters as Cirth glyphs. I suspect the monospace fonts I remember using in the past were probably doing the same thing.

Copy link
Member

@lhecker lhecker May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think the codepoint check is more robust, because that's how most terminals detect powerline glyphs anyways. VS Code for instance checks these codepoints to determine if it should increase the contrast of text or not (it won't change the colors for powerline glyphs). Basically, I believe that it's somewhat unlikely for a user to use a Cirth font and much more likely for them to use a powerline font.
Edit: And it will still work correctly for the real terminal after all. This only changes the behavior for the settings preview.

@DHowett DHowett force-pushed the dev/duhowett/preview-text-powerline branch from 37a1f0f to 1c89407 Compare May 30, 2023 13:32
@github-actions

This comment has been minimized.

@DHowett DHowett changed the title WIP: Detect Powerline fonts, and add a powerline preview? Detect likely Powerline fonts and add a special powerline preview May 30, 2023
@DHowett DHowett marked this pull request as ready for review May 30, 2023 14:22
@DHowett
Copy link
Member Author

DHowett commented May 30, 2023

Screenshots from the PR body

no powerline glyph

image

YES powerline glyph

image

when we "misfire" and detect symbols at E0B6 that are not powerline...

image

(it is not that bad)

@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Jun 2, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Jun 8, 2023
Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
@DHowett DHowett changed the title Detect likely Powerline fonts and add a special powerline preview Detect likely Powerline fonts, add a special powerline preview Jun 8, 2023
@DHowett DHowett enabled auto-merge (squash) June 8, 2023 23:36
@DHowett DHowett added this to To Cherry Pick in 1.18 Servicing Pipeline via automation Jun 8, 2023
@DHowett DHowett merged commit 37e8aff into main Jun 9, 2023
14 of 15 checks passed
@DHowett DHowett deleted the dev/duhowett/preview-text-powerline branch June 9, 2023 22:22
@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.18 Servicing Pipeline Jul 27, 2023
DHowett added a commit that referenced this pull request Jul 27, 2023
When we detect a font that has a glyph for `U+E0B6`, we will switch the
preview connection text to contain a special powerline prompt. This will
allow people to see how different settings might impact their real-world
environment.

When we _don't_ detect such support, we fall back to the CMD-style
`C:\>` prompt.

Pros:
- It's beautiful.

Cons:
- More code

Risks:
- `U+E0B6` is part of the private use area, and fonts that have symbols
there (such as Cirth as sub-allocated by the ConScript Unicode Registry)
will result in something unexpected.
- Actually, `E0B6` isn't part of base powerline... but I think this
specific set of characters looks too good to pass up.

(cherry picked from commit 37e8aff)
Service-Card-Id: 89468351
Service-Version: 1.18
@DHowett DHowett moved this from Cherry Picked to Validated in 1.18 Servicing Pipeline Sep 25, 2023
@DHowett DHowett moved this from Validated to Shipped in 1.18 Servicing Pipeline Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants