Warning: Failed undo group after skew

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

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

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.

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?

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.

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

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

@Tomasz

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

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

Should only be "In Progress" if you're looking into it now. Leaving it open for other's to look into with 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).

Fix committed in trunk r14199

Fix backported to 0.91.x in rev 13798.