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

Process 'copyGroups' attribute #20

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Process 'copyGroups' attribute #20

merged 1 commit into from
Mar 6, 2019

Conversation

miguelsousa
Copy link
Collaborator

@miguelsousa
Copy link
Collaborator Author

It just occurred to me that this process of copying the groups should probably take kerningGroupConversionRenameMaps into account. Otherwise the resulting instance may end up with duplicate kerning groups, e.g. @MMK_L_LAT_A and public.kern1.LAT_A in the same groups.plist file.

@LettError WDYT?

@miguelsousa miguelsousa deleted the copyGroups-fix branch March 6, 2019 09:02
@LettError
Copy link
Owner

Err, are you using UFO2? Do you have a small test maybe?

@typemytype
Copy link
Collaborator

in a UFO3 world @MMK_L_LAT_A is not a valid kerning group, it will be converted with a kerning prefix, and the old group name is removed.

see https://github.com/fonttools/fonttools/blob/master/Lib/fontTools/ufoLib/converters.py#L27-L32

@miguelsousa
Copy link
Collaborator Author

@typemytype I understand that. The case I'm thinking about is:

  • masters have kerning and are UFO2
  • instance has kerning and is UFO3

If the copyGroups step blindly copies all the groups from the default master into the instance, the latter will have duplicate data, I think.

@LettError working on crafting a test

@typemytype
Copy link
Collaborator

internally all current tools always up-convert to UFO3 inside

if a script/user wants to save as UFO2, which that is possible, then in your example the instance will have kerning group with UFO3 style group naming, that is not bad.

The only reason why a rename back to MM names is nice, is if you want to edit kerning of that UFO file in MM afterwards...

@miguelsousa
Copy link
Collaborator Author

I'm fine with ending up with a UFO2 that has public.kern prefixes. What I'm trying to avoid is a UFO3 that contains both sets (MM and public.kern). Judging from the tests I've ran so far, the current code saves UFO3 instances with both sets.

miguelsousa added a commit that referenced this pull request Mar 7, 2019
miguelsousa added a commit that referenced this pull request Mar 7, 2019
@frankrolf
Copy link

This issue comes from my workshop files, which were translated from UFO2 to UFO3 in Robofont. It’s possible that old-school kerning groups did stick around in them.

If I can help with any kind of test scenario I’d be glad to – please let me know which kinds of files would be needed.

@LettError
Copy link
Owner

Can you send me those files, and a designspace maybe?

@miguelsousa
Copy link
Collaborator Author

@frankrolf
Copy link

frankrolf commented Mar 7, 2019

Attached here. The input masters are UFO3 files, with kern.X groups in them. Additionally, they have a non-kerning related COMBINING_MARKS group, which goes missing in the instances. The COMBINING_MARKS group is the same across masters.
The loss of this group is independent of a <groups copy="1" /> flag in the designspace file.

Masters.zip

@LettError
Copy link
Owner

The groups are copied from the default when kerning is calculated. So the copyGroups flag is not really flagging very much any more.

  • Is there a use case in which the kerning + groups come from the default, but non-kerning groups come from elsewhere?

Otherwise it might be easier to just accept all groups from the default.

@miguelsousa
Copy link
Collaborator Author

With the latest changes I think things are working as expected.
@frankrolf can you test with afdko 2.8.7?

This pull request was closed.
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.

4 participants