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

Fix NF Mono glyph selection, NF variants STAT, PL rclt table #721

Merged
merged 6 commits into from
Apr 9, 2024

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Apr 9, 2024

The build issue I was running into was caused by the logic that chooses when to merge Powerline glyphs into the final set before building.

It covered the following conditions:

Conditions Outcome
Mono Set name only ✅
Mono + PL Set name, merge PL ✅
PL Set name, merge PL ✅
NF Set name, merge PL, merge NF ✅

However, it missed one:

Conditions Outcome
Mono + NF ❔❔❔

I've simplified the logic so that we don't make this mistake again. All non-Cascadia-Code variants will have their names overridden, and if they contain PL or NF they will get special treatment.

I've also added the NF variants to the STAT table table and fixed the generation of rclt for PL variants.

@@ -111,7 +111,7 @@ def step_set_feature_file(path: Path, name: str, instance: ufoLib2.Font) -> None
for item in featureList:
if "PL" in name and item == "rclt":
featureSet += Path(path / str("rclt_PL.fea")).read_text()
if "NF" in name and item == "rclt":
elif "NF" in name and item == "rclt":
Copy link
Member Author

Choose a reason for hiding this comment

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

This may have caused issues with the later else, resulting in the inclusion of rclt.fea after we'd already included rclt_PL.fea in the PL variant.

@DHowett
Copy link
Member Author

DHowett commented Apr 9, 2024

I also figured out why the stypo variants were being ignored in the PL/NF builds; /cc @PhMajerus for the last commit

@DHowett DHowett requested a review from aaronbell April 9, 2024 20:23
@DHowett DHowett changed the title Fix glyph selection for NF Mono, add NF variants to STAT Fix NF Mono glyph selection, NF variants STAT, PL rclt table Apr 9, 2024
Copy link
Collaborator

@aaronbell aaronbell left a comment

Choose a reason for hiding this comment

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

SGTM

@aaronbell
Copy link
Collaborator

(Thanks so much for your investigation!!)

@DHowett
Copy link
Member Author

DHowett commented Apr 9, 2024

Hey, I love working on this stuff :D

@DHowett DHowett merged commit d535176 into main Apr 9, 2024
1 check passed
@DHowett DHowett deleted the dev/duhowett/fix-stuff branch April 9, 2024 21:01
@PhMajerus
Copy link
Contributor

Let me know if there is anything I need to do on my side when adding glyphs with stypo variants.
For now I've included the substitution of "normal" to "stypo" versions both in features.fea::rclt of non-italic fonts, and in the global features/rclt.fea::rclt. Is there any other place I need to include the stypo substitutions?

@DHowett
Copy link
Member Author

DHowett commented Apr 10, 2024

Just make sure it ends up in rclt_PL.fea as well! Otherwise the powerline/nerdfont builds won’t get the optional stypo metrics 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants