removeDefaultAttributeValue: Special-case order attribute

[nthykier]

Scour tried to handle "order" attribute as a SVGLength. However, the "order" attribute can consist of two integers according to the SVG 1.1 Specification and SVGLength is not designed to handle that.

With this change, we now pretend that "order" is a string, which side steps this issue.

Closes: #189 Signed-off-by: Niels Thykier niels@thykier.net

[nthykier]

Note we will still have an issue in SVGLength, where it sometimes do not set the "units" attribute. See the "FIXME" I have injected into SVGLength's __init__ below.

            if unitBegin != 0:
                unitMatch = unit.search(str, unitBegin)
                if unitMatch is not None:
                    self.units = Unit.get(unitMatch.group(0))
                # FIXME: else needed here to set self.units
            # invalid
            else:
                # TODO: this needs to set the default for the given attribute (how?)
                self.value = 0
                self.units = Unit.INVALID

[codecov-io]

Codecov Report

Merging #192 into master will increase coverage by 0.47%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   93.08%   93.55%   +0.47%     
==========================================
  Files           5        5              
  Lines        2140     2158      +18     
==========================================
+ Hits         1992     2019      +27     
+ Misses        148      139       -9

Impacted Files	Coverage Δ
scour/scour.py	93.98% <ø> (+0.54%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8ddb7d8...5d579f8. Read the comment docs.

[oberstet]

@Ede123 this also looks good to me .. however, I lack the deep svg knowledge.

@nthykier so the new unit test (feConvolveMatrix order="3 1") would fail on the existing code, right?

[Ede123]

Seeing that there are more attributes than just "order" which allow a "<number-optional-number>" value I'd say it would be more sensible to either a) disable scouring for them for now b) implement a proper solution

A proper solution would probably consist of two code changes:

• a new step in "scourString()"
  (we'd need to check if "first number == second number" in which case the second number could be removed)
• a change in "removeDefaultAttributeValue()" to account for lists of numbers One possible implementation could be to pass the DefaultAttribute's value as a list in this case (i.e. [3]), so we can add a new case for "lists of numbers" that need to be split first (we could likely bail from this function as soon as splitting results in more than a single token)

That said I have no idea how often any of these attributes are used (and if they are used how often they are used as a list) - probably not very often, otherwise we'd have seen reports already. If they are as uncommon as I assume checking them as strings might just be be fine after all. but at the very least the workaround should be implemented for all affected attributes in this case)

[nthykier]

@oberstet Indeed, that test case fails on the current master branch.

@Ede123 I noticed order and kernelUnitLength as number-optional-number. And then there is kernelMatrix which is <list of numbers> (with length equal to orderX * orderY, i.e. the value of the order attribute). On the positive size, kernelMatrix does not appear to have a default value, so we do not have to deal with it from a "DefaultAttribute" perspective.

Did you notice any other of such attributes?

[Ede123]

No, filters (except feGaussianBlur) are used rarely in general in my experience.

I'm fine with the string test, but let's implement it for all attributes that apply. Dealing with cleaning those is an enhancement anyway and can be dealt with separately.

[nthykier]

@Ede123 As far as I can tell from the code, we only tried to analyse the order attribute. I have updated the test case to include the other 2 attributes and the current patch appears to ignore them (i.e. does not crash on them).

[nthykier]

And immediately after, I noticed stdDeviation - fixed in 8a2892b with an "don't crash" test case.

[nthykier]

Hey @Ede123

Did you have a look at my comments plus latest update to this pull request? Let me know if there is anything you want to have improved.

[Ede123]

I found two more attributes which were not yet handled and added them to this PR. Thanks for the fix!