Bug 389876 - Node properties that are not recorded in undo history should mark the document as modified but don't
Summary: Node properties that are not recorded in undo history should mark the documen...
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: General (show other bugs)
Version: 4.0.0-beta.1
Platform: Microsoft Windows Microsoft Windows
: NOR normal
Target Milestone: ---
Assignee: Dmitry Kazakov
URL:
Keywords: regression
: 389561 403149 407302 410697 412623 425585 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-02-04 14:43 UTC by kalia24
Modified: 2021-12-13 08:16 UTC (History)
10 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 kalia24 2018-02-04 14:43:00 UTC
Steps:

1. Create file, put something in, save - everything is fine.
2. Change layer visibility, attempt to save - it still shows [Modified] next to file name and asks to save file at closing (even though the change is actually saved - u can see it in file icon).
Comment 1 Raghavendra kamath 2018-02-04 16:03:03 UTC
+1 I can reproduce this in Krita git master.

Even though I see the message "Finished saving example.kra" in the status bar area the title of the document still says unmodified
Comment 2 Halla Rempt 2018-02-08 13:01:50 UTC
*** Bug 389561 has been marked as a duplicate of this bug. ***
Comment 3 Halla Rempt 2018-02-08 13:41:38 UTC
Git commit 27ba395af739496dcd0635c7f84ea19da650c7c6 by Boudewijn Rempt.
Committed on 08/02/2018 at 13:39.
Pushed by rempt into branch 'master'.

Check all changed node properties when determining undo/redo

If there's even one property in the list of properties that has
changed and that isn't one of the properties that should not end
up in the undo list, we should make an undo command.

And if there isn't any, then then the document should not be set
to modified.

M  +38   -15   libs/image/commands/kis_node_property_list_command.cpp

https://commits.kde.org/krita/27ba395af739496dcd0635c7f84ea19da650c7c6
Comment 4 Halla Rempt 2019-02-18 10:58:01 UTC
*** Bug 403149 has been marked as a duplicate of this bug. ***
Comment 5 Halla Rempt 2019-02-18 10:58:42 UTC
Seems this broke. I think I remember that it was intentional, but I cannot find anything about that in the history. I'll have to check that patch again.
Comment 6 Halla Rempt 2019-02-18 11:32:24 UTC
These properties are:

name
visible
locked
active
alpha locked
Comment 7 Halla Rempt 2019-05-07 15:21:51 UTC
*** Bug 407302 has been marked as a duplicate of this bug. ***
Comment 8 Halla Rempt 2019-05-07 15:38:57 UTC
Git commit a3a4a7bfa73b888689cc6b6eb19f9083c8b0b18a by Boudewijn Rempt.
Committed on 07/05/2019 at 15:38.
Pushed by rempt into branch 'master'.

Set the document to modified if any layer properties change

We don't want to have layer visibility changes end up in the undo
stack, but do want the document to be marked as modified.

M  +2    -1    libs/image/commands/kis_node_property_list_command.cpp

https://invent.kde.org/kde/krita/commit/a3a4a7bfa73b888689cc6b6eb19f9083c8b0b18a
Comment 9 Halla Rempt 2019-08-10 08:57:03 UTC
*** Bug 410697 has been marked as a duplicate of this bug. ***
Comment 10 Halla Rempt 2019-08-10 09:36:58 UTC
Zut, this has regressed again.
Comment 12 Ahab Greybeard 2019-10-06 06:10:45 UTC
*** Bug 412623 has been marked as a duplicate of this bug. ***
Comment 13 Halla Rempt 2020-08-21 10:19:48 UTC

*** This bug has been marked as a duplicate of bug 425585 ***
Comment 14 Halla Rempt 2020-08-21 10:21:05 UTC
*** Bug 425585 has been marked as a duplicate of this bug. ***
Comment 15 Halla Rempt 2020-08-21 10:21:54 UTC
Ivan, I'm assigning this issue to you since it was a commit of yours that broke it.
Comment 16 Dmitry Kazakov 2020-08-21 18:30:20 UTC
Hi, Ivan!

Please check this thread on KA:
https://krita-artists.org/t/take-layer-state-into-account-in-the-undo-history/11040/3

Perhaps it would be easier just to make them undoable :)
Comment 17 vanyossi 2020-08-22 09:17:58 UTC
Thanks dmitry, I was working on this bug and I think it is tricky to get the right solution.
Comment 18 Halla Rempt 2020-08-22 14:51:13 UTC
No: we should not make those changes undoable. We intentionally took them out of the undo stack, and we shouldn't flip-flop on that _or_ make it into an option. We should _also_ not succomb to design-by-committee by asking users about everything we implement.
Comment 19 Dmitry Kazakov 2020-08-26 13:41:25 UTC
Hi, vanyossi!

Can I take this bug? It seems to be related to other work I do atm?
Comment 20 Halla Rempt 2020-12-14 13:13:16 UTC
Dmitry is actually working on this, so fix the assignee field.
Comment 21 Dmitry Kazakov 2021-01-12 08:18:05 UTC
Git commit f55b1b56bebea07d58d3289e94181560b91d64ce by Dmitry Kazakov.
Committed on 12/01/2021 at 08:15.
Pushed by dkazakov into branch 'krita/4.3'.

Fix modified state of the image after changing layer visibility

Now KisDocument has a special field imageModifiedWithoutUndo, which
records the fact if modifying the image without undo, e.g. by changing
layer visibility. This field ca be reset only by saving the image,
not by changing the state if the undo stack.

M  +1    -0    libs/image/KisImageSignals.h
M  +6    -3    libs/image/commands/kis_node_property_list_command.cpp
M  +6    -0    libs/image/kis_image.cc
M  +15   -0    libs/image/kis_image.h
M  +4    -0    libs/image/kis_image_signal_router.cpp
M  +1    -0    libs/image/kis_image_signal_router.h
M  +17   -2    libs/ui/KisDocument.cpp
M  +1    -0    libs/ui/KisDocument.h

https://invent.kde.org/graphics/krita/commit/f55b1b56bebea07d58d3289e94181560b91d64ce
Comment 22 Dmitry Kazakov 2021-01-12 08:19:38 UTC
Git commit 0c53270b078ae04c51a1f58b8291031c6a46305b by Dmitry Kazakov.
Committed on 12/01/2021 at 08:19.
Pushed by dkazakov into branch 'master'.

Fix modified state of the image after changing layer visibility

Now KisDocument has a special field imageModifiedWithoutUndo, which
records the fact if modifying the image without undo, e.g. by changing
layer visibility. This field ca be reset only by saving the image,
not by changing the state if the undo stack.

M  +1    -0    libs/image/KisImageSignals.h
M  +6    -3    libs/image/commands/kis_node_property_list_command.cpp
M  +6    -0    libs/image/kis_image.cc
M  +15   -0    libs/image/kis_image.h
M  +4    -0    libs/image/kis_image_signal_router.cpp
M  +1    -0    libs/image/kis_image_signal_router.h
M  +17   -2    libs/ui/KisDocument.cpp
M  +1    -0    libs/ui/KisDocument.h

https://invent.kde.org/graphics/krita/commit/0c53270b078ae04c51a1f58b8291031c6a46305b
Comment 23 Tyson Tan 2021-01-12 09:24:56 UTC
Thank you everyone! :D
Comment 24 Dmitry Kazakov 2021-01-13 12:32:07 UTC
Git commit 21f7c02319970e18a40ea13aab7e03b877072193 by Dmitry Kazakov.
Committed on 13/01/2021 at 10:43.
Pushed by dkazakov into branch 'master'.

Implement an ability for an undo command to be annihilated when merged with another command

Basically, when the user changes visibility to and fro, the undo step
should be removed.

M  +44   -1    libs/command/kundo2stack.cpp
M  +2    -0    libs/command/kundo2stack.h
M  +9    -0    libs/image/commands/kis_node_property_list_command.cpp
M  +1    -0    libs/image/commands/kis_node_property_list_command.h

https://invent.kde.org/graphics/krita/commit/21f7c02319970e18a40ea13aab7e03b877072193
Comment 25 Halla Rempt 2021-12-13 08:07:52 UTC
*** Bug 446889 has been marked as a duplicate of this bug. ***
Comment 26 Halla Rempt 2021-12-13 08:08:19 UTC
And they are in the undo stack again...
Comment 27 Halla Rempt 2021-12-13 08:11:45 UTC
Okay, dmitry says:

09:09:57 < dmitryK> halla: erm... but there was a huge discussion about that and we decided to make them 
                    undoable, afair
09:10:30 < dmitryK> halla: with the exception that two sequential visibility flips should not add another undo 
                    step

So, let's keep this closed and not change anything.
Comment 28 Dmitry Kazakov 2021-12-13 08:15:32 UTC
After a discussion some time ago we discussed that visibility changes do land in the undo stack, **but** two sequential visibility flips to and fro will remove the undo step from the undo stack. That is, if you set visibility on and off, then no undo step appears.