Use similar logic of pressing ESC key to prevent undo push.
First check if values in panel has been tweaked or not, then set the
menuretval accordingly
Details
Details
- Reviewers
Campbell Barton (campbellbarton) - Maniphest Tasks
- T60609: Color picker undo adds step when cursor exist
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
| source/blender/editors/interface/interface_region_color_picker.cc | ||
|---|---|---|
| 874 ↗ | (On Diff #53344) | Normally these kinds of comments don't use the bug as tag, something like this is more in keeping with our current code style. /* NOTE: The undo flag is enabled if values are edited, see: T60609. */ |
Comment Actions
Looking into this, it causes the eye-dropper not to undo when activated from the popup.
Pressing escape doesn't do an undo push but clicking away from the popup does, I think we need to change the logic of UI_BLOCK_OUT_1 so it is aware if a change has been made.
Comment Actions
Looking into this, it causes the eye-dropper not to undo when activated from the popup.
@Campbell Barton (campbellbarton) , thanks. do you know about any of the case where eyedropper operator is executed but undo-push is intentionally skipped.
I'm talking about this change: rBc8e75c2b00cfb7e87bcb800c9acc8f126949749d (/* Could support finished & undo-skip. */)
Above commit is actually preventing the Undo-push because UI_BUT_UNDO flag is not set
Is it a good idea to remove eye->is_undo = UI_but_flag_is_set(but, UI_BUT_UNDO); ?
or do you expect some other fix?
Comment Actions
@Pratik Borhade (PratikPB2123) I don't know of other cases where this would cause a problem, but I think it's reasonable UI_BUT_UNDO is used as a way to check if changing that setting should undo or not.
Extension the logic that is used with UI_BLOCK_OUT_1 seems like a better solution, to be able to conditionally detect if closing a popup should be considered cancelled or that a change a change was made (as it is with UI_BLOCK_OUT_1).
Comment Actions
Disable UI_BUT_UNDO to avoid undo push if no change has been made in color panel
@Campbell Barton (campbellbarton) , like this?
Comment Actions
It shouldn't be necessary to change the button's UI_BUT_UNDO flag as it could interfere with future use of this button (where undo will be expected).
Currently you can open the color popup and press Escape, which wont push an undo step, while moving the mouse out does.
As far as I can see the correct fix is to make moving the mouse away from the popup use an equivalent code path to the logic for pressing Escape, which is - to do nothing but only if nothing was changed.
I'm not too sure on the exact details though, it may mean extending UI_BLOCK_OUT_1 set the menu->menuretval value based on but->changed, this could be default behavior if it doesn't cause problems in other uses of UI_BLOCK_OUT_1, otherwise it could depend on the button type for e.g. So as not to change behavuor of other button types.
Comment Actions
Use similar logic of pressing ESC key to prevent undo push.
When esc key is pressed, it sets menu->menuretval to
UI_RETURN_CANCEL, which later sets data->cancel = true
And undo push is not made when cancel is true, see: button_activate_exit