Fix issue with bad color names (#3816)

* fix issue with bad color names and define element colors at one place and reuse
* fix bad tartan color
* verify color in gradient block
* add thread color tests
* use default color behavior for elements linked to non-existing definitions (gradients)
* Added mypy change for tests (authored by: CapellanCitizen)

17 files changed, 219 insertions, 41 deletions

diff --git a/lib/elements/element.py b/lib/elements/element.py
index 7b558506..1ca78b04 100644
--- a/lib/elements/element.py
+++ b/lib/elements/element.py
@@ -198,12 +198,35 @@ class EmbroideryElement(object):
         style = element_style.get(style_name, default)
         if style in ['none', 'None']:
             style = None
-        elif style == 'currentColor':
-            style = element_style(style_name)
         return style
 
     @property
     @cache
+    def fill_color(self):
+        try:
+            color = self.node.get_computed_style("fill")
+        except (inkex.ColorError, ValueError):
+            # SVG spec says the default fill is black
+            # A color error could show up, when an element has an unrecognized color name
+            # A value error could show up, when for example when an element links to a non-existent gradient
+            # TODO: This will also apply to currentcolor and alike which will render in black
+            return "black"
+        return color
+
+    @property
+    @cache
+    def stroke_color(self):
+        try:
+            color = self.node.get_computed_style("stroke")
+        except (inkex.ColorError, ValueError):
+            # A color error could show up, when an element has an unrecognized color name
+            # A value error could show up, when for example when an element links to a non-existent gradient
+            # TODO: This will also apply to currentcolor and alike which will not render
+            return None
+        return color
+
+    @property
+    @cache
     def stroke_scale(self):
         # How wide is the stroke, after the transforms are applied?
         #
diff --git a/lib/elements/fill_stitch.py b/lib/elements/fill_stitch.py
index 555c7e09..7ce3188a 100644
--- a/lib/elements/fill_stitch.py
+++ b/lib/elements/fill_stitch.py
@@ -7,7 +7,7 @@ import math
 import re
 
 import numpy as np
-from inkex import LinearGradient
+from inkex import Color, ColorError, LinearGradient
 from shapely import geometry as shgeo
 from shapely import set_precision
 from shapely.errors import GEOSException
@@ -632,14 +632,12 @@ class FillStitch(EmbroideryElement):
 
     @property
     def color(self):
-        # SVG spec says the default fill is black
-        return self.get_style("fill", "#000000")
+        return self.fill_color
 
     @property
     def gradient(self):
-        gradient = self.node.get_computed_style("fill")
-        if isinstance(gradient, LinearGradient):
-            return gradient
+        if isinstance(self.fill_color, LinearGradient):
+            return self.fill_color
         return None
 
     @property
@@ -1032,7 +1030,10 @@ class FillStitch(EmbroideryElement):
     def do_underlay(self, shape, starting_point):
         color = self.color
         if self.gradient is not None and self.fill_method == 'linear_gradient_fill':
-            color = self.gradient.stops[0].get_computed_style('stop-color')
+            try:
+                color = self.gradient.stops[0].get_computed_style('stop-color')
+            except ColorError:
+                color = Color('black')
 
         stitch_groups = []
         for i in range(len(self.fill_underlay_angle)):
diff --git a/lib/elements/satin_column.py b/lib/elements/satin_column.py
index ff5cfe86..3a29aca7 100644
--- a/lib/elements/satin_column.py
+++ b/lib/elements/satin_column.py
@@ -243,7 +243,7 @@ class SatinColumn(EmbroideryElement):
 
     @property
     def color(self):
-        return self.get_style("stroke")
+        return self.stroke_color
 
     @property
     @param('zigzag_spacing_mm',
diff --git a/lib/elements/stroke.py b/lib/elements/stroke.py
index 6e01151d..3740371f 100644
--- a/lib/elements/stroke.py
+++ b/lib/elements/stroke.py
@@ -50,7 +50,7 @@ class Stroke(EmbroideryElement):
 
     @property
     def color(self):
-        color = self.get_style("stroke")
+        color = self.stroke_color
         if self.cutwork_needle is not None:
             color = ThreadColor(color, description=self.cutwork_needle, chart=self.cutwork_needle)
         return color
diff --git a/lib/elements/utils.py b/lib/elements/utils.py
index 6aec1cc2..43c2387d 100644
--- a/lib/elements/utils.py
+++ b/lib/elements/utils.py
@@ -47,9 +47,9 @@ def node_to_elements(node, clone_to_element=False) -> List[EmbroideryElement]:
 
         if not sew_stack.sew_stack_only:
             element = EmbroideryElement(node)
-            if element.get_style("fill", "black") and not element.get_style('fill-opacity', 1) == "0":
+            if element.fill_color is not None and not element.get_style('fill-opacity', 1) == "0":
                 elements.append(FillStitch(node))
-            if element.get_style("stroke"):
+            if element.stroke_color is not None:
                 if element.get_boolean_param("satin_column") and len(element.path) > 1:
                     elements.append(SatinColumn(node))
                 elif not is_command(element.node):
diff --git a/lib/extensions/break_apart.py b/lib/extensions/break_apart.py
index 21806251..05f9375a 100644
--- a/lib/extensions/break_apart.py
+++ b/lib/extensions/break_apart.py
@@ -39,7 +39,7 @@ class BreakApart(InkstitchExtension):
                 elements.append(EmbroideryElement(node))
 
         for element in elements:
-            if not element.get_style("fill", "black"):
+            if not element.fill_color:
                 continue
 
             # we don't want to touch valid elements
diff --git a/lib/extensions/gradient_blocks.py b/lib/extensions/gradient_blocks.py
index 79665a10..364ff2b2 100644
--- a/lib/extensions/gradient_blocks.py
+++ b/lib/extensions/gradient_blocks.py
@@ -5,8 +5,8 @@
 
 from math import degrees, pi
 
-from inkex import (DirectedLineSegment, Group, LinearGradient, Path,
-                   PathElement, Transform, errormsg)
+from inkex import (Color, ColorError, DirectedLineSegment, Group,
+                   LinearGradient, Path, PathElement, Transform, errormsg)
 from shapely import geometry as shgeo
 from shapely.affinity import rotate
 from shapely.ops import split
@@ -67,7 +67,7 @@ class GradientBlocks(InkstitchExtension):
             for i, shape in enumerate(fill_shapes):
                 if shape.area < 15:
                     continue
-                color = attributes[i]['color']
+                color = verify_color(attributes[i]['color'])
                 style['fill'] = color
                 is_gradient = attributes[i]['is_gradient']
                 angle = degrees(attributes[i]['angle'])
@@ -146,7 +146,7 @@ def gradient_shapes_and_attributes(element, shape, unit_multiplier):
         split_line = rotate(split_line, angle, origin=split_point, use_radians=True)
         offset_line = split_line.parallel_offset(1, 'right')
         polygon = split(shape, split_line)
-        color = stop_styles[i]['stop-color']
+        color = verify_color(stop_styles[i]['stop-color'])
         # does this gradient line split the shape
         offset_outside_shape = len(polygon.geoms) == 1
         for poly in polygon.geoms:
@@ -178,6 +178,14 @@ def gradient_shapes_and_attributes(element, shape, unit_multiplier):
     return polygons, attributes
 
 
+def verify_color(color):
+    try:
+        Color(color)
+    except ColorError:
+        return "black"
+    return color
+
+
 if __name__ == '__main__':
     e = GradientBlocks()
     e.run()
diff --git a/lib/extensions/params.py b/lib/extensions/params.py
index cb38585e..0bba13fe 100755
--- a/lib/extensions/params.py
+++ b/lib/extensions/params.py
@@ -721,9 +721,9 @@ class Params(InkstitchExtension):
             elif node.tag in EMBROIDERABLE_TAGS and not node.get_path():
                 pass
             else:
-                if element.get_style("fill", 'black') and not element.get_style("fill-opacity", 1) == "0":
+                if element.fill_color is not None and not element.get_style("fill-opacity", 1) == "0":
                     classes.append(FillStitch)
-                if element.get_style("stroke") is not None:
+                if element.stroke_color is not None:
                     classes.append(Stroke)
                     if len(element.path) > 1:
                         classes.append(SatinColumn)
diff --git a/lib/marker.py b/lib/marker.py
index a2b0965a..cd700674 100644
--- a/lib/marker.py
+++ b/lib/marker.py
@@ -72,8 +72,8 @@ def get_marker_elements(node, marker, get_fills=True, get_strokes=True, get_sati
             continue
 
         element = EmbroideryElement(marker)
-        fill = element.get_style('fill')
-        stroke = element.get_style('stroke')
+        fill = element.fill_color
+        stroke = element.stroke_color
 
         if get_fills and fill is not None:
             fill = FillStitch(marker).shape
diff --git a/lib/sew_stack/stitch_layers/stitch_layer.py b/lib/sew_stack/stitch_layers/stitch_layer.py
index 615a36e8..d9a985f6 100644
--- a/lib/sew_stack/stitch_layers/stitch_layer.py
+++ b/lib/sew_stack/stitch_layers/stitch_layer.py
@@ -74,11 +74,11 @@ class StitchLayer:
 
     @property
     def stroke_color(self):
-        return self.element.get_style("stroke")
+        return self.element.stroke_color
 
     @property
     def fill_color(self):
-        return self.element.get_style("stroke")
+        return self.element.fill_color
 
     def to_stitch_groups(self, *args):
         raise NotImplementedError(f"{self.__class__.__name__} must implement to_stitch_groups()!")
diff --git a/lib/stitches/guided_fill.py b/lib/stitches/guided_fill.py
index 5d9a6018..16f0f2e8 100644
--- a/lib/stitches/guided_fill.py
+++ b/lib/stitches/guided_fill.py
@@ -180,7 +180,7 @@ def apply_stitches(line, max_stitch_length, num_staggers, row_spacing, row_num,
     # corners.
     if not threshold:
         threshold = row_spacing / 2.0
-    simplified_line = line.simplify(threshold, False)
+    simplified_line = line.simplify(threshold, preserve_topology=False)
     simplified_points = [shgeo.Point(x, y) for x, y in simplified_line.coords]
 
     extra_points = []
diff --git a/lib/stitches/linear_gradient_fill.py b/lib/stitches/linear_gradient_fill.py
index 2fb03bf9..589a1f23 100644
--- a/lib/stitches/linear_gradient_fill.py
+++ b/lib/stitches/linear_gradient_fill.py
@@ -6,7 +6,7 @@
 from math import ceil, floor, sqrt
 
 import numpy as np
-from inkex import DirectedLineSegment, Transform
+from inkex import Color, ColorError, DirectedLineSegment, Transform
 from networkx import is_empty
 from shapely import segmentize
 from shapely.affinity import rotate
@@ -75,7 +75,10 @@ def _get_gradient_info(fill, bbox):
         # but inkex/tinycss fails on stop color styles when it is not in the style attribute, but in it's own stop-color attribute
         colors = []
         for i, stop in enumerate(fill.gradient.stops):
-            color = stop.get_computed_style('stop-color')
+            try:
+                color = stop.get_computed_style('stop-color')
+            except ColorError:
+                color = Color('black')
             opacity = stop.get_computed_style('stop-opacity')
             if float(opacity) == 0:
                 color = 'none'
diff --git a/lib/tartan/palette.py b/lib/tartan/palette.py
index 25bd2100..b8c36331 100644
--- a/lib/tartan/palette.py
+++ b/lib/tartan/palette.py
@@ -8,9 +8,10 @@ import re
 from typing import TYPE_CHECKING, List, cast
 
 import wx
-from inkex import Color
+from inkex import Color, ColorError
 
 from .colors import string_to_color
+
 if TYPE_CHECKING:
     from ..gui.tartan.stripe_panel import StripePanel
 
@@ -175,7 +176,11 @@ class Palette:
                     color = wx.Colour(color).GetAsString(wx.C2S_HTML_SYNTAX)
                 except wx.PyNoAppError:
                     # however when we render an embroidery element we do not want to open wx.App
-                    color = str(Color(color).to_named())
+                    try:
+                        color = str(Color(color).to_named())
+                    except ColorError:
+                        color = None
+
                 if not color:
                     color = '#000000'
                     render = 0
diff --git a/lib/threads/color.py b/lib/threads/color.py
index 699f4448..335dc32b 100644
--- a/lib/threads/color.py
+++ b/lib/threads/color.py
@@ -5,19 +5,20 @@
 
 import colorsys
 
-from inkex import Color
+from inkex import Color, ColorError
+
 from pyembroidery.EmbThread import EmbThread
 
 
 class ThreadColor(object):
     def __init__(self, color, name=None, number=None, manufacturer=None, description=None, chart=None):
-        '''
-        avoid error messages:
-          * set colors with a gradient to black
-          * currentColor/context-fill/context-stroke: should not just be black, but we want to avoid
-            error messages until inkex will be able to handle these css properties
-        '''
-        if isinstance(color, str) and color.startswith(('url', 'currentColor', 'context')):
+        self.rgb = None
+
+        if isinstance(color, str) and color.lower().startswith(('url', 'currentcolor', 'context')):
+            '''
+            Avoid error messages for currentcolor, context-fill, context-stroke and every string starting with an url.
+            they should not just be black, but we want to avoid error messages
+            '''
             color = None
         elif isinstance(color, str) and color.startswith('rgb'):
             color = tuple(int(value) for value in color[4:-1].split(','))
@@ -33,11 +34,19 @@ class ThreadColor(object):
             self.rgb = (color.get_red(), color.get_green(), color.get_blue())
             return
         elif isinstance(color, str):
-            self.rgb = tuple(Color(color).to('rgb'))
+            try:
+                self.rgb = tuple(Color(color).to('rgb'))
+            except ColorError:
+                self.rgb = None
         elif isinstance(color, (list, tuple)):
             self.rgb = tuple(color)
-        else:
-            raise ValueError("Invalid color: " + repr(color))
+
+        if self.rgb is None:
+            '''
+            Instead of erroring out, we want to set everything to black at this point.
+            This includes for example patterns and gradients
+            '''
+            self.rgb = (0, 0, 0)
 
         self.name = name
         self.number = number
diff --git a/lib/update.py b/lib/update.py
index e1b62cab..2055da7d 100644
--- a/lib/update.py
+++ b/lib/update.py
@@ -170,7 +170,7 @@ def _update_to_one(element):  # noqa: C901
         element.set_param('running_stitch_length_mm', 1.5)
 
     # convert legacy stroke_method
-    if element.get_style("stroke") and not element.node.get('inkscape:connection-start', None):
+    if element.stroke_color and not element.node.get('inkscape:connection-start', None):
         # manual stitch
         legacy_manual_stitch = element.get_boolean_param('manual_stitch', False)
         if legacy_manual_stitch is True:
diff --git a/mypy.ini b/mypy.ini
index 709e554e..4a06d5e3 100644
--- a/mypy.ini
+++ b/mypy.ini
@@ -25,6 +25,10 @@ disallow_untyped_defs = True
 [mypy-lib.tartan.*]
 ignore_errors = True
 
+[mypy-tests.*]
+# The tests should be typechecked because they're all new code, and because they're tests we don't really care if they have perfect annotations.
+check_untyped_defs = True
+
 # These libraries are missing type information
 [mypy-colormath2.*]
 ignore_missing_imports = True
diff --git a/tests/test_threads_color.py b/tests/test_threads_color.py
index a5442359..075312e6 100644
--- a/tests/test_threads_color.py
+++ b/tests/test_threads_color.py
@@ -1,3 +1,8 @@
+from inkex import LinearGradient, Rectangle, Stop, SvgDocumentElement
+from inkex.tester.svg import svg
+
+from lib.elements import EmbroideryElement
+from lib.elements.utils import node_to_elements
 from lib.threads.color import ThreadColor
 
 
@@ -14,3 +19,123 @@ def test_init_color_from_string_hex():
 def test_init_color_from_string_hex_icc():
     color = ThreadColor("#AABBCC icc-color(Some-Profile, 0.1, 0.2, 0.3, 0.4)")
     assert color.rgb == (170, 187, 204)
+
+
+def test_invalid_color():
+    # defaults to black
+    color = ThreadColor("bad_color")
+    assert color.rgb == (0, 0, 0)
+
+
+def test_fill_color():
+    root: SvgDocumentElement = svg()
+    rect = Rectangle(attrib={
+        "width": "10",
+        "height": "10",
+        "style": "fill:red;"
+    })
+    root.add(rect)
+
+    # test with red color
+    element = EmbroideryElement(rect)
+    assert element.fill_color == [255, 0, 0]
+
+    # test with invalid color (defaults to black)
+    rect.style = "fill:bad_color;"
+    element = EmbroideryElement(rect)
+    assert element.fill_color == "black"
+
+
+def test_stroke_color():
+    root: SvgDocumentElement = svg()
+    rect = Rectangle(attrib={
+        "width": "10",
+        "height": "10",
+        "style": "fill:none;stroke:red;"
+    })
+    root.add(rect)
+
+    # test with red color
+    element = EmbroideryElement(rect)
+    assert element.stroke_color == [255, 0, 0]
+
+    # test with invalid color
+    rect.style = "fill:none;stroke:bad_color;"
+    element = EmbroideryElement(rect)
+    assert element.stroke_color is None
+
+
+def test_gradient_colors():
+    root: SvgDocumentElement = svg()
+
+    defs = root.defs
+    linear_gradient = LinearGradient(
+        attrib={
+            "id": "good_gradient"
+        }
+    )
+    stop1 = Stop(
+        attrib={
+            "style": "stop-color: #ff0000;"
+        }
+    )
+    stop2 = Stop(
+        attrib={
+            "style": "stop-color: bad_color;"
+        }
+    )
+    linear_gradient.add(stop1)
+    linear_gradient.add(stop2)
+    defs.add(linear_gradient)
+
+    rect = Rectangle(attrib={
+        "width": "10",
+        "height": "10",
+        "style": "fill:url(#good_gradient)"
+    })
+    rect.set("inkstitch:fill_method", "linear_gradient_fill")
+    root.add(rect)
+
+    [element] = node_to_elements(root[1])
+    stitch_groups = element.embroider(None)
+
+    assert stitch_groups[0].color == [255, 0, 0]
+    assert stitch_groups[1].color == [0, 0, 0]
+
+
+def test_tartan_colors():
+    root: SvgDocumentElement = svg()
+    rect = Rectangle(attrib={
+        "width": "20",
+        "height": "20",
+    })
+    root.add(rect)
+
+    rect.set('inkstitch:fill_method', 'tartan_fill')
+    rect.set('inkstitch:fill_underlay', False)
+    rect.set(
+        'inkstitch:tartan',
+        '{"symmetry": true, "equal_warp_weft": true, "rotate": 0.0, "scale": 100, "offset_x": 0.0, "offset_y": 0.0,'
+        '"palette": "(#ffff00)/5.0 (#00ffff)/5.0", "output": "embroidery", "stitch_type": "legacy_fill",'
+        '"row_spacing": 1.0, "angle_warp": 0.0, "angle_weft": 90.0, "min_stripe_width": 1.0, "bean_stitch_repeats": 0}'
+    )
+
+    [element] = node_to_elements(root[0])
+    stitch_groups = element.embroider(None)
+
+    assert stitch_groups[0].color == 'yellow'
+    assert stitch_groups[1].color == 'cyan'
+
+    # Set second color to an invalid value. Tartan will disable the color stripe for rendering.
+    rect.set(
+        'inkstitch:tartan',
+        '{"symmetry": true, "equal_warp_weft": true, "rotate": 0.0, "scale": 100, "offset_x": 0.0, "offset_y": 0.0,'
+        '"palette": "(#ffff00)/5.0 (bad_color)/5.0", "output": "embroidery", "stitch_type": "legacy_fill",'
+        '"row_spacing": 1.0, "angle_warp": 0.0, "angle_weft": 90.0, "min_stripe_width": 1.0, "bean_stitch_repeats": 0}'
+    )
+
+    [element] = node_to_elements(root[0])
+    stitch_groups = element.embroider(None)
+
+    assert stitch_groups[0].color == 'yellow'
+    assert len(stitch_groups) == 1