Kicad 7: parse strokes when not all elements are defined

[hvraven]

When saving the schematic with kicad 7 not all of the stroke elements are present all the time. In my case the colour is not specified and only 3 elements are existent.

[hvraven]

just seen that this has been brought up by @hlprasu in #31

[zardini123]

I second this PR. I just ran into this issue earlier.

[mvnmgrx]

The color token seems to be optional in the new KiCad release as @hlprasu mentioned. We would have to make this token optional in kiutils as well, otherwise it will unexpectedly be populated if its missing during parsing. This would only be okay if KiCad 6 does not complain when the color token is missing. Im going to test this when i find some time.

[hvraven]

Why is it a problem if it gets populated? If it gets populated with the same default value it will not change anything and will be read by kicad the same.

[hlprasu]

@hvraven I think we may need to support the case when color is not defined at all. Something like the change below. I have checked this a .kicad_sym file updated by KiCad 7.0.0. Does this seem fine?

diff --git a/src/kiutils/items/common.py b/src/kiutils/items/common.py
index bca15f8..5384ed9 100644
--- a/src/kiutils/items/common.py
+++ b/src/kiutils/items/common.py
@@ -233,7 +233,7 @@ class Stroke():
         Returns:
             - Stroke: Object of the class initialized with the given S-Expression
         """
-        if not isinstance(exp, list) or len(exp) != 4:
+        if not isinstance(exp, list) or len(exp) < 3:
             raise Exception("Expression does not have the correct type")

         if exp[0] != 'stroke':
@@ -246,6 +246,9 @@ class Stroke():
             if item[0] == 'width': object.width = item[1]
             if item[0] == 'type':  object.type = item[1]
             if item[0] == 'color': object.color = ColorRGBA.from_sexpr(item)
+        
+        if len(exp) == 3:
+            object.color = ColorRGBA() # 0, 0, 0, 0 -- default
         return object

     def to_sexpr(self, indent=2, newline=True) -> str:

[hvraven]

@hvraven I think we may need to support the case when color is not defined at all. Something like the change below. I have checked this a .kicad_sym file updated by KiCad 7.0.0. Does this seem fine?

diff --git a/src/kiutils/items/common.py b/src/kiutils/items/common.py
index bca15f8..5384ed9 100644
--- a/src/kiutils/items/common.py
+++ b/src/kiutils/items/common.py
@@ -233,7 +233,7 @@ class Stroke():
         Returns:
             - Stroke: Object of the class initialized with the given S-Expression
         """
-        if not isinstance(exp, list) or len(exp) != 4:
+        if not isinstance(exp, list) or len(exp) < 3:

I think the len(exp) > 4 should be covered as well to exclude extra arguments.

             raise Exception("Expression does not have the correct type")

         if exp[0] != 'stroke':
@@ -246,6 +246,9 @@ class Stroke():
             if item[0] == 'width': object.width = item[1]
             if item[0] == 'type':  object.type = item[1]
             if item[0] == 'color': object.color = ColorRGBA.from_sexpr(item)
+        
+        if len(exp) == 3:
+            object.color = ColorRGBA() # 0, 0, 0, 0 -- default

this is not necessary as the Stroke dataclass has a default_factory set which initialises object.color to this already.

[hlprasu]

@hvraven I think we may need to support the case when color is not defined at all. Something like the change below. I have checked this a .kicad_sym file updated by KiCad 7.0.0. Does this seem fine?

diff --git a/src/kiutils/items/common.py b/src/kiutils/items/common.py
index bca15f8..5384ed9 100644
--- a/src/kiutils/items/common.py
+++ b/src/kiutils/items/common.py
@@ -233,7 +233,7 @@ class Stroke():
         Returns:
             - Stroke: Object of the class initialized with the given S-Expression
         """
-        if not isinstance(exp, list) or len(exp) != 4:
+        if not isinstance(exp, list) or len(exp) < 3:

I think the len(exp) > 4 should be covered as well to exclude extra arguments.

Yes, makes sense. So, this should be if not isinstance(exp, list) or len(exp) < 3 or len(exp) > 4:?

             raise Exception("Expression does not have the correct type")

         if exp[0] != 'stroke':
@@ -246,6 +246,9 @@ class Stroke():
             if item[0] == 'width': object.width = item[1]
             if item[0] == 'type':  object.type = item[1]
             if item[0] == 'color': object.color = ColorRGBA.from_sexpr(item)
+        
+        if len(exp) == 3:
+            object.color = ColorRGBA() # 0, 0, 0, 0 -- default

this is not necessary as the Stroke dataclass has a default_factory set which initialises object.color to this already.

Understood.

[hvraven]

Yes, makes sense. So, this should be if not isinstance(exp, list) or len(exp) < 3 or len(exp) > 4:?

That will cover all the cases, yes. Unless kicad suddenly decides to make more fields optional, but as the optional color isn't documented either I hope this will not happen to soon.

[hlprasu]

@hvraven Agreed. Kindly go ahead with this change in your pull request.

[mvnmgrx]

Why is it a problem if it gets populated? If it gets populated with the same default value it will not change anything and will be read by kicad the same.

Adding a token that was not their prior to parsing is almost always unexpected behavior (see issue #62 for example). We shouldn't just add tokens if we don't have to.

I tested omitting the Color token in KiCad 6, it works just fine and is added back to the part after saving. So i would say that we make the Color token optional.

I also notices that we initialize a Stroke with type = 'dash', but KiCad uses the type default if no Stroke token was given before. So i would propose these two changes:

@dataclass
class Stroke():
    """ ... """

    type: str = "default"
    """ ... """

    color: Optional[ColorRGBA] = None
    """ ... """

This way no tokens are added, existing tokens parse just fine, KiCad 6 does not care about a potentially missing Color token and its up-to-date with the newest KiCad documentation. But happy to hear other thoughts as well.

@hvraven Would you mind bringing those changes in? I'll be happy to merge your PR afterwards.

[mvnmgrx]

Adding some tests + the small change above in a different PR. Thanks for your contribution!