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

Numeric fontWeight is rejected by the profile parser #7690

Closed
TBBle opened this issue Sep 21, 2020 · 5 comments
Closed

Numeric fontWeight is rejected by the profile parser #7690

TBBle opened this issue Sep 21, 2020 · 5 comments
Assignees
Labels
Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@TBBle
Copy link

TBBle commented Sep 21, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.19041.508]
Windows Terminal version (if applicable): Windows Terminal Preview Version: 1.3.2382.0

Any other software? Fira Code 5.2: https://github.com/tonsky/FiraCode/releases/tag/5.2

Steps to reproduce

(How I got there, starting from a working setup, is hidden inside the fold. It shouldn't affect the reproduction, but is here in case there is something relevant hidden there.)

Starting from a working default profile:

  "profiles": {
    "defaults": {
      // Put settings here that you want to apply to all profiles.
      "colorScheme": "Canon Solarized Dark",
      "fontFace": "Fira Code Retina",
      "fontSize": 14,
      "padding": "0, 0, 0, 0",
      "cursorShape": "filledBox"
    },
    "list": [
//...
    ]
  },

with the Fira Code TTFs (six files, one for each of Bold, Light, Mediuma, Regular, Retina, SemiBold) installed.

I then removed the Fira Code TTFs, and installed the Variable TTF (FiraCode-VF.ttf).

Samples in white-on-black are from the Windows "Font Properties" dialogs.

After this change, my terminal looked like this: image which is Consolas Regular at 14pt:
image, but with better anti-aliasing. (Or rather, with any anti-aliasing, I suspect)

This makes sense, since there isn't a "Fira Code Retina" font anymore on my machine, although some kind of notification that my font wasn't found would have been nice. Also, I would have expected a fallback to Cascadia Code, not Consolas. Cascadia Code is installed on my machine as part of the Windows Terminal package (twice, once for Preview, once for regular), so it seems like a better fallback?

I initially tried to move the 'Retina' from the fontFace to the fontWeight

  "profiles": {
    "defaults": {
      // Put settings here that you want to apply to all profiles.
      "colorScheme": "Canon Solarized Dark",
      "fontFace": "Fira Code",
      "fontSize": 14,
      "fontWeight": "Retina",
      "padding": "0, 0, 0, 0",
      "cursorShape": "filledBox"
    },
    "list": [
//...
    ]
  },

but that's not in the hard-coded list of named font-weights. I was disappointed that it wasn't just asking the chosen font if the fontWeight given matched to a font instance name, but not super-disappointed:

I saw in the profile schema and settings implementation that I could pass in a uint to fontWeight, if there was no existing hard-coded name for my desired weight.

I looked up the weight of the Retina instance in the Fira Code variable TTF (450), and got:

  "profiles": {
    "defaults": {
      // Put settings here that you want to apply to all profiles.
      "colorScheme": "Canon Solarized Dark",
      "fontFace": "Fira Code",
      "fontSize": 14,
      "fontWeight": "450",
      "padding": "0, 0, 0, 0",
      "cursorShape": "filledBox"
    },
    "list": [
//...
    ]
  },

Expected behavior

I get the same font I had when using Fira Code Retina as my fontFace and no fontWeight setting, with the Fira Code non-variable TTF files installed, i.e. 6 separate TTFs, one for each of Bold, Light, Mediuma, Regular, Retina, and SemiBold.

Actual behavior

Windows Terminal raises an error about the fontWeight value:

image

and then ignores the entire theme configuration, including my colour scheme setting.

So yeah, unlike above, I was super-disappointed this time. As a workaround, I'm using "fontWeight": "medium".


If I comment-out the fontWeight, it looks like this: image
which appears to be 'Regular' instance (weight 400): image rather than 'Retina' instance (weight 450): image which makes sense, if the default value for fontWeight is 'normal'.

Side-note: I was inspired to make this change by noticing that Cascadia Code recommends their Variable TTF over the fixed-size TTFs, and also #107 (comment)

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 21, 2020
@DHowett
Copy link
Member

DHowett commented Sep 21, 2020

So, the error message does a right poor job of surfacing this...

Can you try 450 without the quotes? 😁

@DHowett DHowett added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 21, 2020
@TBBle
Copy link
Author

TBBle commented Sep 21, 2020

Gah. >_<

Yeah, that works.

Thank you.

I'm going to go sit quietly away from the computer for a little while...


I assume the error message doesn't mention integers because the "Override mapping parser to add boolean parsing" (sic) code doesn't override TypeDescription, but just delegates it up to BaseEnumMapper. I expect it could provide its own TypeDescription, which would probably have left me still confused, but confused in the right direction.


I've just noticed that the hover-text in VS Code says:

Sets the weight (lightness or heaviness of the strokes) for the given font. Possible values:

-"thin"

-"extra-light"

-"light"

-"semi-light"

-"normal" (default)

-"medium"

-"semi-bold"

-"bold"

-"extra-bold"

-"black"

-"extra-black" or the corresponding numeric representation of OpenType font weight.

which suggests a couple of newlines are missing in the schema comment to take the last clause out of the list. Not that this would have clued me in to my mistake, if looking at profiles.schema.json and implementation didn't. Although given the current error message, the first thing I checked was "Is this code newer than the version I'm using?".

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 21, 2020
@DHowett
Copy link
Member

DHowett commented Sep 22, 2020

I should at least throw in a better type description. Thanks for calling that out 😄

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 22, 2020
@DHowett DHowett self-assigned this Sep 22, 2020
@Vanderdecken
Copy link

Vanderdecken commented Sep 23, 2020

I have a similar warning but with fontSize as of the latest update:
Microsoft Windows [Version 10.0.19041.508]

        {
            // Make changes here to the cmd.exe profile
            "fontFace": "Cascadia Code",
            "fontSize": "10",
            "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
            "name": "cmd",
            "commandline": "cmd.exe",
            "hidden": false,
            "startingDirectory": "."
        }

This config with the value 10 in quotes previously worked okay, but now produces:
image
on startup.

Removing the quotes fixes it:

        {
            // Make changes here to the cmd.exe profile
            "fontFace": "Cascadia Code",
            "fontSize": 10,
            "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
            "name": "cmd",
            "commandline": "cmd.exe",
            "hidden": false,
            "startingDirectory": "."
        }

Admittedly putting in a number as a string was dumb to start with (I'm sure I copied it from an example without thinking), but previously it was accepted without error.

@ghost ghost added In-PR This issue has a related PR Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 11, 2020
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8558, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

No branches or pull requests

3 participants