Add back segment-by-segment undo while drawing with the Pen tool

[Keavon]

Good first issue: this is likely a medium-difficulty item suitable for savvy new contributors who have likely already done a few other PRs, but probably isn't best as a first PR or for those with limited programming experience

---

We used to be able to draw with the Pen tool, and as you kept drawing more segments, you could use CtrlZ to undo each individual segment one at a time. This functionality was lost during a refactor PR from @adamgerhant as part of the limitations imposed by that refactor. This functionality will now need to be reimplemented within the current state of the code, and can be referenced in the build link below for its prior functionality.

It was removed between these two builds, which would be a helpful date range if it becomes necessary to bisect which commit this regression was introduced in.

• 2024-08-27: Pen tool undo worked as desired in this production deploy: https://110d2ce1.graphite-editor.pages.dev/
• 2024-09-15: The regression happened no later than this date, since that's when I wrote a comment about its removal

Here's a video showing the prior behavior from that build link when actively drawing segments with the Pen tool:

capture_13_.mp4

The current behavior in master just ignores any attempts to undo while actively drawing segments.

[ayushpatil2122]

Hey i want to work on this issue

[vrrashkov]

Hey I am also checking this issue, here is what I found.

document_message_handler never executes further than this, because the Transaction is still in progress (Started).

It was introduced here

9adc640#diff-d0256cd9c455b2a08acd3c82b1646943bf7574fe77cdd0e85c2543c65ef7e2ddL1003-R1044

Graphite/editor/src/messages/portfolio/document/document_message_handler.rs

Lines 1042 to 1045 in 9adc640

	DocumentMessage::Undo => {
	if self.network_interface.transaction_status() != TransactionStatus::Finished {
	return;
	}

How should this be handled, in the current state, is there a reason for this to be checking if the transaction is Finished? If not, removing it will fix the issue.

[Keavon]

@adamgerhant may be able to answer that for you.

[Keavon]

@ayushpatil2122 @vrrashkov sounds good, thanks for both attempting this, it'll be nice to have fixed. Both of you are welcome and encouraged to continue on your own attempts and we'll land whichever offers the more suitable solution first (a bit of friendly competition to keep you both motivated), and as redundancy in case one of you doesn't figure it out (which is pretty common among new contributors). Good luck to both of you!

[adamgerhant]

This is a remnant of an bug fix for #559. (relevant conversation). The issue is that if you can undo the creation of the layer you are currently working on then your next point will reference a layer that doesn't exist, which crashes. The ideal solution is a bit more nuanced, since it should only be possible to undo up to when the layer was created, but not past that. This could mean two different types of transactions, where the points added would be "sub-transactions", which can be undone while the entire drawing transaction is occurring.

I thought it was easiest to prevent the whole issue by just not allowing undo's while a transaction is happening, which I still think should be the case.

[Keavon]

@adamgerhant the desired behavior would allow for undoing past the point when the layer was initially created, all the way back until the creation of the document. But upon transitioning from segment-by-segment undoing to before the layer was created, the Pen tool would need to update its status so that it doesn't believe it's currently editing any layers. (That's the case whenever you decide to switch to the Pen tool without having any particular layers selected and without beginning to draw yet.)

[adamgerhant]

Sure, that is another solution. But that is significantly more fragile since then it is the responsibility of all tools or other features that could possibly reference an invalid state to perform that check/action on every undo. Having a dedicated "sub-transaction" would enforce this and prevent a tool ever being in an invalid state, although the limitation being that you cannot undo all the way back to document creation while a transaction is occurring. I think restricting undoing to be limited to the context of your current operation is a reasonable restriction and greatly improves stability/prevents crashes.

[Keavon]

@adamgerhant I think I disagree about which approach is more fragile and simple. In my view, it's very definitely the responsibility of the tools to make sure they are working with valid state even if the document teleports itself into a totally different configuration/state like by loading a different point in history out from under the user. If the tools aren't robust to that happening, that's the fault of the tools. Basically, I don't think it's too much to ask of the Pen tool for it to guarantee its own robustness if the layer it's working on suddenly gets deleted and its layer reference stops existing. It should know how to handle that situation and switch itself back into its ready state.

I get where you're coming from with your idea of "sub-transactions" but I'm not convinced the current situation (or any other potential situation we haven't thought of yet) warrants the complexity involved in maintaining that concept which augments the current simple history system with additional facets of complexity.

From a product design and UX angle, I'm also not okay to compromise (long-term, at least) on the inconsistency of the undo history confusingly hitting a "dead end" until the user switches away from the Pen tool and then back to the Pen tool again so they can go further back in history. No tool should block undo from working, and it shouldn't ever be needed to switch away from a tool and go right back to it again to sidestep such a history blockage. That would be confusing and painful to users.

With these arguments in mind, would you be able to offer any hints for implementors of this issue about how to solve the root of the problem that the Pen tool encounters regarding the crash? Thanks :)

[vrrashkov]

Hm so from what I understand while using the Pen Tool if we undo everything plus the layer we are working on it should produce some sort of an issue/crash? Here is a video me trying to reproduce this having removed the lines where it was preventing the Undo before Finished.

Also I see that #559 is mentioning the "Freehand tool" but this tool does not have the option to "Undo while drawing" so I could not reproduce the issue with that tool as well. How to reproduce the exact issue for that crash?

Screen.Recording.2024-12-24.at.9.07.50.mov

[Keavon]

Also I see that #559 is mentioning the "Freehand tool" but this tool does not have the option to "Undo while drawing" so I could not reproduce the issue with that tool as well. How to reproduce the exact issue for that crash?

That should be unrelated because it should never be valid to undo while dragging (i.e. the user's mouse button is down and hasn't been released yet).

Conversely, the Pen tool has the concept of a drawing state where the user's mouse is not dragging but there are intermediate steps possible within the tool, such as drawing additional segments of the path.

[adamgerhant]

I think your take is reasonable. In that case all that is necessary is to remove the early return, and then remember to add special logic for tools/features when undoing to prevent the invalid state (which is already done for the Pen tool)

[vrrashkov]

Should this be separate issues, this one only removing the early return and for missing undo handlings separate issue as well? Also are there any particular tools currently that would be causing problems?

[Keavon]

@vrrashkov I don't think any parts described in this issue need to be split out into a separate issue.

[vrrashkov]

Okay, just if you can let me know which tools/features need to have the Undo added to them while that tool is not Finished, because not really sure which ones need that right now.

[Keavon]

Just the Pen tool.

[vrrashkov]

But the pen tool works from what i see, and if you check the video as well not sure what to fix there really

[Keavon]

Do you mean it works with your fix applied?

[vrrashkov]

No i mean i just removed the early return statement and it works, i dont see a crash. As you can see I can undo and even undo the layer I am working on and it does not crash. And based on what @adamgerhant mentioned (which is already done for the Pen tool) I think this is already implemented ?

[adamgerhant]

The pen tool already has the custom logic when undoing to deselect the layer. In general it will be important to test cases when you have created something, start a transaction, undo to delete that thing, then finish the transaction. For example maybe creating a node, dragging it, undoing so that the node is deleted, then releasing the drag might reference the non existent node and crash. We essentially just introduced memory unsafety into the editor by allowing mutable references to be deleted while they are used/borrowed.

[Keavon]

Ok @vrrashkov, it sounds like the PR will just be the removal of the early return paired with some extensive QA testing to find ways to break it, such as by the means Adam pointed out.