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

VarStore Format=1 is repeated in both attribute and element in ttx dump #3295

Open
anthrotype opened this issue Oct 10, 2023 · 1 comment
Open
Assignees

Comments

@anthrotype
Copy link
Member

I just noticed that when dumping an ItemVariationStore to ttx, the default Format=1 is written both in an attribute as well as as a sub-element:

<VarStore Format="1">
<Format value="1"/>

This is redundant, and I believe it's because in otData.py the VarStore is not defined as a format-switching table (there's in fact only one format right now). Changing it like that makes the redundant Format element go away, only the Format attribute stays:

diff --git a/Lib/fontTools/ttLib/tables/otData.py b/Lib/fontTools/ttLib/tables/otData.py
index 56716824e..1ae9a068e 100644
--- a/Lib/fontTools/ttLib/tables/otData.py
+++ b/Lib/fontTools/ttLib/tables/otData.py
@@ -3287,7 +3287,7 @@ otData = [
         ],
     ),
     (
-        "VarStore",
+        "VarStoreFormat1",
         [
             ("uint16", "Format", None, None, "Set to 1."),
             ("LOffset", "VarRegionList", None, None, ""),

But then a bunch of other tests fail when they import from existing XML files that do still contain a Format element, with KeyError like so:

    def getConverterByName(self, name):
>       return self.convertersByName[self.Format][name]
E       KeyError: 'Format'

Another way to fix it would be to specialise the toXML method of FormatSwitchingBaseTable, right now it uses BaseTable.toXML, which in turns has special handling of Format attribute (arguably that belongs to FormatSwitching variant only):

def toXML(self, xmlWriter, font, attrs=None, name=None):
tableName = name if name else self.__class__.__name__
if attrs is None:
attrs = []
if hasattr(self, "Format"):
attrs = attrs + [("Format", self.Format)]

Maybe there's no easy way to make this change backward compatible and we'll have to live with this duplication..

@behdad
Copy link
Member

behdad commented Oct 10, 2023

Ugh. Ugly. Let me think about it.

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

2 participants