fontParts icon indicating copy to clipboard operation
fontParts copied to clipboard

anchor and guideline may not have specific keys in dict

Open LettError opened this issue 3 years ago • 13 comments

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?

LettError avatar Jan 05 '23 13:01 LettError

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

benkiel avatar Jan 05 '23 14:01 benkiel

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

benkiel avatar Jan 05 '23 14:01 benkiel

It's looking for a name in a dict?

LettError avatar Jan 05 '23 15:01 LettError

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.

benkiel avatar Jan 05 '23 16:01 benkiel

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.

justvanrossum avatar Jan 05 '23 16:01 justvanrossum

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

benkiel avatar Jan 05 '23 17:01 benkiel

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

benkiel avatar Jan 05 '23 18:01 benkiel

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.

benkiel avatar Jan 05 '23 18:01 benkiel

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.

justvanrossum avatar Jan 05 '23 21:01 justvanrossum

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 :)

LettError avatar Jan 05 '23 22:01 LettError

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

benkiel avatar Jan 05 '23 23:01 benkiel

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

benkiel avatar Jan 07 '23 15:01 benkiel

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

benkiel avatar Jan 07 '23 15:01 benkiel