From a7e4d86540eb009105593b2b9f5461f0cca0124f Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Thu, 7 Oct 2021 12:03:47 +0100 Subject: [PATCH 1/4] Use a set for file names for clash checking --- Lib/fontTools/ufoLib/filenames.py | 2 +- Lib/fontTools/ufoLib/glifLib.py | 21 ++++++++++++--------- Tests/ufoLib/glifLib_test.py | 2 ++ 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/Lib/fontTools/ufoLib/filenames.py b/Lib/fontTools/ufoLib/filenames.py index 2815469f36..49cf028224 100644 --- a/Lib/fontTools/ufoLib/filenames.py +++ b/Lib/fontTools/ufoLib/filenames.py @@ -15,7 +15,7 @@ class NameTranslationError(Exception): pass -def userNameToFileName(userName: str, existing=[], prefix="", suffix=""): +def userNameToFileName(userName: str, existing=set(), prefix="", suffix=""): """ existing should be a case-insensitive list of all existing file names. diff --git a/Lib/fontTools/ufoLib/glifLib.py b/Lib/fontTools/ufoLib/glifLib.py index 1437d8876b..f63ec74d8b 100755 --- a/Lib/fontTools/ufoLib/glifLib.py +++ b/Lib/fontTools/ufoLib/glifLib.py @@ -10,6 +10,8 @@ glyph data. See the class doc string for details. """ +from __future__ import annotations + import logging import enum from warnings import warn @@ -205,7 +207,7 @@ def __init__( self.glyphNameToFileName = glyphNameToFileNameFunc self._validateRead = validateRead self._validateWrite = validateWrite - self._existingFileNames = None + self._existingFileNames: set[str] | None = None self._reverseContents = None self.rebuildContents() @@ -455,12 +457,10 @@ def writeGlyph(self, glyphName, glyphObject=None, drawPointsFunc=None, formatVer fileName = self.contents.get(glyphName) if fileName is None: if self._existingFileNames is None: - self._existingFileNames = {} - for fileName in self.contents.values(): - self._existingFileNames[fileName] = fileName.lower() - fileName = self.glyphNameToFileName(glyphName, self._existingFileNames.values()) + self._existingFileNames = set(fileName.lower() for fileName in self.contents.values()) + fileName = self.glyphNameToFileName(glyphName, self._existingFileNames) self.contents[glyphName] = fileName - self._existingFileNames[fileName] = fileName.lower() + self._existingFileNames.add(fileName.lower()) if self._reverseContents is not None: self._reverseContents[fileName.lower()] = glyphName data = _writeGlyphToBytes( @@ -485,9 +485,9 @@ def deleteGlyph(self, glyphName): fileName = self.contents[glyphName] self.fs.remove(fileName) if self._existingFileNames is not None: - del self._existingFileNames[fileName] + self._existingFileNames.remove(fileName.lower()) if self._reverseContents is not None: - del self._reverseContents[self.contents[glyphName].lower()] + del self._reverseContents[fileName.lower()] del self.contents[glyphName] # dict-like support @@ -573,9 +573,12 @@ def __exit__(self, exc_type, exc_value, exc_tb): def glyphNameToFileName(glyphName, existingFileNames): """ Wrapper around the userNameToFileName function in filenames.py + + Note that existingFileNames should be a set for large glyphsets + or performance will suffer. """ if existingFileNames is None: - existingFileNames = [] + existingFileNames = set() return userNameToFileName(glyphName, existing=existingFileNames, suffix=".glif") # ----------------------- diff --git a/Tests/ufoLib/glifLib_test.py b/Tests/ufoLib/glifLib_test.py index 3af0256cdb..485c2bd947 100644 --- a/Tests/ufoLib/glifLib_test.py +++ b/Tests/ufoLib/glifLib_test.py @@ -159,6 +159,8 @@ def test_conflicting_case_insensitive_file_names(self, tmp_path): dst.writeGlyph("a", glyph) dst.writeGlyph("A", glyph) dst.writeGlyph("a_", glyph) + dst.deleteGlyph("a_") + dst.writeGlyph("a_", glyph) dst.writeGlyph("A_", glyph) dst.writeGlyph("i_j", glyph) From 827004608cfcddaf5ba7764fa9e3a81e7d5cecf9 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Thu, 7 Oct 2021 13:29:03 +0100 Subject: [PATCH 2/4] userNameToFileName should not have a mutable default parameter --- Lib/fontTools/ufoLib/filenames.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/fontTools/ufoLib/filenames.py b/Lib/fontTools/ufoLib/filenames.py index 49cf028224..8b9d76423e 100644 --- a/Lib/fontTools/ufoLib/filenames.py +++ b/Lib/fontTools/ufoLib/filenames.py @@ -15,9 +15,9 @@ class NameTranslationError(Exception): pass -def userNameToFileName(userName: str, existing=set(), prefix="", suffix=""): +def userNameToFileName(userName: str, existing=(), prefix="", suffix=""): """ - existing should be a case-insensitive list + existing should be a case-insensitive set of all existing file names. >>> userNameToFileName("a") == "a" From cd32e1d16b832b0397bfe5f0056119dd0fe67d1e Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Thu, 7 Oct 2021 13:30:11 +0100 Subject: [PATCH 3/4] Use set comprehension --- Lib/fontTools/ufoLib/glifLib.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/fontTools/ufoLib/glifLib.py b/Lib/fontTools/ufoLib/glifLib.py index f63ec74d8b..df80ed6d88 100755 --- a/Lib/fontTools/ufoLib/glifLib.py +++ b/Lib/fontTools/ufoLib/glifLib.py @@ -457,7 +457,9 @@ def writeGlyph(self, glyphName, glyphObject=None, drawPointsFunc=None, formatVer fileName = self.contents.get(glyphName) if fileName is None: if self._existingFileNames is None: - self._existingFileNames = set(fileName.lower() for fileName in self.contents.values()) + self._existingFileNames = { + fileName.lower() for fileName in self.contents.values() + } fileName = self.glyphNameToFileName(glyphName, self._existingFileNames) self.contents[glyphName] = fileName self._existingFileNames.add(fileName.lower()) From 04664089cc65a16fcfb52f21610a15ed384e9f7e Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Thu, 7 Oct 2021 14:44:20 +0100 Subject: [PATCH 4/4] Minor: wording --- Lib/fontTools/ufoLib/filenames.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/fontTools/ufoLib/filenames.py b/Lib/fontTools/ufoLib/filenames.py index 8b9d76423e..ec33b452ad 100644 --- a/Lib/fontTools/ufoLib/filenames.py +++ b/Lib/fontTools/ufoLib/filenames.py @@ -17,8 +17,7 @@ class NameTranslationError(Exception): def userNameToFileName(userName: str, existing=(), prefix="", suffix=""): """ - existing should be a case-insensitive set - of all existing file names. + `existing` should be a set-like object. >>> userNameToFileName("a") == "a" True