Bug 392014 - Implement Undo/Redo Move Tool
Summary: Implement Undo/Redo Move Tool
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: Tools/Move (show other bugs)
Version: nightly build (please specify the git hash!)
Platform: Microsoft Windows Microsoft Windows
: NOR major
Target Milestone: ---
Assignee: Dmitry Kazakov
URL:
Keywords: regression, usability
: 378862 393456 395386 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-03-18 19:41 UTC by eliotJ
Modified: 2018-09-16 05:57 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description eliotJ 2018-03-18 19:41:04 UTC
1. First moves (by Move Tool) some part of image which has selection.
2. Next hit Ctrl + Z for undo Move Tool operation.
3. After this redo doesn't work for Move Tool operation...

I have tried on K 4.0 nightly build git b4b1459
Comment 1 mvowada 2018-03-18 21:43:04 UTC
I can confirm on Linux Ubuntu as well - Krita 4.1.0-pre-alpha (git 97015cd)

(possibly related? Bug 389561, Bug 389876)
Comment 2 Dmitry Kazakov 2018-04-04 13:07:26 UTC
Git commit d72098bf9463b6ed25befafd84bc9d5d667a41de by Dmitry Kazakov.
Committed on 04/04/2018 at 13:07.
Pushed by dkazakov into branch 'master'.

Fix outline offset when cancelling move tool's stroke

M  +9    -19   plugins/tools/basictools/kis_tool_move.cc

https://commits.kde.org/krita/d72098bf9463b6ed25befafd84bc9d5d667a41de
Comment 3 Dmitry Kazakov 2018-04-04 13:07:26 UTC
Git commit f094f600e2e12f03181bb05ad291af6768a71b18 by Dmitry Kazakov.
Committed on 04/04/2018 at 13:07.
Pushed by dkazakov into branch 'master'.

Disable handling Ctrl+Z as "cancel" action in KisToolMove

Basically, we shouldn't cancel the stroke on undo in "continued"
tool actions. Instead the user should use "Esc" key for that.

Otherwise the previous command (the one that came before the
current action) will be undone, which is not what the user wants.

Ideally we should finally implement an undo system for the move tool,
but it is still a todo.

M  +7    -0    plugins/tools/basictools/kis_tool_move.cc
M  +1    -0    plugins/tools/basictools/kis_tool_move.h

https://commits.kde.org/krita/f094f600e2e12f03181bb05ad291af6768a71b18
Comment 4 Dmitry Kazakov 2018-04-04 13:08:21 UTC
I'm downgrading this bug to a wishlist. Undo/Redo system is just not implemented for the move tool. It should be implemented though :)
Comment 5 Dmitry Kazakov 2018-04-07 13:45:48 UTC
Git commit 21faf92eadc4387facf7938acac53b4cf09b1d65 by Dmitry Kazakov.
Committed on 07/04/2018 at 13:28.
Pushed by dkazakov into branch 'krita/4.0'.

Disable handling Ctrl+Z as "cancel" action in KisToolMove

Basically, we shouldn't cancel the stroke on undo in "continued"
tool actions. Instead the user should use "Esc" key for that.

Otherwise the previous command (the one that came before the
current action) will be undone, which is not what the user wants.

Ideally we should finally implement an undo system for the move tool,
but it is still a todo.

M  +7    -0    plugins/tools/basictools/kis_tool_move.cc
M  +1    -0    plugins/tools/basictools/kis_tool_move.h

https://commits.kde.org/krita/21faf92eadc4387facf7938acac53b4cf09b1d65
Comment 6 Dmitry Kazakov 2018-04-07 13:46:04 UTC
Git commit 379725fd58eeb0b6557c84d3c84c51e044b90760 by Dmitry Kazakov.
Committed on 07/04/2018 at 13:28.
Pushed by dkazakov into branch 'krita/4.0'.

Fix outline offset when cancelling move tool's stroke

M  +9    -19   plugins/tools/basictools/kis_tool_move.cc

https://commits.kde.org/krita/379725fd58eeb0b6557c84d3c84c51e044b90760
Comment 7 Tyson Tan 2018-04-08 13:04:04 UTC
Suggestion: can we recognize it as "one move stroke" when user releases left mouse button? Because:

1) It's how we expect things to work -- we consider it one "move" after we stop and release mouse button. And take it for granted that the program will recognize that as "one move" so that we can easily Undo/Redo.

2) It's difficult for new users to teach themselves about ESC key's function in move tool like this. It's inconsistent with all other places in Krita. It's confusing that after using Move tool for one time we cannot undo anymore. It just feels like a bug.
Comment 8 Raghavendra kamath 2018-04-14 05:03:39 UTC
Is this bug somehow related to this bug report -> https://bugs.kde.org/show_bug.cgi?id=378862
Comment 9 mvowada 2018-04-24 08:47:47 UTC
*** Bug 393456 has been marked as a duplicate of this bug. ***
Comment 10 Halla Rempt 2018-08-29 13:34:44 UTC
*** Bug 395386 has been marked as a duplicate of this bug. ***
Comment 11 Halla Rempt 2018-08-29 13:35:44 UTC
Raising this to the level of bug, since it's a regression from Krita 4.0 and very annoying to our users.
Comment 12 Halla Rempt 2018-08-29 13:36:01 UTC
*** Bug 378862 has been marked as a duplicate of this bug. ***
Comment 13 Dmitry Kazakov 2018-09-13 10:56:50 UTC
Git commit 3aa6dbcc5eda4f04d9805f6d4c97c013caa0f56f by Dmitry Kazakov.
Committed on 13/09/2018 at 10:56.
Pushed by dkazakov into branch 'master'.

Now move tool also supports undo for intermediate actions
CC:kimageshop@kde.org

M  +2    -0    libs/ui/CMakeLists.txt
A  +46   -0    libs/ui/tool/KisToolChangesTracker.cpp     [License: UNKNOWN]  *
A  +31   -0    libs/ui/tool/KisToolChangesTracker.h     [License: UNKNOWN]  *
A  +18   -0    libs/ui/tool/KisToolChangesTrackerData.cpp     [License: UNKNOWN]  *
A  +19   -0    libs/ui/tool/KisToolChangesTrackerData.h     [License: UNKNOWN]  *
M  +140  -116  plugins/tools/basictools/kis_tool_move.cc
M  +11   -7    plugins/tools/basictools/kis_tool_move.h
M  +3    -0    plugins/tools/basictools/kis_tool_movetooloptionswidget.cpp
M  +2    -0    plugins/tools/basictools/kis_tool_movetooloptionswidget.h
M  +0    -1    plugins/tools/tool_transform2/CMakeLists.txt
M  +9    -5    plugins/tools/tool_transform2/kis_tool_transform.cc
M  +3    -3    plugins/tools/tool_transform2/kis_tool_transform.h
M  +5    -0    plugins/tools/tool_transform2/tool_transform_args.cc
M  +4    -2    plugins/tools/tool_transform2/tool_transform_args.h
D  +0    -49   plugins/tools/tool_transform2/tool_transform_changes_tracker.cpp
D  +0    -54   plugins/tools/tool_transform2/tool_transform_changes_tracker.h

The files marked with a * at the end have a non valid license. Please read: https://community.kde.org/Policies/Licensing_Policy and use the headers which are listed at that page.


https://commits.kde.org/krita/3aa6dbcc5eda4f04d9805f6d4c97c013caa0f56f
Comment 14 Halla Rempt 2018-09-13 11:59:34 UTC
Git commit 0857f999ae86b1a403cee9141877da5c10c58db9 by Boudewijn Rempt, on behalf of Dmitry Kazakov.
Committed on 13/09/2018 at 11:59.
Pushed by rempt into branch 'krita/4.1'.

Now move tool also supports undo for intermediate actions
CC:kimageshop@kde.org

M  +2    -0    libs/ui/CMakeLists.txt
A  +46   -0    libs/ui/tool/KisToolChangesTracker.cpp     [License: UNKNOWN]  *
A  +31   -0    libs/ui/tool/KisToolChangesTracker.h     [License: UNKNOWN]  *
A  +18   -0    libs/ui/tool/KisToolChangesTrackerData.cpp     [License: UNKNOWN]  *
A  +19   -0    libs/ui/tool/KisToolChangesTrackerData.h     [License: UNKNOWN]  *
M  +140  -116  plugins/tools/basictools/kis_tool_move.cc
M  +11   -7    plugins/tools/basictools/kis_tool_move.h
M  +3    -0    plugins/tools/basictools/kis_tool_movetooloptionswidget.cpp
M  +2    -0    plugins/tools/basictools/kis_tool_movetooloptionswidget.h
M  +0    -1    plugins/tools/tool_transform2/CMakeLists.txt
M  +9    -5    plugins/tools/tool_transform2/kis_tool_transform.cc
M  +3    -3    plugins/tools/tool_transform2/kis_tool_transform.h
M  +5    -0    plugins/tools/tool_transform2/tool_transform_args.cc
M  +4    -2    plugins/tools/tool_transform2/tool_transform_args.h
D  +0    -49   plugins/tools/tool_transform2/tool_transform_changes_tracker.cpp
D  +0    -54   plugins/tools/tool_transform2/tool_transform_changes_tracker.h

The files marked with a * at the end have a non valid license. Please read: https://community.kde.org/Policies/Licensing_Policy and use the headers which are listed at that page.


https://commits.kde.org/krita/0857f999ae86b1a403cee9141877da5c10c58db9
Comment 15 Tyson Tan 2018-09-13 19:40:11 UTC
Thank you guys! This is going to help a lot!

However, this implementation has some usability  problem:
1) When Move tool is still in-use, it cannot Undo the previous actions before the Move tool is selected, which is confusing.
2) If I use any other tool after Move tool, only the final result of the Move tool is recorded in history.

Because of the inconsistencies between normal Undo/Redo logic and Move tool's inside logic, it's kinda confusing. Is it possible to treat each Move as a single history entry?
Comment 16 Dmitry Kazakov 2018-09-14 08:37:22 UTC
Hi, Tyson!

The main idea of the move tool was to compress all the intermediate moves and not to bloat the undo stack, so I actually don't know how we should fix that.

Here is a test patch with a compromise solution: 
https://phabricator.kde.org/P260

It that does two things:

1) adds an explicit frame when the move stroke is in progress; 
2) fixes your point 1 (ctrl+z cancels the stroke if there is nothing to undo)

But this patch doesn't make every move a distinct action in history. It only makes UIX a bit more smooth.
Comment 17 Dmitry Kazakov 2018-09-14 09:28:31 UTC
Git commit 14d70faef8b56cf955b3846dce12e27cbde26302 by Dmitry Kazakov.
Committed on 14/09/2018 at 08:59.
Pushed by dkazakov into branch 'master'.

Fix minor UIX issues in the move tool

1) adds an explicit frame when the move stroke is in progress;
2) Ctrl+Z now cancels the stroke if there is nothing to undo

M  +4    -2    libs/ui/tool/KisToolChangesTracker.cpp
M  +9    -5    plugins/tools/basictools/kis_tool_move.cc
M  +0    -1    plugins/tools/basictools/kis_tool_move.h
M  +5    -1    plugins/tools/tool_transform2/kis_tool_transform.cc

https://commits.kde.org/krita/14d70faef8b56cf955b3846dce12e27cbde26302
Comment 18 Dmitry Kazakov 2018-09-14 13:48:13 UTC
Git commit eea81dcb74b0ce1fd7c8371216978262f002732d by Dmitry Kazakov.
Committed on 14/09/2018 at 13:34.
Pushed by dkazakov into branch 'krita/4.1'.

Fix minor UIX issues in the move tool

1) adds an explicit frame when the move stroke is in progress;
2) Ctrl+Z now cancels the stroke if there is nothing to undo

M  +4    -2    libs/ui/tool/KisToolChangesTracker.cpp
M  +9    -5    plugins/tools/basictools/kis_tool_move.cc
M  +0    -1    plugins/tools/basictools/kis_tool_move.h
M  +5    -1    plugins/tools/tool_transform2/kis_tool_transform.cc

https://commits.kde.org/krita/eea81dcb74b0ce1fd7c8371216978262f002732d
Comment 19 Tyson Tan 2018-09-16 05:57:31 UTC
Thanks Dmitry. The workflow feels so much better now.

I wonder whether it is possible to separate each Move action when Krita detects the cursor stops moving. This way we treat Move tool actions the same as Brush tool, with each stroke of Move action being recorded as a single history node. By doing this, the artist can compare different position even after they used other tools afterward. 

Same suggestion goes to Transformation tool.