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.

Sign up for GitHub

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 '--expand-features-to-instances' arg to fontmake UFO generation #985

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add '--expand-features-to-instances' arg to fontmake UFO generation #985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c98f78
Add '--expand-features-to-instances' arg to fontmake UFO generation
arrowtype Jun 11, 2024
b56edad
remove --expand-features-to-instances from static build args, as this…
arrowtype Oct 3, 2024
532a8b2
Pass bool of expandFeaturesToInstances from config to instantiateUfo …
arrowtype Oct 3, 2024
cc8617e
format document
arrowtype Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Lib/gftools/builder/operations/instantiateUfo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ def targets(self):
def variables(self):
vars = super().variables
vars["args"] += " --ufo-structure=json "
if self.original["expandFeaturesToInstances"]:
Copy link
Collaborator

@m4rc1e m4rc1e Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Any reason why you're using self.original here? It is breaking the tests since expandFeaturesToInstances isn't in self.original. If I change it to vars it seems to be working but I could be wrong.

Personally, I would use a dict's get method if I cannot guarantee that a key exists. It also allows you to set a value incase it doesn't exist so in your case self.original.get("expandFeaturesToInstances", False) will return False if the key doesn't exist, or it will simply return the value if it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First, thank you for that tip on .get() – that is news to me, but really helpful to learn!

Any reason why you're using self.original here?

As far as I can remember, I think I just tried to use a similar format as what is a few lines below:

gftools/Lib/gftools/builder/operations/instantiateUfo.py

Line 69 in cc8617e

if self.original.get("glyphData") is not None:

...and when it seemed to work, I thought I had solved it.

My impression was that self.original is the GOOGLEFONTS_SCHEMA, but I now realize that, even if that is true, there are other possible schemas.

vars["args"] += " --expand-features-to-instances "
if self.first_source.is_glyphs:
vars["args"] += f"--instance-dir {escape_path(str(self.instance_dir))}"
else:
Expand Down
6 changes: 4 additions & 2 deletions Lib/gftools/builder/recipeproviders/googlefonts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"checkCompatibility": True,
"overlaps": "booleanOperations",
"splitItalic": True,
"expandFeaturesToInstances": True,
}


Expand Down Expand Up @@ -193,8 +194,6 @@ def fontmake_args(self, source, variable=False):
args += " --keep-direction"
if self.config.get("removeOutlineOverlaps") is False:
args += " --keep-overlaps"
if self.config.get("expandFeaturesToInstances"):
args += " --expand-features-to-instances"
Comment on lines -196 to -197
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we 100% sure that this doesn't work? If I was implementing such a feature then I'd simply chuck it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the approach in the release as of when I made the PR, right? If so, then yes, I’m 100% sure it wasn’t working. I can test it again, though, if it seems likely that it may have changed since then.

Copy link
Contributor

Choose a reason for hiding this comment

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

That line feels right - I think I'd like to understand why it's not working before trying something else, because understanding why not might lead us to the right answer.

if self.config.get("extraFontmakeArgs") is not None:
args += " " + str(self.config["extraFontmakeArgs"])
if variable:
Expand Down Expand Up @@ -340,6 +339,9 @@ def build_a_static(self, source: File, instance: InstanceDescriptor, output):
"operation": "instantiateUfo",
"instance_name": instancename,
"glyphData": self.config.get("glyphData"),
"expandFeaturesToInstances": self.config.get(
"expandFeaturesToInstances"
),
}
)
steps += (
Expand Down
Loading