Details
- Reviewers
dkazakov - Group Reviewers
Krita - Commits
- R37:edf99991b2aa: Merge down should not remove locked layers
R37:556dd7e90221: Merge down should not remove locked layers
Diff Detail
- Repository
- R37 Krita
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Hi, @nsmirnov!
Please check KDE's coding policies to make formatting and indentation of the code correct :)
https://community.kde.org/Policies/Kdelibs_Coding_Style
And check my inline comments for details :)
| libs/image/kis_layer_utils.cpp | ||
|---|---|---|
| 685 | This code will not work if the user merges down more than 2 layer. Please use KritaUtils::filterContainer() to filter out the nodes in the list. You can find an example in KisToolMove::startStrokeImpl(). | |
| libs/ui/kis_layer_manager.cc | ||
| 637 |
| |
| 641 | Even though you added a check above guaranteeing that the size of selected nodes is not null, I would still recommend using more common and safe construction like for (it = selectedNodes.begin(); it != selectedNodes.end(); ++it). It is not guaranteed that one day someone comes and changes the 'if' above without noticing the problem in this loop. | |
| 643 | As far as I can tell, there should have been break statement somewhere there. Probably, we could still allow merging down multiple layer, but just don't delete them? | |
| libs/image/kis_layer_utils.cpp | ||
|---|---|---|
| 687 | As far as I understand this check was used to support the previous check. Could you check if the merging four layers, two of which are locked still works correctly? By "correctly" I mean: one layer is added, two removed and two kept intact. | |
Hi, @nsmirnov!
The patch itself works fine now. I have just reread the original task (T1587) and it says that the layers, used for the source of the merge but not deleted, should also be made hidden...
You can implement that by creating a new command class in KisLayerUtils and applying this command inside CleanUpNodes::populateChildCommands()
This patch has a major bug: the visibility change is not undone when pressing Ctrl+Z after doing the merge. To overcome this issue, one should create a new undo-command in KisLayerUtils and apply this command inside CleanUpNodes::populateChildCommands()
Hi, @nsmirnov!
There is still a small bug: if a layer is invisible and locked, it gets deleted when doing multinode merge-down. I guess it happens somewhere in filterInvisibleNodes
| libs/image/commands/kis_image_change_visibility_command.cpp | ||
|---|---|---|
| 10 | I guess you can avoid inheriting from KisImageCommand and inherit directly from KUndo2Command, you don't need any updates here :) | |
| libs/image/kis_layer_utils.cpp | ||
| 687 | ++it for iterators :) | |