remove more attributes with default values
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Inkscape |
Fix Released
|
Low
|
jazzynico | ||
Scour |
Fix Released
|
Medium
|
Unassigned | ||
scour (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned |
Bug Description
Within removeDefaultAt
There is a problem, though: if an element changes such a value, and a child element sets the value back to the default value, we can’t delete this attribute in the child element, since the default value is now meaningful. Therefore, it is necessary to keep track whether some attribute has been changed somewhere among the ancestors, in which case an attribute pointing to a default value should be left untouched.
I appended a code fragment where I made the necessary changes to the removeDefaultAt
Related branches
Changed in scour: | |
status: | Confirmed → In Progress |
Changed in inkscape: | |
status: | Fix Committed → Fix Released |
That code gives me deja vu for some reason, as if I had read it somewhere else already...
Anyway, on to the comments:
1. Are you in an environment where you can't make unified diffs, or perhaps you don't know how to make them yet? Unified diffs are an excellent way to make patches, where the diff itself shows what was removed and added, yet any contributor only needs to keep the old or new version of a file to apply or revert a patch. On Unix or BSD, you can copy the Bazaar repository version of the file to scour.py.old and issue "diff -u scour.py.old scour.py", after editing scour.py, to make a patch. On Windows you can probably get a tool to make unified diffs too, like a gnuwin32 port of 'diff'.
2. _getStyle and _setStyle could probably be used elsewhere when the code needs to read and write style attributes; well done on making the accessor code into functions.
3. Using str(), float() and convertColor() as canonicalising functions before checking the values is a nice touch, but it may make Scour a bit inefficient. It makes #000 equivalent to black, etc., but the time taken to ensure that both colors/numbers are canonical even after colors and number precision were optimised seems like a bit of a waste.
Overall your code looks sound, but I would hugely prefer having a patch in unified diff format to work with.