pen tool commit for each node for more granular undo

[Mental-users]

The pen tool has a different style of interaction to the pencil and
calligraphy tools; for the pen tool, creating a single curve is composed of
many discrete operations.

This means it should should commit an undo step for each node, rather than
waiting until the very end, so that undo and redo work as expected.
Currently, undoing simply unrecoverably destroys the entire path being
drawn, rather than undoing the previous node addition.

The functionality of backspace is halfway there, but segments deleted that
way can't be un-deleted, so I really think we need to do this right with
undo/redo somehow.

[jimmac (jimmac)]

I didn't know about the backspace, but still I've nuked a complex path on many ocasions because I'm so used to the undo shortcut. This is a destructive action as you cannot redo to get your path back. Please bump the priority on this one. It is a destructive behavior, a bug not an enhancement wish.

[Jeremiah Darais (jdarais)]

I am starting to use Inkscape more after finding Illustrator frustrating to use for some things. One thing that gets me a lot in Inkscape, though, is this very issue. It looks like it hasn't gotten much attention recently, (judging by date of last comment.) Is this on anyone's radar?

[Krzysztof Kosinski (tweenk)]

This could be more easily solved by effectively routing a Ctrl+Z keypress to Backspace in the pen tool.

[Paweł Wawruch (pawlo)]

My proposition of fix.
The diff is from inkscape-0.48.
Tested on ubuntu 13.10

[Krzysztof Kosinski (tweenk)]

Patch from comment #4 will not work if undo is mapped to a different key combination in the Keyboard Shortcuts dialog.

[Krzysztof Kosinski (tweenk)]

A better fix would be to add a special case in sp_undo (file selection-chemistry.cpp) when the pen tool is active.

[Jeremiah Darais (jdarais)]

It's been a while, and I thought I'd take another look at this. I'm new to the code, so I could be wrong, but it looks like it would be difficult for sp_undo to get access to the pen tool object. At that point there would also need to be a publicly exposed function it could call to remove the last point that was placed. Would it be reasonable to take Pawel's approach, but also account for te shortcut mapping? (See patch.)

[jazzynico (jazzynico)]

Tested on Windows XP, Inkscape trunk rev. 13791, with the patch from comment #7.

Undo works as expected, but redo is not implemented (I mention it because it's part of the original report).
Additionally, would it make sense to add the action in the undo history?

[Jeremiah Darais (jdarais)]

I agree it would be really nice to have redo working. From what I can tell, the undo history is driven by changes to the underlying document object, and currently, a curve created by the pen tool isn't added to the document until it is completed. In order to properly integrate with undo history, we'd probably have to re-write portions of the tool to edit the document as the user adds individual nodes, and clean up if the user cancels the tool. I think it's worth looking into, but that fix might be a bit more involved.

Let me know if you'd like to use the patch I posted as a short-term fix. With it, at least the user is spared a moment of frustration when the entire curve is lost on ctrl-z rather than just the last anchor as expected. I don't mind investigating a solution with working undo and redo.

[jazzynico (jazzynico)]

> Let me know if you'd like to use the patch I posted as a short-term fix.

IMHO it should be committed to the 0.92 branch so that it could be widely tested and validated. I'm going to test it again on Crunchbang Waldorf and then commit if there's no obvious regression.

[jazzynico (jazzynico)]

@Jeremiah, do you think a full undo/redo feature would need a completely different solution (implying that your patch would have to be removed first when working on it)?
I'm still hesitating to commit what could be a temporary workaround. At least we should add some comments in the code to let developer know its status.

Could other users or developers give an opinion on that report?

[Jeremiah Darais (jdarais)]

In order to integrate fully with undo/redo, you wouldn't need to remove any code from this fix that wouldn't otherwise need to be removed. In other words, this doesn't make implementation of the real fix any more complicated. This fix is mostly made up of refactoring that allows the existing code that removes the last node in the path to be reused in multiple places.

I would also be fine adding more comments to give more information on the status of the fix.

[jazzynico (jazzynico)]

Partial fix committed in the trunk revision 13848.
Thanks Jeremiah.

[jazzynico (jazzynico)]

New report added for the redo feature: bug #1630461 "Pen tool, granular redo".