silnrsi / pysilfont

Python-based font utilities collection and framework
Other
61 stars 16 forks source link

Do not perform normalization when not asked to, warn only #96

Closed rimas-kudelis closed 2 weeks ago

rimas-kudelis commented 2 weeks ago

I'm trying to unify the set of glyphs in 8 instances of the same font, and for that, I need to copy several glyphs from one instance to a few others, as well as rename a set of glyphs.

So, I prepared a csv file and copied the missing glyphs, then I ran psfrenameglyphs in a loop for each UFO, and here's the (example) stuff I'm getting in the logs:

2024-09-30 09:55:26 Progress: Running:  /home/rq/.local/bin/psfrenameglyphs -i ../rename.csv Jonova-Bold.ufo
2024-09-30 09:55:26 Progress: Opening file for input: ../rename.csv
2024-09-30 09:55:26 Progress: Reading UFO: Jonova-Bold.ufo
2024-09-30 09:55:26 Progress: Checking fontinfo.plist metadata
2024-09-30 09:55:26 Warning:  openTypeNameUniqueID would be updated  Old value: Fotonija:Jonova Bold..., new value: UAB Fotonija: Jonova...
2024-09-30 09:55:26 Progress: Checking lib.plist metadata
2024-09-30 09:55:26 Warning:  com.fontlab.v2.tth would be removed from lib.plist  Old value: {'codeppm': 0, 'stem...
2024-09-30 09:55:26 Progress: Check & fix results:- Errors: 0, Changes to make: 2, Other warnings: 0
2024-09-30 09:55:26 Progress: See log file for details
2024-09-30 09:55:26 Progress: 1 glyphs renamed in UFO
2024-09-30 09:55:26 Progress: Backing up input font to backups/Jonova-Bold.ufo.2~
2024-09-30 09:55:27 Progress: Processing font for output
2024-09-30 09:55:27 Progress: Writing font to Jonova-Bold.ufo
2024-09-30 09:55:27 Warning:  Skipping invalid file uniE_F_E_5.glif from input UFO
2024-09-30 09:55:27 Warning:  Deleting uniE_F_E_5.glif from existing output UFO
2024-09-30 09:55:27 Warning:  Skipping invalid file uniE_F_E_6.glif from input UFO
2024-09-30 09:55:27 Warning:  Deleting uniE_F_E_6.glif from existing output UFO
2024-09-30 09:55:27 Warning:  Skipping invalid file uniE_F_E_7.glif from input UFO
2024-09-30 09:55:27 Warning:  Deleting uniE_F_E_7.glif from existing output UFO
2024-09-30 09:55:27 Warning:  Skipping invalid file uniE_F_E_8.glif from input UFO
2024-09-30 09:55:27 Warning:  Deleting uniE_F_E_8.glif from existing output UFO
2024-09-30 09:55:27 Warning:  Skipping invalid file uniE_F_E_9.glif from input UFO
2024-09-30 09:55:27 Warning:  Deleting uniE_F_E_9.glif from existing output UFO
2024-09-30 09:55:27 Progress: Command completed with 0 errors and 12 warnings
2024-09-30 09:55:27 Progress: See log file for warning messages or rerun with '-p scrlevel=w'

Well, thanks, but really, the fact that psfrenameglyphs – a script whose job is to rename glyphs only – has DELETED several glyphs without even asking me whether it should (as opposed to simply warning that they would have been deleted like in case of unwanted/disliked lib.plist and fontinfo.plist entries) is really unsettling.

Luckily, at least a backup was made, but still, now it's extra work for me to restore these glyphs from backups, for no good reason.

jvgaultney commented 2 weeks ago

How did you copy over the glyphs? Manually or using psfcopyglyphs? If manually did you also update contents.plist? I don't know what's causing your new glyphs to be considered invalid, but having them in your UFO makes the UFO invalid. You don't want them there.

Most of our tools will produce a normalized UFO - that's by design and cannot be turned off. Part of that is not writing out glyphs that are considered invalid. The problem is not that pysilfont is deleting glyphs. That should happen. The problem is that you have glyphs that are (for some reason) not properly formed or integrated into the UFO.

It should be trivial to retrieve and rename the backup UFO. You don't need to copy glyphs manually. Delete the normalized UFO, copy backup UFO, rename it. Then fix the glyphs that are invalid.

rimas-kudelis commented 2 weeks ago

I copied the glyphs manually, and hadn't yet updated contents.plist by then. I've already re-copied these glyphs and registered them normally this time, so all is good in that regard.

Like I said, if this was a normalization that I explicitly ran (via psfnormalize), that would make sense. But when a tool that is supposed to do something entirely different is deleting files that have no relation to its current task, I don't think this is good behavior.

jvgaultney commented 2 weeks ago

I can understand your argument, but basic normalization is intentionally built into our library so that whenever possible you end up with a normalised, valid UFO. We're not interested in changing this behavior.

rimas-kudelis commented 2 weeks ago

Fair enough, but could you elaborate about what criteria is used when deciding whether to just warn, like in case of the following two lines, or actually apply the fix:

2024-09-30 09:55:26 Warning:  openTypeNameUniqueID would be updated  Old value: Fotonija:Jonova Bold..., new value: UAB Fotonija: Jonova...
2024-09-30 09:55:26 Warning:  com.fontlab.v2.tth would be removed from lib.plist  Old value: {'codeppm': 0, 'stem...

Or is it because the unregistered glyphs somehow made the font absolutely invalid?

jvgaultney commented 2 weeks ago

I'll admit that our reporting could be improved. Normalization does not run checkfix automatically. It's telling you that if you run checkfix it can make those changes.