Warning: Failed undo group after skew

Bug #1227193 reported by Martin Owens on 2013-09-18
16
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Inkscape
Medium
Tomasz Boczkowski

Bug Description

Using inkscape trunk r12526 on Ubuntu 13.04

Create two objects
Group them
Apply skew
Undo skew (works)
Undo group (fails)

Warning on terminal:

** (lt-inkscape:8585): WARNING **: Incomplete undo transaction:

** (lt-inkscape:8585): WARNING **: Event: Set attribute d to "m 400,502.3622 c 0,60.7513 -47.3299,110 -105.7143,110 -58.3844,0 -105.7143,-49.2487 -105.7143,-110 0,-60.7513 47.3299,-110 105.7143,-110 58.3844,0 105.7143,49.2487 105.7143,110 z" on #<Element:0x0xc9e9f70>

** (lt-inkscape:8585): WARNING **: Event: Set attribute d to "m 514.2857,403.7907 c 0,45.7607 -44.1319,82.8572 -98.5714,82.8572 -54.4395,0 -98.5714,-37.0965 -98.5714,-82.8572 0,-45.7607 44.1319,-82.8571 98.5714,-82.8571 54.4395,0 98.5714,37.0964 98.5714,82.8571 z" on #<Element:0x0xc9e9ef0>

Pressing undo again will ungroup them, so this looks like an extra action has been posted into the tracking system which can't be undone (or never was done in the first place)

Martin Owens (doctormo) wrote :

Addendum: I created two circles with fill. See attached.

su_v (suv-lp) on 2013-09-18
tags: added: undo
su_v (suv-lp) wrote :

Reproduced with Inkscape 0.47, 0.48.x and current trunk (r12543) on OS X 10.7.5.

- Not limited to 'Skew' transformation (same e.g. with 'Scale')
- After undoing group twice, the list in the 'Undo History' dialog is broken:
  the 'Skew' entry in the undo list loses it name, and later entries in the list are blanked (though apparently still functional with 'Shift+Ctrl+Z')
- Does not happen e.g. with a group of two rectangles or regular paths

Changed in inkscape:
importance: Undecided → Medium
status: New → Triaged
Changed in inkscape:
assignee: nobody → Tomasz Boczkowski (penginsbacon)

I have noticed that the bug affects only ellipses.

After Inkscape::DocumentUndo::undo reverted "d" attribute in svg:path and started new transaction, an idle handler is called. This handler calls SPDocument::_updateDocument. It results in execution of SPGenericEllipse::update_patheffect. The problem is that "d" attribute calculated here is different from original one and setting this ends up as uncommited change, later displayed as warning.

If SPGenericEllipse::update_patheffect and SPGenericEllipse::write set the same "d" attribute value, this bug would not appear. I'll write a post about this on mailing list.

Another possible solution to this bug is to call SPDocument::_updateDocument in Inkscape::DocumentUndo::undo before starting new transaction. That is what attached patch does.

Attached patch also secures the undo stack against other possible document changes introduced by _updateDocument.

su_v (suv-lp) wrote :

Patch tested successfully with Inkscape 0.48+devel r12572 on OS X 10.7.5

@Tomasz Boczkowski - thx a lot for looking into this bug! Do you think it would be feasible to backport a fix to the stable release branch, or does that depend on which of the two solutions you describe is chosen?

Changed in inkscape:
milestone: none → 0.49
status: Triaged → In Progress
Martin Owens (doctormo) wrote :

Tomasz, are you sure the attached patch secures the undo stack? I can't see any new checks in the diff so is the updateDocument already protected and calling it just makes those work?

Sending a patch for inkscape stable (solution 2). I think solution 1 might be too radical for stable release.

What I ment by saing about securing the undo stack is that any possible changes to document introduced by _updateDocument would not be recorded in the stack. As far as I'm concerned, instructions between finish_incomplete_transaction and sp_repr_begin_transaction are outside of transaction.

_updateDocument can introduce partial transaction, when it's called twice. First time emitModified() can change XML tree and record those changes in SimpleDocument::_log_builder. Second time, sp_document_set_undo_sensitive saves those changes as partial transaction.

Cojnel (cojnel) wrote :

Maybe this is related, at least the first warning message is the same.

1. draw a rect (not specific to a rect, it can even be deleted, the undo button just needs to be enabled)
2. start dragging out a guide from the ruler (don't drop it on the page)
3. dragging it on or past the ruler to delete it
4. press undo (any undo method works, Edit menu, toolbar button and ctrl + z)
5. the terminal outputs:
** (inkscape:8556): WARNING **: Incomplete undo transaction:
** (inkscape:8556): WARNING **: Event: Set attribute showguides to "true" on #<Element:0x0x91bbef0>
** (inkscape:8556): WARNING **: Event: Set attribute inkscape:guide-bbox to "true" on #<Element:0x0x91bbef0>

Tested with r12931 and 0.48.4 r9939,
on linux mint 15 32bit

su_v (suv-lp) wrote :

@Cojnel - please file a separate report (the message "Incomplete undo transaction:" is far to generic to link every occurrence to this specific report).

Liam P. White (liampwhite) wrote :

@Tomasz

Fix committed in lp:inkscape, r13496. Thank you very much! Other (related) issueswere driving me nuts.

Changed in inkscape:
status: In Progress → Fix Committed
Liam P. White (liampwhite) wrote :

Reverted r13496 due to follow-up report about markers. Will look at this later.

Changed in inkscape:
status: Fix Committed → In Progress
Martin Owens (doctormo) wrote :

Should only be "In Progress" if you're looking into it now. Leaving it open for other's to look into with Confirmed.

Changed in inkscape:
status: In Progress → Confirmed

The problem with the follow-up about markers was caused by marker UI code.

Updating the document in the DocumentUndo::undo method seems to be the right thing. However, there's one thing missing in the original solution.

Changes introduced by SPDocument::_updateDocument method called in DocumentUndo::undo implementation should somehow be registered in the undo stack. It is particularly important when operations stored in the undo stack affect the elements or attributes that are also changed in document update process. The simplest solution is to coalesce the changes introduced by SPDocument::_updateDocument call with the operation at the top of the undo stack.

This patch should work. After undo operation is done, a document update is performed and possible changes are coalesced to last operation on undo stack.

To avoid crash the patch should be applied together with patch to bug #1357805 (comment 5).

su_v (suv-lp) on 2014-11-06
Changed in inkscape:
milestone: 0.91 → 0.91.1
status: Confirmed → Triaged
Liam P. White (liampwhite) wrote :

Fix committed in trunk r14199

Changed in inkscape:
status: Triaged → Fix Committed
su_v (suv-lp) on 2015-06-10
Changed in inkscape:
milestone: 0.91.1 → 0.92
su_v (suv-lp) on 2015-06-12
tags: added: backport-proposed
su_v (suv-lp) wrote :

Fix backported to 0.91.x in rev 13798.

Changed in inkscape:
milestone: 0.92 → 0.91.1
tags: removed: backport-proposed
jazzynico (jazzynico) on 2017-01-22
Changed in inkscape:
milestone: 0.91.1 → 0.92
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers