Skip to content

add font.style_set_names attribute to Python API #5354

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

Jump to bottom
Merged
skef merged 7 commits into from
Feb 1, 2024
Merged

add font.style_set_names attribute to Python API #5354

skef merged 7 commits into from
Feb 1, 2024

Conversation

@mf2vec-dev
Copy link
Contributor

@mf2vec-dev mf2vec-dev commented Jan 27, 2024

Closes #5263.

This PR adds the style_set_names attribute to the font type of the Python API to get and set the style set names (equivalent to Element -> Font Info... -> StyleSet Names in the GUI), including input validation, a Python test and documentation.

The last non-empty test case (name: "😀𝄞") passes in the Python interface. But when generating an sfd and loading it with the GUI, the name shown in the GUI is different (턞). I am not sure if this is a bug in my PR or an issue with SFDDumpUTF7Str/SFDReadUTF7Str or the GUI.
I would appreciate it if someone could double check this.

Type of change

  • New feature

dscorbett
dscorbett reviewed Jan 27, 2024
View reviewed changes
dscorbett
dscorbett reviewed Jan 27, 2024
View reviewed changes
@mf2vec-dev
Support lang ids in style_set_names' setter
c7202e7 
Also add test for language give as int,
use FindFlagByName instead of loop, check if font is closed,
improve error messages and fix small bug.
@iorsh
Copy link
Contributor

iorsh commented Jan 27, 2024 •
edited
Loading

I tried to run your branch, but could see no data in Font Info -> StyleSet Names at all. Saved SFD font also has no entries for StyleSets. What is your system?

EDIT: Sorry, missed something in your test script. It's ok now.

iorsh
iorsh reviewed Jan 27, 2024
View reviewed changes
@iorsh
Copy link
Contributor

iorsh commented Jan 27, 2024

The issue with "😀𝄞" is a bug in SFDDumpUTF7Str/SFDReadUTF7Str. You can observe it regardless of your PR, by setting any name (for example in "Font Info" --> "TTF Names") with characters in a supplementary plane.

@mf2vec-dev
Copy link
Contributor Author

mf2vec-dev commented Jan 28, 2024

The issue with "😀𝄞" is a bug in SFDDumpUTF7Str/SFDReadUTF7Str. You can observe it regardless of your PR, by setting any name (for example in "Font Info" --> "TTF Names") with characters in a supplementary plane.

@iorsh Correct, but I'm not 100% sure if it's SFDDumpUTF7Str/SFDReadUTF7Str or the GUI, because saving the font as sfd via Python and loading it with Python gives the correct language name "😀𝄞". So these Unicode characters seem to "survive" the sfd in this case.
Can you reproduce this?
(Perhaps a new issue would be a good place for this.)

BTW I'm using Ubuntu 22.04.3 LTS with Python 3.10.12.

@iorsh
Copy link
Contributor

iorsh commented Jan 28, 2024

@iorsh Correct, but I'm not 100% sure if it's SFDDumpUTF7Str/SFDReadUTF7Str or the GUI, because saving the font as sfd via Python and loading it with Python gives the correct language name "😀𝄞". So these Unicode characters seem to "survive" the sfd in this case. Can you reproduce this? (Perhaps a new issue would be a good place for this.)

I couldn't reproduce this. Could you please specify the exact course of action or script?

@mf2vec-dev
Copy link
Contributor Author

mf2vec-dev commented Jan 28, 2024

Could you please specify the exact course of action or script?

@iorsh This works for me:

import fontforge

style_set_names = (("English (British)", "ss20", "😀𝄞"),)
path = "/some/path/test.sfd" # change this

font = fontforge.font()
font.style_set_names = style_set_names
font.save(path)

font = fontforge.open(path)
assert font.style_set_names == style_set_names

The sfd file includes the line OtfFeatName: 'ss20' 2057 "+9gDRHgAA".

The GUI shows 턞 when you open it.

@iorsh
Copy link
Contributor

iorsh commented Jan 28, 2024

Ah, you mean the assertion, which passes and indicates correctly loaded string. That's interesting, I'll take a look.

@iorsh
Copy link
Contributor

iorsh commented Jan 28, 2024

If you add font.close() after saving, the assertion will fail. Probably fontforge.open() doesn't read file from disk if it's already open. So this is indeed a separate issue with SFDDumpUTF7Str/SFDReadUTF7Str.

Overall, I think we are good to go with this PR.

skef
skef reviewed Jan 28, 2024
View reviewed changes
skef
skef reviewed Jan 30, 2024
View reviewed changes
skef
skef approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@skef skef left a comment

Choose a reason for hiding this comment

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

LGTM

@skef
Copy link
Contributor

skef commented Feb 1, 2024

@iorsh do you have any further comment on the recent changes before I merge this?

@iorsh
Copy link
Contributor

iorsh commented Feb 1, 2024

@iorsh do you have any further comment on the recent changes before I merge this?

Looks good to me

@skef skef merged commit eff88fc into fontforge:master Feb 1, 2024
@delphinus delphinus mentioned this pull request Feb 1, 2024
Open
@mf2vec-dev mf2vec-dev deleted the py-font-style_set_names-attr branch February 1, 2024 17:05
@iorsh iorsh mentioned this pull request Feb 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iorsh iorsh iorsh left review comments

@skef skef skef approved these changes

+2 more reviewers

@dscorbett dscorbett dscorbett left review comments

@myoresh myoresh myoresh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

“StyleSet Names” cannot edit by Python API

5 participants

@mf2vec-dev @iorsh @skef @dscorbett @myoresh