Make the Fill tool's fill click operation cancellable

[milan-sedivy]

This is the last tool that needed a cancellable transaction.

It is implemented in a way that it introduces a "Fake drag" the reason for this is simple, emitting DocumentMessage::AbortTransaction currently works as an undo step. Thus if the user would not perform an operation before trying to cancel the transaction it would result in undoing his current history.

This could and should be reimplemented once a Document can be in different states such as "Transaction In Progress"

Closes #1657

[github-actions]

📦 Build Complete for 3053c3a
https://24122b30.graphite.pages.dev

[Keavon]

There are a few bugs I found:

• If I click and drag outside the shape then release, I get stuck in the mousedown state (notice the hints bar) and neither left clicking nor right clicking nor Escape outside the shape fix the stuck state (I have to left click to apply, or right click to abort, inside the shape)

• If I hold down the left mouse button, then hit CtrlZ, then abort with Rmb, this causes it to go back to a previous history step (it undoes the creation of a layer I made immediately before this, for example).

  If this is true for all the abortable/cancellable tools you've worked on, let's skip fixing it in this specific PR.

[milan-sedivy]

There are a few bugs I found:

• If I click and drag outside the shape then release, I get stuck in the mousedown state (notice the hints bar) and neither left clicking nor right clicking nor Escape outside the shape fix the stuck state (I have to left click to apply, or right click to abort, inside the shape)
• If I hold down the left mouse button, then hit CtrlZ, then abort with Rmb, this causes it to go back to a previous history step (it undoes the creation of a layer I made immediately before this, for example).
  If this is true for all the abortable/cancellable tools you've worked on, let's skip fixing it in this specific PR.

The CTRL+Z+ESC/RMB will probably be true for all tools but I will look into an easy hotfix (before the whole transaction system is refactored so that it doesn't rely on undos).

As for the other behavior I will check and see what I can do to fix it.

[milan-sedivy]

I noticed a new TODO (or maybe old?) i.e.

// TODO: Use a match statement here instead of if-else

In that given case i think it's preferable to keep it as an if-else rather than match, because of the fact that match would just add another arm and I think that down the line it might lead to uncaught issues.

[milan-sedivy]

@Keavon

• If I hold down the left mouse button, then hit CtrlZ, then abort with Rmb, this causes it to go back to a previous history step (it undoes the creation of a layer I made immediately before this, for example).
  If this is true for all the abortable/cancellable tools you've worked on, let's skip fixing it in this specific PR.

As suspected this is something that is affecting all tools that either call Undo or AbortTransaction. (Not just the tool I worked on)

I was hoping for a quick hotfix through a "hack" in default_mapping.rs but unfortunately to implement the hack I would have to refactor that file. This definitely needs a separate issue/PR
So currently there are two approaches I suggest:

• (the hack) Fix keymappings so that more modifiers => bigger priority when chosing an action_dispatch this would allow us to make a DocumentMessage::Noop and send that when you use a forbidden combination (CTRL + Z + LMB)
• a better approach would be to refactor StartTransaction/CommitTransaction/AbortTransaction so that they actually do what the names suggest. That would allow us to move from AbortTransaction using undo under the hood.

[Keavon]

Ok, thanks for investigating. Can you move your findings about the undo bug to another issue which can be home for that future PR?

The to-do I added is a suggestion to better model the invariants. Else doesn't imply filling with the secondary color. If there's something other than filling with primary or secondary color, it'll still enter the else arm. A match statement better models the problem and is this cleaner.

[milan-sedivy]

Ok, thanks for investigating. Can you move your findings about the undo bug to another issue which can be home for that future PR?

The to-do I added is a suggestion to better model the invariants. Else doesn't imply filling with the secondary color. If there's something other than filling with primary or secondary color, it'll still enter the else arm. A match statement better models the problem and is this cleaner.

Hmm good point didn't think about that.

As of the PR it's already done (with an issue)

[Keavon]

!build

[github-actions]

📦 Build Complete for e85f99f
https://a9db192f.graphite.pages.dev