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

[designspaceLib] Multiple <mappings> elements in .designspace format, without support in Python API? #3466

Open
justvanrossum opened this issue Mar 26, 2024 · 8 comments
Assignees

Comments

@justvanrossum
Copy link
Collaborator

Re:
#3435
#3437

Do I understand correctly that multiple axis mappings groups are "flattened" into one group in the DesignSpaceDocument?

The "read" code seems to flatten all groups into one group:

for mappingsElement in self.root.findall(".axes/mappings"):
groupDescription = mappingsElement.attrib.get("description")
for mappingElement in mappingsElement.findall("mapping"):
description = mappingElement.attrib.get("description")
inputElement = mappingElement.find("input")
outputElement = mappingElement.find("output")
inputLoc = {}
outputLoc = {}
for dimElement in inputElement.findall(".dimension"):
name = dimElement.attrib["name"]
value = float(dimElement.attrib["xvalue"])
inputLoc[name] = value
for dimElement in outputElement.findall(".dimension"):
name = dimElement.attrib["name"]
value = float(dimElement.attrib["xvalue"])
outputLoc[name] = value
axisMappingObject = self.axisMappingDescriptorClass(
inputLocation=inputLoc,
outputLocation=outputLoc,
description=description,
groupDescription=groupDescription,
)
self.documentObject.axisMappings.append(axisMappingObject)

The "write" code only ever seems to output a single <mappings> element:

if self.documentObject.axisMappings:
mappingsElement = None
lastGroup = object()
for mappingObject in self.documentObject.axisMappings:
if getattr(mappingObject, "groupDescription", None) != lastGroup:
if mappingsElement is not None:
self.root.findall(".axes")[0].append(mappingsElement)
lastGroup = getattr(mappingObject, "groupDescription", None)
mappingsElement = ET.Element("mappings")
if lastGroup is not None:
mappingsElement.attrib["description"] = lastGroup
self._addAxisMapping(mappingsElement, mappingObject)
if mappingsElement is not None:
self.root.findall(".axes")[0].append(mappingsElement)

I don't think it is a good idea to add features to the format without having the reader/writer support them in a round-trippable way.

This is really problematic when the .designspace file isn't edited in a text editor, like Fontra does, or RoboFont's DesignSpace editor.

Maybe it has not been spelled out, but to me designspaceLib has always promised "readfile, write it back and get the same document" (ignoring comments or formatting differences).

We should either change the API to properly support multiple mappings groups, or allow only one <mappings> element.

@justvanrossum
Copy link
Collaborator Author

@behdad @Lorp

@behdad
Copy link
Member

behdad commented Mar 26, 2024

It should work just fine if napping have descriptions. The groupDescription takes care of that.

Is that still problematic in your opinion? I can synthesize group descriptions or something to round-trip.

@justvanrossum
Copy link
Collaborator Author

justvanrossum commented Mar 26, 2024

Ah, I see I misunderstood the code, thanks for pointing that out.

So the code does output multiple <mappings> elements, based on the groupDescription attribute of individual mapping objects.

I now understand how this works for round tripping. But only as long as the group description is unique, though.

I find this an unfortunate mismatch between format and API, and feels like a hack.

@behdad
Copy link
Member

behdad commented Mar 26, 2024

We can definitely change it. No one replies on it yet... What do you suggest?

@justvanrossum
Copy link
Collaborator Author

My current thought is: allowing multiple <mappings> elements overcomplicates things, and the original use case could be done without that change (or any change), by using a prefix naming convention on the <mapping> description attribute.

@behdad
Copy link
Member

behdad commented Mar 26, 2024

I'm find that. I'll come up with a PR.

@Lorp
Copy link

Lorp commented Mar 26, 2024

I’m fine going back to a single <mappings>. Hacking the description attribute feels odd though. Maybe class or name?

@behdad behdad self-assigned this Mar 28, 2024
@behdad
Copy link
Member

behdad commented Apr 19, 2024

I haven't forgotten this. I'll try to find the time to revert the change and just use description.

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

No branches or pull requests

3 participants