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