Fix undo/redo behavior of ColorPicker and add ability to cancel/confirm color selection

[MajorMcDoom]

Relevant Issues

• Fixes #88607
• Fixes #83642
• Fixes #45186
• Could replace #83786

Changes

This PR fixes a couple of bugs while at the same time introducing some UX changes:

• There will no longer be multiple undos committed for the same color.
• Operations performed in the popup are no longer individually committed to the undo stack. Rather, only one operation is committed when the popup is dismissed.
• If the popup is closed while the HEX code is being edited, it will no longer retain its (possibly invalid) contents when you re-open the popup.
• You are now able to cancel out of the popup without following through with the color selection, instead of having to commit a value you don't want, and then undo it. The ui_cancel action (ESCAPE by default) is used to cancel out. Dismissing the popup through other means (like clicking outside of it) still confirms the color change as before.
• You can now additionally use the ui_accept action (ENTER by default) to dismiss the popup and confirm the color operation.

UX Impact

• The color picker dialog (as in other software) is designed to be a workspace/context in which the user has tools at their disposal to tweak a color as they wish, while previewing the changes. It would be inconsistent with this intent if each color tweak commits an additional undo operation. With this PR, the UX now aligns with the intent.
• The user is now able to either confirm or cancel their operation via keyboard. Granted, users who have grown accustomed to using ui_cancel to confirm a color operation would have to get used to now pressing ui_accept instead, but the old scheme was an oddity and the new scheme is universally known and consistent with the behaviour of all other dialogs.

[MajorMcDoom]

In the long run, it might make sense to add an ability for Popup to set whether it allows ui_accept and ui_cancel to dismiss it. Right now it just uses ui_cancel, but I could definitely see a use case for disabling that, or allowing for both.

For now, the ColorPicker just specifies a subclass of PopupPanel which adds the ui_accept functionality on top.

Also might make sense to eventually add an internal UndoRedo for the popup itself.

[JekSun97]

Perhaps you could solve this problem #88412

[MajorMcDoom]

Noticed that this introduces a bug in a Visual Shader node similar to a previous attempted fix. #44895 I'm going to take a look at this.

[MajorMcDoom]

@aXu-AP @KoBeWi I believe I've found a solution to the big web of issues/PRs surrounding color property changes.

To summarize the situation before:

• EditorProperty and its subclasses provide a single point of access for consumers to know about property changes: property_changed. This was intentional, and abstracts away the UX details of how said property changes are achieved. Consumers only need to know that it has changed, and what it has changed to. We want to keep this setup.
• ColorPicker and ColorPickerButton are UI classes, and as such should be completely removed from concerns of property change propagation. They only provide signals to inform of color picking results, and of the popup dismissal. i.e. UX events.
• EditorPropertyColor was firing property_changed as part of live changes, but this was undesirable, as it generates undo operations.
• EditorPropertyColor is used in multiple places in the editor, with each use case having different UX demands. Specifically, the inspector requires live changes during picking, whereas the shader editor node properties actually suffer from it.

My solution was to give EditorPropertyColor some API to configure its UX preferences. Specifically, set_live_changes_enabled (true by default). The shader editor just has to set this to false.

I've considered other alternatives like editor property hints, or using property_changed + p_changing like it currently does. However:

• You can only have one hint per property, and EditorPropertyColor already has PROPERTY_HINT_COLOR_NO_ALPHA taking up that special role.
• p_changing isn't very well documented or named. It is currently being used for different purposes, and that should be addressed eventually. But the most core use is in EditorInspector, where it affects tree updates. It just also happens to be exposed to the user to respond to UX nuances that sometimes line up with the nuances in tree update requirements. As we can see though, these needs don't always line up. It also requires firing a property_changed signal during live previewing, which always results in an undo operation.

I would recommend we phase out the co-opting of p_changing to convey and interpret UX nuances. It's limiting, and creates inconsistencies. Instead, each type of EditorProperty can expose its own API to tweak their UX preferences. Anywho, that is my rationale. This PR seems to address all the issues previously encountered. Let me know your thoughts.

[KoBeWi]

EditorPropertyColor was firing property_changed as part of live changes, but this was undesirable, as it generates undo operations.

It causes live edit to not preview color changes in real time. Continuous undo operations are not really a problem; that's what undoredo merging is for.

[MajorMcDoom]

EditorPropertyColor was firing property_changed as part of live changes, but this was undesirable, as it generates undo operations.

It causes live edit to not preview color changes in real time. Continuous undo operations are not really a problem; that's what undoredo merging is for.

This PR does not break live previews. Is that what you are seeing?

Also, undo redo merging is not relevant here because the color picker is not only capable of generating multiple successive undo operations that don't merge, they also cannot be undone until you close the dialog, after which you have to undo all your edits you did in the dialog.

[KoBeWi]

By live edit I mean syncing scene changes with the running project:

https://github.com/godotengine/godot/assets/2223172/95a1fb94-906a-474e-9528-5d94432963e7

With your PR the changes are synced only once the dialog is closed:

https://github.com/godotengine/godot/assets/2223172/76f55c30-a858-425f-af0e-1aa2483a2cf5

Though not sure how important is that. Otherwise the new behavior is an improvement.

[MajorMcDoom]

@AThousandShips The Linux editor build failed with the following message: FATAL ERROR: An incorrectly used memory was found. Could it be related to this closed issue? #78749

[AThousandShips]

Probably not, will re run and see if it's just a random thing, otherwise it's somehow related to this PR

[akien-mga]

Thanks!

[TheSofox]

Great job. I tried to tackle this a bit ago but couldn't figure out how to fix this while keeping the live preview working. Great to see this resolved.