[cffLib/psCharStrings] remove cffCtx, pass down isCFF2 #1007
Conversation
* Removed `CFFContext` * Added `isCFF2` argument to CFFFontSet.decompile/compile, used from respective ttLib classes * Index classes get a `isCFF2` argument in constructor (used for decompiling); must be True/False if `file` argument is not None; it is stored as self._isCFF2 to support lazy loading * Removed `TopDictData` class; reuse same `TopDictIndexCompiler` for both CFF and CFF2 * `CFFWriter` and all `*Compiler` classes get an `isCFF2` argument; defaults to the parent compiler's `isCFF2` attribute * Removed `size` argument from `produceItem` method as unused and useless (`len(data)` is the same) * psCharStrings: removed useless ByteCodeBase class * A reference to the TopDict's VarStoreData is passed down to all the FontDicts' PrivateDict, so it can be used to get the number of regions while decompiling blend and vsindex operators See dicussion: fonttools#968 (comment)
|
I was concerned after a conversation with Behdad today that a vsindex defined in a PrivateDict would not be inherited as the default vsindex by the charstrings which reference the PrivateDict, but I see that is not the case. Looks good to me. |
|
Read, FWIW, one reason we are removing the context is because it didn't work as you thought it does. Since some structs are loaded lazily, and the decompilation depends on isCFF2 bit of the context, you cannot load CFF, change the major version, and save as CFF2. That will confuse the lazy decompiler to think it should load CFF2 objects instead of CFF! Hence why we removed the context and always pass down isCFF2 during compile and decompile, separately. |
| @@ -2310,6 +2304,7 @@ def decompileAllCharStrings(self, progress): | |||
| progress.increment(0) # update | |||
| i = i + 1 | |||
There was a problem hiding this comment.
Or better yet, replace L2296-2297 with,
for i, charString in enumerate(self.CharStrings.values()):and remove L2305.
There was a problem hiding this comment.
@miguelsousa I think i'll leave that old piece of code like that for now. I'm actually tempted to deprecate the progress bar interface altogether. BTW, do you know if anybody is using that? /cc @justvanrossum
There was a problem hiding this comment.
I am not, and I doubt anyone is. If there's a safe way to deprecate it then I'm fine with that.
There was a problem hiding this comment.
I'm not using it either
As discussed in #968 (comment) and in private conversation with @behdad, here is an (admittedly huge) patch that should break the current deadlock, and allow us to keep backward compatibility with the old cffLib interface (so fontmake or robofont don't break).
CFFContextisCFF2argument to CFFFontSet.decompile/compile, used from respective ttLib classesisCFF2argument in constructor (used for decompiling); must be True/False iffileargument is not None; it is stored as self._isCFF2 to support lazy loadingTopDictDataclass; reuse sameTopDictIndexCompilerfor both CFF and CFF2CFFWriterand all*Compilerclasses get anisCFF2argument; defaults to the parent compiler'sisCFF2attributesizeargument fromproduceItemmethod as unused and useless (len(data)is the same)I know I should have broken this into smaller patches, but... 😬
/cc @readroberts