Repository: googlefonts/glyphsLib
Pull request #864 (2023-02-23 16:09:02)

Keep keys sorted alphabetically during serialization
A continuation of #862.


Patch:
diff --git a/Lib/glyphsLib/classes.py b/Lib/glyphsLib/classes.py
index 1b8b5e64..0bb99b0a 100755
--- a/Lib/glyphsLib/classes.py
+++ b/Lib/glyphsLib/classes.py
@@ -2559,15 +2559,17 @@ class segment(list):
 
 class GSComponent(GSBase):
     def _serialize_to_plist(self, writer):
+        # NOTE: The fields should come in alphabetical order.
         writer.writeObjectKeyValue(self, "alignment", "if_true")
         writer.writeObjectKeyValue(self, "anchor", "if_true")
+        if writer.format_version > 2:
+            writer.writeObjectKeyValue(self, "rotation", keyName="angle", default=0)
         writer.writeObjectKeyValue(self, "locked", "if_true")
         if writer.format_version == 2:
             writer.writeObjectKeyValue(self, "name")
         if self.smartComponentValues:
             writer.writeKeyValue("piece", self.smartComponentValues)
         if writer.format_version > 2:
-            writer.writeObjectKeyValue(self, "rotation", keyName="angle", default=0)
             writer.writeObjectKeyValue(
                 self, "position", keyName="pos", default=Point(0, 0)
             )
@@ -2806,16 +2808,13 @@ GSAnchor._add_parsers(
 
 class GSHint(GSBase):
     def _serialize_to_plist(self, writer):
-        if writer.format_version > 2:
-            writer.writeObjectKeyValue(self, "name", condition="if_true")
-        for field in ["horizontal", "options"]:
-            writer.writeObjectKeyValue(self, field, condition="if_true")
-        for field in ["origin", "place"]:
-            writer.writeObjectKeyValue(self, field)
-        if writer.format_version > 2:
-            writer.writeObjectKeyValue(self, "stem", self.stem != -2)
-        for field in ["other1", "other2", "scale"]:
+        # NOTE: The fields should come in alphabetical order.
+        for field in ["horizontal", "name", "options"]:
+            writer.writeObjectKeyValue(self, field, "if_true")
+        for field in ["origin", "other1", "other2", "place", "scale"]:
             writer.writeObjectKeyValue(self, field)
+        writer.writeObjectKeyValue(self, "settings", "if_true")
+        writer.writeObjectKeyValue(self, "stem", self.stem != -2)
         if writer.format_version == 2:
             writer.writeObjectKeyValue(self, "target")
         elif self.target:
@@ -2825,11 +2824,7 @@ class GSHint(GSBase):
             else:
                 writer.writeValue(self.target)
             writer.file.write(";\n")
-        for field in ["settings", "type"]:
-            writer.writeObjectKeyValue(self, field, condition="if_true")
-        if writer.format_version == 2:
-            writer.writeObjectKeyValue(self, "stem", self.stem != -2)
-            writer.writeObjectKeyValue(self, "name", condition="if_true")
+        writer.writeObjectKeyValue(self, "type", "if_true")
 
     _defaultsForName = {
         # TODO: (jany) check defaults in glyphs
@@ -3010,6 +3005,7 @@ GSHint._add_parsers(
 
 class GSFeature(GSBase):
     def _serialize_to_plist(self, writer):
+        # NOTE: The fields should come in alphabetical order.
         writer.writeObjectKeyValue(self, "automatic", "if_true")
         writer.writeObjectKeyValue(self, "code", True)
         writer.writeObjectKeyValue(self, "disabled", "if_true")
@@ -3065,6 +3061,7 @@ GSFeature._add_parsers(
 
 class GSClass(GSFeature):
     def _serialize_to_plist(self, writer):
+        # NOTE: The fields should come in alphabetical order.
         writer.writeObjectKeyValue(self, "automatic", "if_true")
         writer.writeObjectKeyValue(self, "code", True)
         writer.writeObjectKeyValue(self, "disabled", "if_true")
@@ -3561,6 +3558,7 @@ class LayerComponentsProxy(LayerShapesProxy):
 
 class GSLayer(GSBase):
     def _serialize_to_plist(self, writer):
+        # NOTE: The fields should come in alphabetical order.
         writer.writeObjectKeyValue(self, "anchors", "if_true")
         writer.writeObjectKeyValue(self, "annotations", "if_true")
         if self.layerId != self.associatedMasterId:
@@ -3580,6 +3578,7 @@ class GSLayer(GSBase):
         writer.writeObjectKeyValue(self, "layerId", "if_true")
         if writer.format_version == 2:
             writer.writeObjectKeyValue(self, "metricLeft", keyName="leftMetricsKey")
+            # NOTE: The following two are an exception from the ordering rule.
             writer.writeObjectKeyValue(self, "metricRight", keyName="rightMetricsKey")
             writer.writeObjectKeyValue(self, "metricWidth", keyName="widthMetricsKey")
         else:


Test patch:
diff --git a/tests/data/GlyphsFileFormatv2.glyphs b/tests/data/GlyphsFileFormatv2.glyphs
new file mode 100644
index 00000000..a1702590
--- /dev/null
+++ b/tests/data/GlyphsFileFormatv2.glyphs
@@ -0,0 +1,348 @@
+{
+.appVersion = "1350";
+DisplayStrings = (
+A,
+AB,
+D,
+"/_part.test"
+);
+classes = (
+{
+code = "A B C D E F G H I J K L M N O P Q R S T U V W X Y Z\012";
+name = Uppercase;
+}
+);
+copyright = "Default Copyright1";
+customParameters = (
+{
+name = "Import Font";
+value = {
+};
+},
+{
+name = localizedCopyright;
+value = "German;Deutsches Copyright";
+},
+{
+name = trademark;
+value = "Default Trademark";
+},
+{
+name = description;
+value = "Default Description";
+},
+{
+name = Axes;
+value = (
+{
+Hidden = 1;
+Name = Weight;
+Tag = wght;
+}
+);
+}
+);
+date = "2020-01-03 19:42:13 +0000";
+designerURL = "Designer URL";
+disablesNiceNames = 1;
+familyName = "New Font";
+featurePrefixes = (
+{
+automatic = 1;
+code = "languagesystem DFLT dflt;\012";
+name = Languagesystems;
+}
+);
+features = (
+{
+code = "sub C by D;\012";
+disabled = 1;
+name = test;
+},
+{
+code = "sub A by B;\012";
+name = ss01;
+notes = "featureNames {\012	name 3 1 0x409 \"Stylistic Set Name 1\";\012	name 3 1 0x407 \"Formatsatzname 1\";\012};\012";
+}
+);
+fontMaster = (
+{
+alignmentZones = (
+"{800, 15}",
+"{700, 15}",
+"{550, 15}",
+"{500, 15}",
+"{0, -15}",
+"{-200, -15}"
+);
+ascender = 800;
+capHeight = 700;
+customParameters = (
+{
+name = "Default Layer Width";
+value = "600";
+},
+{
+name = smallCapHeight;
+value = 550;
+}
+);
+descender = -200;
+guideLines = (
+{
+angle = 17.1044;
+locked = 1;
+position = "{121, 148}";
+showMeasurement = 1;
+}
+);
+horizontalStems = (
+321
+);
+id = m01;
+verticalStems = (
+123
+);
+visible = 1;
+weightValue = 0;
+xHeight = 500;
+}
+);
+glyphs = (
+{
+glyphname = A;
+lastChange = "2020-10-28 19:17:01 +0000";
+layers = (
+{
+guideLines = (
+{
+angle = 12.9339;
+locked = 1;
+position = "{348, 402}";
+showMeasurement = 1;
+}
+);
+hints = (
+{
+horizontal = 1;
+origin = "{0, 0}";
+target = "{0, 3}";
+type = Stem;
+}
+);
+layerId = m01;
+rightMetricsKey = "=20";
+paths = (
+{
+closed = 1;
+nodes = (
+"10 66 LINE",
+"439 66 LINE",
+"439 608 LINE",
+"10 608 LINE {name = \"Hallo\011Welt\";\ntest = \"Hallo\012Welt\";}"
+);
+}
+);
+width = 459;
+},
+{
+associatedMasterId = m01;
+layerId = "B53B276E-7ED6-4F56-94FF-4162BC3B585A";
+name = Color;
+width = 600;
+}
+);
+leftKerningGroup = A;
+leftMetricsKey = "=10";
+rightKerningGroup = A;
+topKerningGroup = A;
+bottomKerningGroup = A;
+unicode = "0041,0061";
+},
+{
+glyphname = B;
+lastChange = "2020-01-04 22:33:44 +0000";
+layers = (
+{
+components = (
+{
+name = A;
+transform = "{0.75175, 0.27362, -0.27362, 0.75175, 0, 0}";
+}
+);
+layerId = m01;
+width = 480;
+}
+);
+unicode = "0042,0062";
+},
+{
+glyphname = C;
+lastChange = "2020-01-04 19:35:27 +0000";
+layers = (
+{
+backgroundImage = {
+crop = "{{0, 0}, {140, 160}}";
+imagePath = Screenshot.png;
+locked = "1";
+transform = "{0.89877, 0.04712, -0.04188, 0.7989, 106, 89}";
+};
+layerId = m01;
+width = 600;
+}
+);
+unicode = 0043;
+},
+{
+glyphname = D;
+lastChange = "2020-01-04 23:28:31 +0000";
+layers = (
+{
+components = (
+{
+name = _part.test;
+piece = {
+Width = 29.74825;
+};
+}
+);
+layerId = m01;
+width = 100;
+}
+);
+unicode = 0044;
+},
+{
+glyphname = one;
+lastChange = "2020-01-04 16:36:08 +0000";
+layers = (
+{
+layerId = m01;
+width = 600;
+}
+);
+unicode = 0031;
+userData = {
+case = upper;
+};
+},
+{
+glyphname = space;
+layers = (
+{
+layerId = m01;
+width = 200;
+}
+);
+unicode = 0020;
+},
+{
+glyphname = smily;
+production = someSmily;
+lastChange = "2020-12-17 12:02:33 +0000";
+layers = (
+{
+layerId = m01;
+width = 600;
+}
+);
+script = "Black Letter";
+category = Symbol;
+subCategory = Emoji;
+},
+{
+export = 0;
+glyphname = _part.test;
+lastChange = "2020-01-04 23:28:09 +0000";
+layers = (
+{
+layerId = m01;
+paths = (
+{
+closed = 1;
+nodes = (
+"5 0 LINE",
+"138 0 LINE",
+"38 700 LINE",
+"5 700 LINE"
+);
+}
+);
+userData = {
+PartSelection = {
+Width = 1;
+};
+};
+width = 100;
+},
+{
+associatedMasterId = m01;
+layerId = "BEE6AB80-1627-43EB-904A-37A721971B03";
+name = Wide;
+paths = (
+{
+closed = 1;
+nodes = (
+"5 0 LINE",
+"988 0 LINE",
+"38 700 LINE",
+"5 700 LINE"
+);
+}
+);
+userData = {
+PartSelection = {
+Width = 2;
+};
+};
+width = 1000;
+}
+);
+partsSettings = (
+{
+name = Width;
+bottomName = "";
+bottomValue = 0;
+topName = "";
+topValue = 100;
+}
+);
+}
+);
+instances = (
+{
+customParameters = (
+{
+name = familyName;
+value = "Instance Family Name";
+}
+);
+interpolationWeight = 123;
+instanceInterpolations = {
+m01 = 1;
+};
+isBold = 1;
+isItalic = 1;
+name = Regular;
+}
+);
+kerning = {
+m01 = {
+A = {
+B = 30;
+};
+};
+};
+vertKerning = {
+m01 = {
+A = {
+A = -30;
+};
+};
+};
+manufacturer = Manufacturer;
+manufacturerURL = "Manufacturer URL";
+unitsPerEm = 1000;
+versionMajor = 1;
+versionMinor = 0;
+}
diff --git a/tests/writer_test.py b/tests/writer_test.py
index 2b98a5e2..dde043c8 100644
--- a/tests/writer_test.py
+++ b/tests/writer_test.py
@@ -13,11 +13,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
+import shutil
+import tempfile
 import unittest
+from collections import OrderedDict
 from io import StringIO
 from textwrap import dedent
-from collections import OrderedDict
-import os
 
 from glyphsLib import classes
 from glyphsLib.types import parse_datetime, Point, Rect
@@ -25,8 +27,16 @@ from glyphsLib.writer import dump, dumps
 
 from . import test_helpers
 
+DATA = os.path.join(os.path.dirname(__file__), "data")
+
 
 class WriterTest(unittest.TestCase, test_helpers.AssertLinesEqual):
+    def setUp(self):
+        self.temporary_path = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.temporary_path)
+
     def assertWrites(self, glyphs_object, text, format_version=2):
         """Assert that the given object, when given to the writer,
         produces the given text.
@@ -1120,13 +1130,13 @@ rememberToDownloadARealRemindersApp = 1;}"',
                 """\
             {
             horizontal = 1;
+            name = "My favourite hint";
             options = 128;
             origin = "{0, 0}";
             other1 = "{0, 2}";
             other2 = "{1, 0}";
             target = "{0, 1}";
             type = 16;
-            name = "My favourite hint";
             }
         """
             ),
@@ -1176,8 +1186,8 @@ rememberToDownloadARealRemindersApp = 1;}"',
             dedent(
                 """\
              {
-             name = "My favourite hint";
              horizontal = 1;
+             name = "My favourite hint";
              options = 128;
              origin = (0,0);
              other1 = (0,2);
@@ -1227,6 +1237,69 @@ rememberToDownloadARealRemindersApp = 1;}"',
             ),
         )
 
+    def test_write_official_reference_v2(self):
+        name = "GlyphsFileFormatv2.glyphs"
+
+        path = os.path.join(DATA, name)
+        font = classes.GSFont(path)
+        with open(path, encoding="utf=8") as file:
+            expected = file.read()
+
+        path = os.path.join(self.temporary_path, name)
+        font.save(path)
+        with open(path, encoding="utf=8") as file:
+            actual = file.read()
+
+        exceptions = [
+            # The line is removed.
+            "visible = 1;",
+            # The tab is changed.
+            r'"10 608 LINE {name = \"Hallo\011Welt\";\ntest = \"Hallo\012Welt\";}"',
+            r'"10 608 LINE {name = \"Hallo	Welt\";\ntest = \"Hallo\012Welt\";}"',
+            # The quotes are removed.
+            'locked = "1";',
+            "locked = 1;",
+            # The line is removed.
+            'bottomName = "";',
+            # The line is removed.
+            'topName = "";',
+            # The block is removed.
+            "vertKerning = {",
+            "m01 = {",
+            "A = {",
+            "A = -30;",
+            "};",
+            "};",
+            "};",
+        ]
+        expected = [
+            line
+            for line in expected.splitlines()
+            if not any(line == exception for exception in exceptions)
+        ]
+        actual = [
+            line
+            for line in actual.splitlines()
+            if not any(line == exception for exception in exceptions)
+        ]
+
+        assert actual == expected
+
+    def test_write_official_reference_v3(self):
+        name = "GlyphsFileFormatv3.glyphs"
+
+        path = os.path.join(DATA, name)
+        font = classes.GSFont(path)
+        with open(path, encoding="utf=8") as file:
+            expected = file.read()
+
+        path = os.path.join(self.temporary_path, name)
+        font.save(path)
+        with open(path, encoding="utf=8") as file:
+            actual = file.read()
+
+        assert actual.splitlines() == expected.splitlines()
+
 
 class WriterDumpInterfaceTest(unittest.TestCase):
     def test_dump(self):