Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix color component layers #974

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Fix color component layers #974

merged 3 commits into from
Jan 29, 2024

Conversation

simoncozens
Copy link
Collaborator

This is a tiny fix for #973. In components.py we gather a list of color layers and take the index to get the new component name, like so:

color_layers = [
l for l in component_glyph.layers if l._is_color_palette_layer()
]
for i, l in enumerate(color_layers):
if l._color_palette_index() == layer._color_palette_index():
if l.layerId != l.associatedMasterId:
# If it is not a master layer, we rename it in
# _to_ufo_color_palette_layers(), so we reference the
# same name here.
component_name += f".color{i}"

We do this because we do the same operation in _to_ufo_color_palette_layers to rename the glyphs:

for i, layer in enumerate(layers):
if layer.components and layer.attributes:
colorLayers.append(_to_component_paint(layer.components[0]))
continue
if layer.layerId == master.id:
# This is master layer, we can re-use its UFO glyph.
layerGlyphName = glyph.name
else:
# Not the master layer, create a new UFO glyph for it.
layerGlyphName = f"{glyph.name}.color{i}"

However, the list which we take the index from there is actually built here:

layerMapping = [
(l.layerId, l._color_palette_index())
for l in glyph.layers
if l._is_color_palette_layer()
and l.associatedMasterId == layer.associatedMasterId
]

So in components.py we need to make sure we are building the same list by also testing for l.associatedMasterId == layer.associatedMasterId.

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH! thanks!!

@anthrotype
Copy link
Member

would be nice if we could also add that test file of yours to the test suite. Maybe just parsing it and converting to_designspace and asserting that the component names are as expected with the correct .color{i} suffix, perhaps inside tests/builder/components_test.py. Could you please add that?

@anthrotype anthrotype merged commit ce6d451 into main Jan 29, 2024
10 checks passed
@anthrotype anthrotype deleted the fix-973 branch January 29, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants