Effects dirty document even when they do nothing

Bug #166697 reported by Rwst
4
Affects Status Importance Assigned to Milestone
Inkscape
Fix Released
Low
jazzynico

Bug Description

Starting up, then clicking on Effects/Draw Handles will change the default
document, so the confirmation dialog unnecessarily pops up on quit. diff
shows (apart from different filenames):

--- d1.svg 2005-07-13 11:23:00.119986064 +0200
+++ d2.svg 2005-07-13 11:22:31.037407288 +0200
@@ -45,7 +45,7 @@
     </rdf:RDF>
   </metadata>
   <g
- id="layer1"
      inkscape:groupmode="layer"
+ id="layer1"
      inkscape:label="Ebene 1" />
 </svg>

Revision history for this message
Aaron C Spike (acspike) wrote :

That is because I'm parsing the SVG and reserializing it.
Evidently attribute order isn't preserved.

Revision history for this message
Rwst (rwst) wrote :

At first, I thought this was a duplicate of

https://sourceforge.net/tracker/index.php?func=detail&aid=1241132&group_id=93438&atid=604306

but the fix of that bug will not affect this one here.
But maybe a fix would be similar?

Changed in inkscape:
status: New → Confirmed
tags: added: extensions-plugins
removed: extensions
Revision history for this message
su_v (suv-lp) wrote :

In the launchpad bug tracker, comment #2 refers to
Bug #166716 “Order of attributes is inverted on each save”:
<https://bugs.launchpad.net/inkscape/+bug/166716>

Revision history for this message
Craig Marshall (craig9-deactivatedaccount) wrote :

This was happening for me with the handles extension too (this extension draws handles where there are curves in the document, and does nothing if not). If you run the extension and there are no curves to draw handles for, inkscape still reads back in a (slightly) different document, and this triggers a save-requirement/dirty-flag inside inkscape.

A way around this (which I've implemented in the handles.py as a demonstration) is to set a simple dirty flag within the extension itself, and if there are no changes to the document, make a call to sys.exit(0), which bypasses the self.output() call in inkex.effect(), which rewrites the file. The 0 denotes a success exit from the program.

This seems to me a reasonable and simple workaround that could easily be done for each extension that can feasibly return an unchanged SVG document.

A better long-term solution would be to compare the input and output files for every extension within inkex.py (or in the inkscape core) and then set output to Null if there are no changes. Of course, the comparison is more difficult than running diff or testing if doc1 == doc2. Also, I couldn't get xmldiff to work here... Hence the simpler solution for now.

Apologies for the full file attachment, I don't yet have a working diff utility here.

Revision history for this message
Craig Marshall (craig9-deactivatedaccount) wrote :

This patch works for me, and I am happy/ready to commit it, if there are no problems?

Revision history for this message
Craig Marshall (craig9-deactivatedaccount) wrote :
Revision history for this message
su_v (suv-lp) wrote :

As commented on irc:
«about the handles - iirc I tested it and it worked [1]. but as you comment yourself - that's a workaround, and the proper fix might be in inkex.py to prevent dirtying the document if nothing is changed by the extension. Your fix masks the underlying issue. Maybe get some other opinions about it?»

[1] fix tested and confirmed with Inkscape 0.48 and 0.48+devel on OS X 10.5.8

jazzynico (jazzynico)
Changed in inkscape:
status: Confirmed → Triaged
jazzynico (jazzynico)
Changed in inkscape:
assignee: nobody → JazzyNico (jazzynico)
milestone: none → 0.49
status: Triaged → In Progress
Revision history for this message
jazzynico (jazzynico) wrote :

Patch tested on Windows XP, Inkscape trunk revision 11992.

The idea is to keep a separate copy of the document tree, and then compare a serialized versions of the original and resulting documents before updating the canvas.

Tested with Visualize Path>Draw Handles, and some from the Color and Render menus, with and without selection.

Revision history for this message
su_v (suv-lp) wrote :

@JazzyNico - the issue mentioned on irc with the patch seems to originate from two unrelated issues: one was a typo in the custom extension (and not explicitly returning the original r,g,b values if all sliders are '0'), the other one was a badly chosen test case - or something which is still kind of a mystery to me:

In the attached file (reduced test case), the path data 'd' of the circle segment changes in Inkscape apparently already on load (from 'a' to 'c' bezier path commands), as can be verified in the XML Editor. The same path data of a regular path, with the same transform attribute remains unchanged. The change of the segment path data on load (?) does not dirty the document though, and apparently isn't written back to file either on save (unless the shape has been modified (?)). However, when running an extension on this file which supposedly doesn't dirty the file, your patch "fails" on the first run (apparently the path data now differs). After undoing that change from the first run of the extension (Edit > Undo), subsequent runs of the same extension with the same settings no longer dirty the document…

Revision history for this message
jazzynico (jazzynico) wrote :

> In the attached file (reduced test case), the path data 'd' of the circle segment changes in Inkscape apparently already on load

Confirmed, but not directly related to the extension. The "undoing" effect also works when directly modifying the drawing:
1. Open the file from comment #10 in a text editor. Path4295 has d="m -64,293.36218 a 98,98 0 1 1 -196,0 L -162,293.36218 Z".
2. Open the same file in Inkscape. The path changes automatically and has now d="m -64,293.36218 c 0,54.12391 -43.87609,98 -98,98 -54.12391,0 -98,-43.87609 -98,-98 0,0 0,0 0,0 L -162,293.36218 Z".
3. Modify the drawing (create a rectangle). Path4295 doesn't change.
4. Undo. Path4295 reverts to the d value it has in the text file.

There must be some kind of synchronization missing when initializing the document and what we see (the modified d value) is not what we get (the original ones) if we pass the document to the extensions system.
That said, since that problem is not related to the extensions (and can't be fixed in inkex.py anyway), I suggest we open a new specific report and apply the patch from comment #9 (unless we find performance issues or regressions of course).

jazzynico (jazzynico)
tags: added: svg
Revision history for this message
jazzynico (jazzynico) wrote :

Fix committed in the trunk, revision 12236.

Changed in inkscape:
status: In Progress → Fix Committed
tags: added: backport-proposed
Revision history for this message
su_v (suv-lp) wrote :

Setting milestone to 0.48.5 so that we don't forget to backport.

Changed in inkscape:
status: Fix Committed → In Progress
milestone: 0.49 → 0.48.5
Revision history for this message
su_v (suv-lp) wrote :

Committed to lp:inkscape/0.48.x in revision 9957.

Changed in inkscape:
status: In Progress → Fix Committed
Kris (kris-degussem)
tags: removed: backport-proposed
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.

Other bug subscribers

Remote bug watches

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