Bug 406697 - Cut & Paste layers is possible inside locked group
Summary: Cut & Paste layers is possible inside locked group
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: Layer Stack (show other bugs)
Version: nightly build (please specify the git hash!)
Platform: Microsoft Windows Microsoft Windows
: NOR normal
Target Milestone: ---
Assignee: Dmitry Kazakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-20 09:11 UTC by Tiar
Modified: 2020-11-19 15:49 UTC (History)
3 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 Tiar 2019-04-20 09:11:44 UTC
SUMMARY
You can easily cut and paste layers from/into a group layer, even though you can't remove layers. Other operations possible: duplicate layer; merge layer; flatten layer; flatten image.


STEPS TO REPRODUCE
1. Make a group layer.
2. Add some layers inside.
3. Perform any operations mntioned above.

OBSERVED RESULT
It is possible to do that.

EXPECTED RESULT
Since you can't remove layers, being able to cut them and perform other operations seems inconsistent.

NOTES
You can't remove layer by clicking "trashbin" icon or by right-clicking on the layer.
If you try to add a new layer (while having a layer inside the group selected), the layer is added over the locked group, not inside. If you unlock the group and try to do the same thing, the new layer will appear inside the group.


SOFTWARE/OS VERSIONS
Version: 4.2.0-pre-alpha (git fb08951)
Windows: 10
Qt Version: 5.12.2
Comment 1 Halla Rempt 2019-04-24 13:50:56 UTC
What you describe in the NOTES is at least intentional; the other actions should not be possible.
Comment 2 wolthera 2019-09-21 16:00:43 UTC
If two developers agree on something being a bug it is confirmed.
Comment 3 Dmitry Kazakov 2020-08-14 20:01:57 UTC
Git commit bac3cdd85570404f17c156d79a4200f9932b4d92 by Dmitry Kazakov.
Committed on 14/08/2020 at 19:54.
Pushed by dkazakov into branch 'krita/4.3'.

Fix flatten/merge down/merge multiple action handle locked groups correctly

This patch extends the already existing strategy, which was previously
used for merge down operation only:

1) If there is at least one unlocked layer in the selection, then the
   merge will happen.
2) The merge result will be written into nearest unlocked parent
3) All unlocked layers of the selection will be removed
4) All locked layers will be kept in the layers stack, but switched
   into *invisible* state.
CC:kimageshop@kde.org

M  +85   -20   libs/image/kis_layer_utils.cpp
M  +18   -0    libs/ui/kis_layer_manager.cc

https://invent.kde.org/graphics/krita/commit/bac3cdd85570404f17c156d79a4200f9932b4d92
Comment 4 Dmitry Kazakov 2020-08-14 20:02:05 UTC
Git commit 7db68af4d1b9fd5816eb8d206627ec4231dfc672 by Dmitry Kazakov.
Committed on 14/08/2020 at 19:56.
Pushed by dkazakov into branch 'krita/4.3'.

Fix layer duplication handle locked layers properly

M  +7    -0    libs/ui/kis_node_juggler_compressed.cpp

https://invent.kde.org/graphics/krita/commit/7db68af4d1b9fd5816eb8d206627ec4231dfc672
Comment 5 Dmitry Kazakov 2020-08-14 20:02:19 UTC
Git commit fe9741d27cb87a59b3add558489ce5b407a35d9a by Dmitry Kazakov.
Committed on 14/08/2020 at 20:02.
Pushed by dkazakov into branch 'master'.

Fix flatten/merge down/merge multiple action handle locked groups correctly

This patch extends the already existing strategy, which was previously
used for merge down operation only:

1) If there is at least one unlocked layer in the selection, then the
   merge will happen.
2) The merge result will be written into nearest unlocked parent
3) All unlocked layers of the selection will be removed
4) All locked layers will be kept in the layers stack, but switched
   into *invisible* state.
CC:kimageshop@kde.org

M  +85   -20   libs/image/kis_layer_utils.cpp
M  +18   -0    libs/ui/kis_layer_manager.cc

https://invent.kde.org/graphics/krita/commit/fe9741d27cb87a59b3add558489ce5b407a35d9a
Comment 6 Dmitry Kazakov 2020-08-14 20:02:28 UTC
Git commit bccda781066a1e9be37d45ad6e5db6aa5d7f70b1 by Dmitry Kazakov.
Committed on 14/08/2020 at 20:02.
Pushed by dkazakov into branch 'master'.

Fix layer duplication handle locked layers properly

M  +7    -0    libs/ui/kis_node_juggler_compressed.cpp

https://invent.kde.org/graphics/krita/commit/bccda781066a1e9be37d45ad6e5db6aa5d7f70b1
Comment 7 Dmitry Kazakov 2020-08-17 22:57:18 UTC
Git commit 9fe41211deb5a77aa501e379086df93e980240ab by Dmitry Kazakov.
Committed on 17/08/2020 at 22:57.
Pushed by dkazakov into branch 'krita/4.3'.

Make sure the user cannot "cut" locked layer

M  +24   -3    libs/ui/kis_node_manager.cpp

https://invent.kde.org/graphics/krita/commit/9fe41211deb5a77aa501e379086df93e980240ab
Comment 8 Dmitry Kazakov 2020-08-17 22:57:26 UTC
Git commit 1893c0a79a225d01b0bb364bafe6bb37cd23315c by Dmitry Kazakov.
Committed on 17/08/2020 at 22:57.
Pushed by dkazakov into branch 'krita/4.3'.

Fix actions not to modify locked layers

1) When trying to D&D **from** a locked layer Krita will
   force a "copy" mode

2) When trying to D&D **into** a locked group the operation will
   be forbidden. If the user still managed to do that, the layer
   will be put into a new position.

3) Layer->Properties should not be allowed for locked layers or
   masks.

4) Layer->Convert should not be allowed for locked layers

5) Lower/Raise node should not move nodes in a locked group

6) Lower/Raise node should not try to enter a locked group
Related: bug 425412

M  +18   -5    libs/ui/kis_layer_manager.cc
M  +9    -15   libs/ui/kis_mask_manager.cc
M  +17   -13   libs/ui/kis_node_juggler_compressed.cpp
M  +84   -29   libs/ui/kis_node_manager.cpp
M  +8    -1    libs/ui/kis_node_manager.h
M  +7    -3    libs/ui/kis_node_model.cpp

https://invent.kde.org/graphics/krita/commit/1893c0a79a225d01b0bb364bafe6bb37cd23315c
Comment 9 Dmitry Kazakov 2020-08-17 22:57:34 UTC
Git commit 36d91be01f3059bc1c8db9a814d1248021047c34 by Dmitry Kazakov.
Committed on 17/08/2020 at 22:57.
Pushed by dkazakov into branch 'krita/4.3'.

Make sure pasted (and D&D'ed) layers are not pasted into locked groups

M  +1    -1    libs/ui/kis_mimedata.cpp

https://invent.kde.org/graphics/krita/commit/36d91be01f3059bc1c8db9a814d1248021047c34
Comment 10 Dmitry Kazakov 2020-08-17 23:24:56 UTC
Git commit e885115b21a96c47734a126fd3de43f71e897f95 by Dmitry Kazakov.
Committed on 17/08/2020 at 23:00.
Pushed by dkazakov into branch 'master'.

Fix actions not to modify locked layers

1) When trying to D&D **from** a locked layer Krita will
   force a "copy" mode

2) When trying to D&D **into** a locked group the operation will
   be forbidden. If the user still managed to do that, the layer
   will be put into a new position.

3) Layer->Properties should not be allowed for locked layers or
   masks.

4) Layer->Convert should not be allowed for locked layers

5) Lower/Raise node should not move nodes in a locked group

6) Lower/Raise node should not try to enter a locked group
Related: bug 425412

# Conflicts:
#	libs/ui/kis_layer_manager.cc
#	libs/ui/kis_mask_manager.cc

M  +19   -4    libs/ui/kis_layer_manager.cc
M  +9    -15   libs/ui/kis_mask_manager.cc
M  +17   -13   libs/ui/kis_node_juggler_compressed.cpp
M  +84   -29   libs/ui/kis_node_manager.cpp
M  +8    -1    libs/ui/kis_node_manager.h
M  +7    -3    libs/ui/kis_node_model.cpp

https://invent.kde.org/graphics/krita/commit/e885115b21a96c47734a126fd3de43f71e897f95
Comment 11 Dmitry Kazakov 2020-08-17 23:25:04 UTC
Git commit f4ad7de3bcdc285729e0764dd850210b05af1380 by Dmitry Kazakov.
Committed on 17/08/2020 at 22:57.
Pushed by dkazakov into branch 'master'.

Make sure the user cannot "cut" locked layer

M  +24   -3    libs/ui/kis_node_manager.cpp

https://invent.kde.org/graphics/krita/commit/f4ad7de3bcdc285729e0764dd850210b05af1380
Comment 12 Dmitry Kazakov 2020-08-17 23:25:12 UTC
Git commit f0c27aa07d3bda35a1bec298c1345c4331f2a6f4 by Dmitry Kazakov.
Committed on 17/08/2020 at 22:57.
Pushed by dkazakov into branch 'master'.

Make sure pasted (and D&D'ed) layers are not pasted into locked groups

M  +1    -1    libs/ui/kis_mimedata.cpp

https://invent.kde.org/graphics/krita/commit/f0c27aa07d3bda35a1bec298c1345c4331f2a6f4
Comment 13 Tiar 2020-11-18 18:47:55 UTC
Git commit c1ab1ee97f196e73d8340918e2d9b7a79eafbbca by Agata Cacko.
Committed on 18/11/2020 at 18:45.
Pushed by tymond into branch 'master'.

Fix assert on New Layer From Visible on invisible active layer

Before this commit, Krita would assert on New Layer from Visible
when used on invisible layer. This commit fixes this.

Explanation:
Buckle up, it's going to be a ride :)

In 2016 there was a problem that invisible layers would show up
when they were merged, see bug 359707.
https://bugs.kde.org/show_bug.cgi?id=359707
As a solution, "just remove the layers separately instead of
merging" strategy was used.

In 2018 it turned out that in case when putAfter (the layer
Krita puts the merged result above of) is invisible, it gets
removed, so then it's null and Krita tries to merge other layers
in instead of putAfter, see bug 395903.
https://bugs.kde.org/show_bug.cgi?id=395903
As a solution, if putAfter was invisible, one of the visible
nodes-to-be-merged were assigned to putAfter to replace the
original putAfter node, to make sure that when original putAfter
is removed, some other layer is doing its job.
(The fact that it's a soon-to-be-merged layer is not a problem).

In 2020 it turned out that there is quite a lot of actions
that are possible in locked groups. It required some more changes
to the code. See bug 406697.
https://bugs.kde.org/show_bug.cgi?id=406697
A solution included a change to the relevant code such that
invisible putAfter would be unconditionally replaced with
one of the nodes from the to-be-merged list, and then
if putAfter had no parent, Krita would encounter an assert.

Problem with this behaviour:
In case of newLayerFromVisible(), mergedNodes list would only
contain a root node (which has no parent). If the putAfter is
invisible, then it's replaced with "one of" mergedNodes, which
means the root node. Which means it's going to assert.

However it's not always necessary to replace putAfter with one
of the to-be-merged nodes. It's only needed when it's going to
be removed. Hence moving the replacing into the if.
Because root is not invisible, the list of invisible nodes
will be empty and nothing will be replaced and nothing will
be removed.

NOTE: I also added a new condition: '&& cleanupNodes'.
It's not stricly necessary to cover all usecases of
mergeMultipleNodesImpl, because the only usecase where
cleanupNodes is false is the newLayerFromVisible case
(which will fail the first condition).
However it makes it more future-proof since the only case
those nodes needs to be removed is when cleanupNodes is true
(otherwise removing nodes would be a dataloss bug).

NOTE2: It's possible that putAfter replacement needs one more
check (to see if it is in fact in the invisibleNodes list).
I couldn't find a situation  where it was needed, though,
so I left it as it is.
Related: bug 428683, bug 359707, bug 395903

M  +15   -8    libs/image/kis_layer_utils.cpp

https://invent.kde.org/graphics/krita/commit/c1ab1ee97f196e73d8340918e2d9b7a79eafbbca
Comment 14 Tiar 2020-11-19 15:49:54 UTC
Git commit d25e08a3c6298f98850e1f22284f30536425eec1 by Agata Cacko.
Committed on 19/11/2020 at 15:49.
Pushed by tymond into branch 'krita/4.3'.

Fix assert on New Layer From Visible on invisible active layer

Before this commit, Krita would assert on New Layer from Visible
when used on invisible layer. This commit fixes this.

Explanation:
Buckle up, it's going to be a ride :)

In 2016 there was a problem that invisible layers would show up
when they were merged, see bug 359707.
https://bugs.kde.org/show_bug.cgi?id=359707
As a solution, "just remove the layers separately instead of
merging" strategy was used.

In 2018 it turned out that in case when putAfter (the layer
Krita puts the merged result above of) is invisible, it gets
removed, so then it's null and Krita tries to merge other layers
in instead of putAfter, see bug 395903.
https://bugs.kde.org/show_bug.cgi?id=395903
As a solution, if putAfter was invisible, one of the visible
nodes-to-be-merged were assigned to putAfter to replace the
original putAfter node, to make sure that when original putAfter
is removed, some other layer is doing its job.
(The fact that it's a soon-to-be-merged layer is not a problem).

In 2020 it turned out that there is quite a lot of actions
that are possible in locked groups. It required some more changes
to the code. See bug 406697.
https://bugs.kde.org/show_bug.cgi?id=406697
A solution included a change to the relevant code such that
invisible putAfter would be unconditionally replaced with
one of the nodes from the to-be-merged list, and then
if putAfter had no parent, Krita would encounter an assert.

Problem with this behaviour:
In case of newLayerFromVisible(), mergedNodes list would only
contain a root node (which has no parent). If the putAfter is
invisible, then it's replaced with "one of" mergedNodes, which
means the root node. Which means it's going to assert.

However it's not always necessary to replace putAfter with one
of the to-be-merged nodes. It's only needed when it's going to
be removed. Hence moving the replacing into the if.
Because root is not invisible, the list of invisible nodes
will be empty and nothing will be replaced and nothing will
be removed.

NOTE: I also added a new condition: '&& cleanupNodes'.
It's not stricly necessary to cover all usecases of
mergeMultipleNodesImpl, because the only usecase where
cleanupNodes is false is the newLayerFromVisible case
(which will fail the first condition).
However it makes it more future-proof since the only case
those nodes needs to be removed is when cleanupNodes is true
(otherwise removing nodes would be a dataloss bug).

NOTE2: It's possible that putAfter replacement needs one more
check (to see if it is in fact in the invisibleNodes list).
I couldn't find a situation  where it was needed, though,
so I left it as it is.
Related: bug 428683, bug 359707, bug 395903


(cherry picked from commit c1ab1ee97f196e73d8340918e2d9b7a79eafbbca)

M  +15   -8    libs/image/kis_layer_utils.cpp

https://invent.kde.org/graphics/krita/commit/d25e08a3c6298f98850e1f22284f30536425eec1