From 7474aa7a3a6c2a7cacd3b3c5067201bb608029c4 Mon Sep 17 00:00:00 2001 From: capellancitizen Date: Wed, 28 Aug 2024 20:12:16 -0400 Subject: [PATCH] Fixed hidden objects being stitched out when cloned (Fix #3167) (#3171) Extracted Base.descendants into a util function --- lib/elements/clone.py | 11 +++--- lib/elements/utils.py | 69 ++++++++++++++++++++++++++++++++++-- lib/extensions/base.py | 65 +++++---------------------------- tests/test_clone.py | 34 +++++++++++++++++- tests/test_elements_utils.py | 52 +++++++++++++++++++++++++++ 5 files changed, 166 insertions(+), 65 deletions(-) create mode 100644 tests/test_elements_utils.py diff --git a/lib/elements/clone.py b/lib/elements/clone.py index ce9c2162d..73f3c6c05 100644 --- a/lib/elements/clone.py +++ b/lib/elements/clone.py @@ -72,7 +72,8 @@ class Clone(EmbroideryElement): source_elements = self.clone_to_elements(source_node) return [element.get_cache_key(previous_stitch) for element in source_elements] - def clone_to_elements(self, node): + def clone_to_elements(self, node) -> List[EmbroideryElement]: + # Only used in get_cache_key_data, actual embroidery uses nodes_to_elements+iterate_nodes from .utils import node_to_elements elements = [] if node.tag in EMBROIDERABLE_TAGS: @@ -107,11 +108,13 @@ class Clone(EmbroideryElement): Could possibly be refactored into just a generator - being a context manager is mainly to control the lifecycle of the elements that are cloned (again, for testing convenience primarily) """ + from .utils import nodes_to_elements, iterate_nodes + cloned_nodes = self.resolve_clone() try: # In a try block so we can ensure that the cloned_node is removed from the tree in the event of an exception. # Otherwise, it might be left around on the document if we throw for some reason. - yield self.clone_to_elements(cloned_nodes[0]) + yield nodes_to_elements(iterate_nodes(cloned_nodes[0])) finally: # Remove the "manually cloned" tree. for cloned_node in cloned_nodes: @@ -163,7 +166,7 @@ class Clone(EmbroideryElement): ret = [cloned_node] - # For aesthetic purposes, transform allof the cloned command symbols so they're facing upwards + # For aesthetic purposes, transform all of the cloned command symbols so they're facing upwards point_command_symbols_up(cloned_node) # We need to copy all commands that were attached directly to the href'd node @@ -245,7 +248,7 @@ def clone_with_fixup(parent: BaseElement, node: BaseElement) -> BaseElement: parent.append(cloned) id_map[f"#{node.get_id()}"] = f"#{cloned.get_id()}" - for child in node.getchildren(): + for child in node: clone_children(cloned, child) return cloned diff --git a/lib/elements/utils.py b/lib/elements/utils.py index a340f838b..79d5ac937 100644 --- a/lib/elements/utils.py +++ b/lib/elements/utils.py @@ -3,9 +3,15 @@ # Copyright (c) 2010 Authors # Licensed under the GNU GPL version 3.0 or later. See the file LICENSE for details. -from ..commands import is_command +from lxml.etree import Comment +from typing import List, Optional +from inkex import BaseElement + +from ..commands import is_command, layer_commands from ..marker import has_marker -from ..svg.tags import EMBROIDERABLE_TAGS, SVG_IMAGE_TAG, SVG_TEXT_TAG +from ..svg.tags import (CONNECTOR_TYPE, EMBROIDERABLE_TAGS, INKSCAPE_GROUPMODE, + NOT_EMBROIDERABLE_TAGS, SVG_CLIPPATH_TAG, SVG_DEFS_TAG, + SVG_GROUP_TAG, SVG_MASK_TAG, SVG_IMAGE_TAG, SVG_TEXT_TAG) from .clone import Clone, is_clone from .element import EmbroideryElement from .empty_d_object import EmptyDObject @@ -17,7 +23,7 @@ from .stroke import Stroke from .text import TextObject -def node_to_elements(node, clone_to_element=False): # noqa: C901 +def node_to_elements(node, clone_to_element=False) -> List[EmbroideryElement]: # noqa: C901 if is_clone(node) and not clone_to_element: # clone_to_element: get an actual embroiderable element once a clone has been defined as a clone return [Clone(node)] @@ -59,3 +65,60 @@ def nodes_to_elements(nodes): elements.extend(node_to_elements(node)) return elements + + +def iterate_nodes(node: BaseElement, # noqa: C901 + selection: Optional[List[BaseElement]] = None, + troubleshoot=False) -> List[BaseElement]: + # Postorder traversal of selected nodes and their descendants. + # Returns all nodes if there is no selection. + def walk(node: BaseElement, selected: bool) -> List[BaseElement]: + nodes = [] + + if node.tag == Comment: + return [] + + element = EmbroideryElement(node) + + if element.has_command('ignore_object'): + return [] + + if node.tag == SVG_GROUP_TAG and node.get(INKSCAPE_GROUPMODE) == "layer": + if len(list(layer_commands(node, "ignore_layer"))): + return [] + + if (node.tag in EMBROIDERABLE_TAGS or node.tag == SVG_GROUP_TAG) and element.get_style('display', 'inline') is None: + return [] + + # defs, masks and clippaths can contain embroiderable elements + # but should never be rendered directly. + if node.tag in [SVG_DEFS_TAG, SVG_MASK_TAG, SVG_CLIPPATH_TAG]: + return [] + + # command connectors with a fill color set, will glitch into the elements list + if is_command(node) or node.get(CONNECTOR_TYPE): + return [] + + if not selected: + if selection: + if node in selection: + selected = True + else: + # if the user didn't select anything that means we process everything + selected = True + + for child in node: + nodes.extend(walk(child, selected)) + + if selected: + if node.tag == SVG_GROUP_TAG: + pass + elif (node.tag in EMBROIDERABLE_TAGS or is_clone(node)) and not has_marker(node): + nodes.append(node) + # add images, text and elements with a marker for the troubleshoot extension + elif troubleshoot and (node.tag in NOT_EMBROIDERABLE_TAGS or has_marker(node)): + nodes.append(node) + + return nodes + + return walk(node, False) diff --git a/lib/extensions/base.py b/lib/extensions/base.py index 4d5aacfe7..5f840417a 100644 --- a/lib/extensions/base.py +++ b/lib/extensions/base.py @@ -6,18 +6,12 @@ import os import inkex -from lxml.etree import Comment -from ..commands import is_command, layer_commands -from ..elements import EmbroideryElement, nodes_to_elements -from ..elements.clone import is_clone from ..i18n import _ -from ..marker import has_marker from ..metadata import InkStitchMetadata from ..svg import generate_unique_id -from ..svg.tags import (CONNECTOR_TYPE, EMBROIDERABLE_TAGS, INKSCAPE_GROUPMODE, - NOT_EMBROIDERABLE_TAGS, SVG_CLIPPATH_TAG, SVG_DEFS_TAG, - SVG_GROUP_TAG, SVG_MASK_TAG) +from ..svg.tags import INKSCAPE_GROUPMODE, SVG_GROUP_TAG +from ..elements.utils import iterate_nodes, nodes_to_elements from ..update import update_inkstitch_document @@ -62,58 +56,15 @@ class InkstitchExtension(inkex.EffectExtension): inkex.errormsg(_("Tip: Run Extensions > Ink/Stitch > Troubleshoot > Troubleshoot Objects") + "\n") - def descendants(self, node, selected=False, troubleshoot=False): # noqa: C901 - nodes = [] - - if node.tag == Comment: - return [] - - element = EmbroideryElement(node) - - if element.has_command('ignore_object'): - return [] - - if node.tag == SVG_GROUP_TAG and node.get(INKSCAPE_GROUPMODE) == "layer": - if len(list(layer_commands(node, "ignore_layer"))): - return [] - - if (node.tag in EMBROIDERABLE_TAGS or node.tag == SVG_GROUP_TAG) and element.get_style('display', 'inline') is None: - return [] - - # defs, masks and clippaths can contain embroiderable elements - # but should never be rendered directly. - if node.tag in [SVG_DEFS_TAG, SVG_MASK_TAG, SVG_CLIPPATH_TAG]: - return [] - - # command connectors with a fill color set, will glitch into the elements list - if is_command(node) or node.get(CONNECTOR_TYPE): - return [] - - if self.svg.selection: - if node in list(self.svg.selection): - selected = True - else: - # if the user didn't select anything that means we process everything - selected = True - - for child in node: - nodes.extend(self.descendants(child, selected, troubleshoot)) - - if selected: - if node.tag == SVG_GROUP_TAG: - pass - elif (node.tag in EMBROIDERABLE_TAGS or is_clone(node)) and not has_marker(node): - nodes.append(node) - # add images, text and elements with a marker for the troubleshoot extension - elif troubleshoot and (node.tag in NOT_EMBROIDERABLE_TAGS or has_marker(node)): - nodes.append(node) - - return nodes - def get_nodes(self, troubleshoot=False): # Postorder traversal of selected nodes and their descendants. # Returns all nodes if there is no selection. - return self.descendants(self.document.getroot(), troubleshoot=troubleshoot) + if self.svg.selection: + selection = list(self.svg.selection) + else: + selection = None + + return iterate_nodes(self.document.getroot(), selection=selection, troubleshoot=troubleshoot) def get_elements(self, troubleshoot=False): self.elements = nodes_to_elements(self.get_nodes(troubleshoot)) diff --git a/tests/test_clone.py b/tests/test_clone.py index 1c84f3e9f..07683a16b 100644 --- a/tests/test_clone.py +++ b/tests/test_clone.py @@ -1,6 +1,6 @@ from lib.elements import Clone, EmbroideryElement, FillStitch from lib.commands import add_commands -from lib.svg.tags import INKSTITCH_ATTRIBS, SVG_RECT_TAG +from lib.svg.tags import INKSTITCH_ATTRIBS, SVG_RECT_TAG, INKSCAPE_LABEL from lib.utils import cache_module from inkex import SvgDocumentElement, Rectangle, Circle, Group, Use, Transform, TextElement from inkex.tester import TestCase @@ -19,6 +19,7 @@ def element_fill_angle(element: EmbroideryElement) -> Optional[float]: class CloneElementTest(TestCase): + # Monkey-patch the cahce to forcibly disable it: We may need to refactor this out for tests. def setUp(self): from pytest import MonkeyPatch self.monkeypatch = MonkeyPatch() @@ -78,6 +79,37 @@ class CloneElementTest(TestCase): self.assertEqual(len(elements), 1) self.assertAlmostEqual(element_fill_angle(elements[0]), 30) + def test_hidden_cloned_elements_not_embroidered(self): + root = svg() + g = root.add(Group()) + g.add(Rectangle(attrib={ + INKSCAPE_LABEL: "NotHidden", + "width": "10", + "height": "10" + })) + g.add(Rectangle(attrib={ + INKSCAPE_LABEL: "Hidden", + "width": "10", + "height": "10", + "style": "display:none" + })) + hidden_group = g.add(Group(attrib={ + "style": "display:none" + })) + hidden_group.add(Rectangle(attrib={ + INKSCAPE_LABEL: "ChildOfHidden", + "width": "10", + "height": "10", + })) + use = root.add(Use()) + use.href = g + + clone = Clone(use) + + with clone.clone_elements() as elements: + self.assertEqual(len(elements), 1) + self.assertEqual(elements[0].node.get(INKSCAPE_LABEL), "NotHidden") + def test_angle_rotated(self): root: SvgDocumentElement = svg() rect = root.add(Rectangle(attrib={ diff --git a/tests/test_elements_utils.py b/tests/test_elements_utils.py new file mode 100644 index 000000000..651635beb --- /dev/null +++ b/tests/test_elements_utils.py @@ -0,0 +1,52 @@ +from lib.elements import utils, FillStitch +from inkex import Rectangle, Group, Style +from inkex.tester import TestCase +from inkex.tester.svg import svg + + +class ElementsUtilsTest(TestCase): + # These tests test two functions at once, but they're sort of complimentary. + # Might suggest that they could be combined in a later refactor? + def test_iterate_nodes_to_elements(self): + root = svg() + g = root.add(Group()) + rect = g.add(Rectangle(attrib={ + "width": "10", + "height": "10" + })) + hidden_rect = g.add(Rectangle(attrib={ # noqa: F841 + "width": "10", + "height": "10", + "style": "display:none" + })) + hidden_group = g.add(Group(attrib={ + "style": "display:none" + })) + child_of_hidden = hidden_group.add(Rectangle(attrib={ # noqa: F841 + "width": "10", + "height": "10", + })) + + elements = utils.nodes_to_elements(utils.iterate_nodes(g)) + self.assertEqual(len(elements), 1) + self.assertEqual(type(elements[0]), FillStitch) + self.assertEqual(elements[0].node, rect) + + def test_iterate_nodes_to_elements_root_embroiderable(self): + """ Case where the root node is directly embroiderable """ + root = svg() + rect = root.add(Rectangle(attrib={ + "width": "10", + "height": "10" + })) + + elements = utils.nodes_to_elements(utils.iterate_nodes(rect)) + self.assertEqual(len(elements), 1) + self.assertEqual(type(elements[0]), FillStitch) + self.assertEqual(elements[0].node, rect) + + # Now make the element hidden: It shouldn't return an element + rect.style = rect.style + Style({"display": "none"}) + + elements = utils.nodes_to_elements(utils.iterate_nodes(rect)) + self.assertEqual(len(elements), 0)