Undoing color change in Inspector requires two Crtl-Z's (sometimes)

[miv391]

Godot version

v4.2.beta2.official [f8818f8]

System information

Godot v4.2.beta2 - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1060 6GB (NVIDIA; 31.0.15.3640) - Intel(R) Core(TM) i5-3570K CPU @ 3.40GHz (4 Threads)

Issue description

After a color property is changed in Inspector (for example Sprite2D / Visibility / Modulate), undoing it requires pressing Ctrl-Z (in Windows) two times. I tried other kind of properties too, but only color seems to have this feature.

Steps to reproduce

This is what I see in Output, when I change Sprite2D's Visible property (boolean) and Modulate property (color) and then press Ctrl-Z three times.

Set visible
Set modulate
Set modulate
Scene Undo: Set modulate    -> 1. Ctrl-Z, nothing happens
Scene Undo: Set modulate    -> 2. Ctrl-Z, color change is undoed
Scene Undo: Set visible     -> 3. Ctrl-Z, boolean is undoed

In that example I used color swatch to select a new color and then I closed the color picker by clicking the same color picker button.

However, sometimes only one Ctrl-Z is enough to undo. I don't know what it exactly needs, but by doing the "select color and undo it" cycle a few times usually seems to change something and one Ctrl-Z starts working. But if another color (e.g. Self Modulate) is changed next, it again needs two Ctrl-Z's.

Minimal reproduction project

N/A

[astillich]

This seems to be a general problem with shortcuts. The same happens with the copy & paste shortcuts in the blend tree editor #83518.

[aXu-AP]

I don't think this has something to do with shortcut, the color change is committed twice into history. I made a pr for fixing it, however I didn't find that other place which commits the color so I'm missing something from the big picture. The fix seems to be working well still. #83787

Found another bug when searching for this and made a fix for that. I noticed that Set modulate text appeared thrice in console, meaning there was still one place which made extra color change, altough it didn't result in extra commit: #83786

[TheSofox]

I looked into this myself and I think it's more complicated and buggy at first instance, even taking your fix into account.

Repo

1. Open up the history tab (maybe move it so it and the Inspector are both visible at the same time), and then do some standard stuff with the sprite (moving, visibility toggle, etc).

2. The open the color picker, and with the picker open, change the colour a several times, maybe try different swatches.

3. Close the color picker.

4. Try to undo and redo the last few operations. While keeping an eye on the History windows

Expected:
Your position in the History list should correspond to the actions you are doing/undoing.

Actual:
Things get desynched. Somehow the history queue only lists about 2 modulate actions (despite only opening up the picker once). Yet you can undo through several different colour changes that you did while the picker was open. This leads to the history queue listing that you are undoing a move or visiblity operation, when you're still undoing modulate colour changes.

I'm not sure what's going on exactly. I think the UndoRedo system has some system for merging several changes that are made right next to eachother, but for some reason it's not working properly with the ColourPicker.

In terms of the files effected, I think editor_inspector.cpp which has the functions
EditorInspector::_property_changed and EditorInspector::_edit_set

The _edit_set is a big function that makes heavy use of EditorUndoRedoManager which I think is what handles the history merging.

[aXu-AP]

You're right, looking at the history tab exposed that my fix doesn't work as intended. I should use that tab more often, I don't think I ever have used it 😅. Changed my pr to draft.

[TheSofox]

I decided to add a entirely new Issue #83945 since this is bigger than a simple Undo hiccup.

[TheSofox]

So I found the root cause of the desync bug and submitted a pull request for its fix ( found here: #84557 ). Once that is accepted, you should check your Pull Request again because it'll be easier to see whether it fixes this specific bug without the underlying bug messing with results.

[aXu-AP]

Nice, subbed to your pr so that I won't miss it.

[TheSofox]

My PR was merged. I've a feeling that your Pull Requests will work fine now, though still best you double check them.

[aXu-AP]

Ok I tested my prs now and it works as long as they both were applied, so I merged them into one pr.

Altough I think committing colors into history until the popup is closed is unnecessary anyway, since choosing color is not incremental work in the same vein as writing text.

[TheSofox]

Great going with the PR.

As for whether changing colours is incremental... well, it technically can be, maybe switching between swatches for example? But I do see your point. Back when I first looked into it I tried to change it so the colour change only happened when you closed the popup, but then the live preview of the colour change didn't happen.

[aXu-AP]

I see, because the inspector abstracts away the calls to undoredo, it's not so easy to make it work that way. But it's not an actual problem, not worth it to look into further. Plus it's good when there's consistency in editor logic (even when it's a bit extra at times).