Read colors and set UFO keys for Glyphs ColorIndex and public.markColor #285
Conversation
|
Cool! Could you add tests? |
|
Oh, and could you remove the italic commits? |
|
Definitely. I totally forgot to rebase after moving the italic stuff to ufo2ft. I'll add some tests, too. |
Lib/glyphsLib/builder/glyph.py
Outdated
| ufo_glyph.lib[PUBLIC_PREFIX + 'markColor'] = GLYPHS_COLORS[color_index] | ||
| color_tuple = None | ||
| if isinstance(color_index, list): | ||
| color_tuple = ','.join(['{0:.4f}'.format(i/255.0) |
There was a problem hiding this comment.
Note that we are using python 3 division operator (or if we don’t we should, by importing it from __future__.
You need to use two slashes for integer division; one slash is always float division.
There was a problem hiding this comment.
The join method of str accept an iterable, so you can pass it a generator expression directly without needing to create a temporary list (with a list comprehension).
“”.join(x for x in something)
Lib/glyphsLib/builder/glyph.py
Outdated
| if i in range(1, 255) else str(int(i/255)) for i in color_index]) | ||
| elif isinstance(color_index, int) and color_index in range(len(GLYPHS_COLORS)): | ||
| color_tuple = GLYPHS_COLORS[color_index] | ||
| if color_tuple is not None: |
There was a problem hiding this comment.
Shouldn’t we at least log a warning that we dropped some data even though invalid or unsupported that we could not fit?
There was a problem hiding this comment.
Well, the parser won't allow most invalid things to even get to this point, but it is possible to manually put in some bad values like a tuple of (344, -400, 23, 1) so we can warn for that. A Glyphs color index of > 11 will fail because GLYPHS_COLORS only has 12 elements. I put in some warnings for either of those cases.
Lib/glyphsLib/builder/glyph.py
Outdated
| color_tuple = None | ||
| if isinstance(color_index, list): | ||
| color_tuple = ','.join(['{0:.4f}'.format(i/255.0) | ||
| if i in range(1, 255) else str(int(i/255)) for i in color_index]) |
There was a problem hiding this comment.
Curious how did you come up with this formula?
There was a problem hiding this comment.
A little trial and error. We know it needs to be in the range 0-255, but if we don't adjust the precision then the UFO gets a bunch of ugly values like 0.0,0.952941176471,1.0,0.00392156862745
If we set precision to 4 places we get 0.0000,0.9529,1.0000,0.0039 which is still kind of ugly. That's why the weird formula to only give 4 place precision for anything between 0 and 1, but for 0 and 1 we don't need anything so ignore it on those.
for invalid colors that parser allows, but are not currently valid Added tests
|
Ok, added some warnings and tests. There didn't seem to be a place for this kind of test so I added GlyphPropertiesTest in the builder tests which I figure could test other GSGlyph specific properties as well. |
|
I added the The |
| @@ -1131,5 +1131,40 @@ def test_to_ufo_draw_paths_qcurve(self): | |||
| self.assertEqual(first_segment_type, 'qcurve') | |||
|
|
|||
|
|
|||
| class GlyphPropertiesTest(unittest.TestCase): | |||
There was a problem hiding this comment.
you're testing to_ufos function (or that what you are calling in the test), so this test_glyph_color method should go to ToUfosTest class, I think.
@belluzj wdyt?
There was a problem hiding this comment.
To me the current class name is good because it spells out the intent of the tests (test glyph properties) rather than the technical details (the test calls to_ufos). Beyond that, I don't know what the best practice is for organizing tests in python...
As mentioned here: #116
With colors coming from outside Glyphs we were failing on
GLYPHS_COLORS[color_index]when color_index wasn't in GLYPHS_COLORS.Here we should check for a tuple or a Glyphs color index and set the keys accordingly. Using the FontLab export script to make a Glyphs file results in tuples like (235,0,255,1), but the UFO spec requires integer or floats between 0 and 1 (http://unifiedfontobject.org/versions/ufo3/conventions/#colors). So check here to make sure we get what we expect.
I'm not sure about how many decimal places we care about so put 4 for now.