robotools / fontParts

The replacement for RoboFab
MIT License
135 stars 44 forks source link

anchor and guideline may not have specific keys in dict #678

Open LettError opened 1 year ago

LettError commented 1 year ago

After some mathGlyphing around in RF Version 4.4b (build 2212022233) I end up here as part of a fromMathGlyph(): https://github.com/robotools/fontParts/blob/f5250dc93916f45d5bf3e286cf44ff4c7dad9965/Lib/fontParts/base/glyph.py#L1697

Traceback (most recent call last):
  File "testRefactor_RF.py", line 64, in <module>
  File "/Users/erik/code/fontParts/Lib/fontParts/base/glyph.py", line 1669, in fromMathGlyph
  File "lib/fontObjects/fontPartsWrappers.pyc", line 87, in wrapper
  File "/Users/erik/code/fontParts/Lib/fontParts/base/glyph.py", line 1697, in _fromMathGlyph
KeyError: 'name'

Mysteriously, the anchors of the mathGlyph appear to have names:

[{'name': 'top', 'identifier': None, 'x': 235.72533333333334, 'y': 854.5736666666667, 'color': None}, {'name': 'bottom', 'identifier': None, 'x': 258.90666666666664, 'y': 0.0, 'color': None}]

Maybe something in RF's fontPartsWrappers?

benkiel commented 1 year ago

@LettError Must be, as fontParts doesn't implement _get_name or _set_name in base

benkiel commented 1 year ago

But, maybe we should change this up to check if there is a name, looking into it

LettError commented 1 year ago

It's looking for a name in a dict?

benkiel commented 1 year ago

It's looking for the anchor name from the math glyph object. I think that changing it to a get makes sense so that if there is no anchor name, it gets set to None.

justvanrossum commented 1 year ago

But Erik mentioned that the anchors do have names, so something appears off. Not failing when there's no name may not solve this actual problem.

benkiel commented 1 year ago

@justvanrossum right, the above solves the first issue, but yes, not sure about what fontPartsWrappers is doing

benkiel commented 1 year ago

Looking through all the code, I'm not seeing how that name is getting lost

benkiel commented 1 year ago

But, yes, @justvanrossum, you're right, the above solves the key error for the optional name and now color, but not the issue @LettError is running into. I looked at Defcon, which is what RF is wrapping, and I don't see how the name is getting lost there, looked at RF code, not seeing it there, etc. So I'm also at a bit of a loss.

justvanrossum commented 1 year ago

Mysteriously, the anchors of the mathGlyph appear to have names

The traceback shows this is not the case, though. Did you find out you have anchors without a name after all? If not, I would suggest looking into that first. If yes, then #679 is likely the right solution.

I feel the analysis of this bug is incomplete, and therefore the proposed fix could be premature. Especially since toMathGlyph() does guarantee for anchor names to exist.

LettError commented 1 year ago

Analysis of the bug is indeed not complete, it was not possible to make a neatly isolated testable thing. However, some progress.

Note 1: maybe I misreported anchor rather than guideline. I don't know whether I reported it wrong, or whether I tested with a different glyph with different anchors. I understand this is not ideal, but there is progress anyway!

Note 2: There definitely is an issue with guideline, it may also be anchor. But as that is already changed in this branch and that specific traceback did not come back so I will just leave the fix in anchor.

New issue 1: guideline["name"] -> guideline.get("name") # ok that works, phew New issue 2: guideline["color"] -> guideline.get("color") # ah, new traceback, see below

Changes made in this branch: https://github.com/robotools/fontParts/blob/b4cb042c0693e90b38bb4d373c82e97d63a41b1a/Lib/fontParts/base/glyph.py#L1709

MathGlyph guidelines, no names, and only 1 with a color.

{'x': 171.16424854606674, 'y': 398.3774716865626, 'angle': 39.17460681282844, 'identifier': '4jjGjAobNR'}
{'x': 296.0430976430976, 'y': 196.8342822161004, 'angle': 214.52107759503738, 'identifier': 'nFXtuqy0Zx'}
{'x': 224.6831955922865, 'y': 415.5753902662994, 'angle': 39.19414618573358, 'identifier': 'IO6VxBZgbG'}
{'x': 415.8800122436486, 'y': 613.9582491582493, 'angle': 273.92069015079494, 'color': '0.999,0.001,0.001,0.999', 'identifier': 'sieX9anfuL'}

Running the code in this branch, with these guidelines gives a new traceback centered on color:

Traceback (most recent call last):
  File "testRefactor_RF.py", line 69, in <module>
  File "/Users/erik/code/fontParts/Lib/fontParts/base/glyph.py", line 1680, in fromMathGlyph
  File "lib/fontObjects/fontPartsWrappers.pyc", line 87, in wrapper
  File "/Users/erik/code/fontParts/Lib/fontParts/base/glyph.py", line 1705, in _fromMathGlyph
  File "/Users/erik/code/fontParts/Lib/fontParts/base/glyph.py", line 1389, in appendGuideline
  File "/Users/erik/code/fontParts/Lib/fontParts/base/normalizers.py", line 921, in normalizeColor
TypeError: Colors must be tuple instances, not Color.

So, the name issues appear gone at least :)

benkiel commented 1 year ago

Ah, that error is the normalizer being grumpy, we can fix that!

benkiel commented 1 year ago

I guess the thing to do in the normalizer would be to transform the Color object to a tuple

benkiel commented 1 year ago

Ok, that's weird as the normalizer allows Color, at least a fontParts.base.Color