Scour

scour does not clean comments if file starts with a comment

Bug #833666 reported by Hungerburg
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Fix Released
Low
jazzynico
Inkscape 0.91 "0.91"
Scour
Fix Released
Medium
Unassigned

Bug Description

It is quite usual that creators put a comment on the top of the file with their name.

<?xml version="1.0" encoding="UTF-8"?>
<!-- generated by Me and My Uncle -->
<svg …>

This will keep scour from stripping comments in the file, as it will not descend the child nodes. Below patch fixes that, and keeps the offending line - what is the best choice many times.

@@ -2557,6 +2560,10 @@
    if isinstance(element, xml.dom.minidom.Comment):
     numCommentBytes += len(element.data)
     element.documentElement.removeChild(subelement)
+ elif isinstance(subelement, xml.dom.minidom.Comment):
+ # first element in document may be a comment
+ # just ignore it then
+ pass
    else:
     removeComments(subelement)
  elif isinstance(element, xml.dom.minidom.Comment):

file numbers may not match trunk, as my version includes the polyline patch of mine.

Revision history for this message
Hungerburg (pch-myzel) wrote :
Revision history for this message
Louis Simard (louis-simard-deactivatedaccount) wrote :

It's also common that vector editors put a comment at the start of files with their name, which sometimes people want to remove. It would be better as an option.

Revision history for this message
Louis Simard (louis-simard-deactivatedaccount) wrote :

This bug is now only about the comment patch. I'll create another bug for the polyline negative coordinate bug.

Revision history for this message
Louis Simard (louis-simard-deactivatedaccount) wrote :

Or not. Please see bug 804238 for the polyline negative coordinate bug, and post a patch to fix only that bug on its bug thread.

Revision history for this message
Hungerburg (pch-myzel) wrote :

Louis, thank you for the attention. I posted the patch in the other thread. The one above does not strip the leading comment line. It just prevents the leading comment line from disabling the comment stripping altogether. Please use as a starting point for you own code. You may yank the first hunk of this one and it should still apply correctly. Regards

Revision history for this message
Louis Simard (louis-simard-deactivatedaccount) wrote :

Aha. I completely misread your bug description. I now see that it's not a feature request that should be an option, it's a bug with the code.

Revision history for this message
Louis Simard (louis-simard-deactivatedaccount) wrote :

Fixed in Scour revision 216. Thanks for your patch!

The 'element' versus 'subelement' variables in previous Scour trunk code were a mismatch. It should have really been 'subelement'.

Changed in scour:
importance: Undecided → Medium
status: New → Fix Committed
Revision history for this message
Hungerburg (pch-myzel) wrote :

Louis, the polylines patch is working here, but the clean comments change has no effect. Please see the new patch that will fix the comments scouring. Node.childNodes is a live view of the DOM, so removing childs messes with pythons counters.

Revision history for this message
Louis Simard (louis-simard-deactivatedaccount) wrote :

Right, that was an oversight on my part.

When I iterate over a collection that can be modified but plan on removing elements, I usually start at the ending index (size - 1) and count down to 0, because copies are rather inefficient. That preserves indices in the array. Can that be done with the minidom package?

Revision history for this message
Kris (kris-degussem) wrote :

Seems the patch in comment 8 is not applied yet ...
Afterwards, can scour be updated without issues in Inkscape (to close the bug there)

Changed in scour:
status: Fix Committed → Confirmed
Revision history for this message
Kris (kris-degussem) wrote :

With the current code in Inkscape and the following comments just after the xml tag and in front of the svg tag in an svg-file:
   <!-- blah blah blah -->
   <!-- Created with Inkscape (http://www.inkscape.org/) -->
   <!-- blah blah blah -->

keeps the middle comment in the resulting svg-file. With the patch in comment 8 applied, all goes well.

jazzynico (jazzynico)
Changed in inkscape:
assignee: nobody → JazzyNico (jazzynico)
importance: Undecided → Low
milestone: none → 0.49
status: New → Triaged
Revision history for this message
Louis Simard (louis-simard-deactivatedaccount) wrote :

This bug has been fixed in Scour trunk, revision 220.

Changed in scour:
status: Confirmed → Fix Committed
Revision history for this message
jazzynico (jazzynico) wrote :

Fixed in Inkcape trunk revision 11591.

Changed in inkscape:
status: Triaged → Fix Committed
jazzynico (jazzynico)
Changed in scour:
status: Fix Committed → Fix Released
Changed in inkscape:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.