Differential D20314

Fix for bug 397836 - undo merging a layer that is cloned
ClosedPublic
Actions

Authored by tusooaw on Apr 6 2019, 6:43 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Commits
R37:40fd2be3ee84: Fix undo merging a layer that is cloned
R37:2239e9550e35: Fix undo merging a layer that is cloned
Summary

https://bugs.kde.org/show_bug.cgi?id=397836

This patch fixed this bug.

Under my implementation, if we are merging a layer that is the source of a clone layer, the clone layer will become the clone of the merged layer.
Under this implementation, merging down will keep all its current behaviours (clones turn into paint layers).

Not only merging, but deleting nodes is also affected.

The real cause of this bug is that when removing a layer, we are using the prevAbove pointer to indicate the position where it has been removed from. In current master, all its clones are converted into paint layers when removing the layer. If the prevAbove pointer is just a clone of the layer to remove, when we restore the layer, KisNode will just refuse to add the layer since prevAbove is not in the parent now (it has been removed and replaced with a paint layer).

Test Plan

As the "correct behavior" in the bug link. However, the original behaviour (clones turn into paint layers) will not change. The canvas will not change in its look.

Also,

  1. Add two paint layers. Assume they are "Layer 1" and "Layer 2," from bottom to top.
  2. Add a clone layer copying from "Layer 1." Assume the clone is called "Layer 3."
  3. Move "Layer 3" below "Layer 1."
  4. In the layer docker, click on "Layer 1."
  5. Hold Ctrl, click on "Layer 2."
  6. Right click, then select "Remove Layer."

    Expected: Layer 1 and Layer 2 are removed. Layer 3 turns into a Paint Layer.
  7. Undo.

    Expected: Layer 1 and Layer 2 are restored. Layer 3 goes back into a Clone Layer.

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.
tusooaw created this revision.Apr 6 2019, 6:43 PM
Restricted Application added a project: Krita. · View Herald TranscriptApr 6 2019, 6:43 PM
tusooaw requested review of this revision.Apr 6 2019, 6:43 PM
tusooaw updated this revision to Diff 55592.Apr 6 2019, 7:31 PM

remove redundant operations

tusooaw updated this revision to Diff 55602.Apr 6 2019, 9:39 PM

clear the original list in next redo

tusooaw updated this revision to Diff 55851.Apr 9 2019, 6:44 PM
tusooaw edited the summary of this revision. (Show Details)
tusooaw edited the test plan for this revision. (Show Details)

When we undo() the removal of a cloned layer, restore its clones first, in case that one clone is just below the source layer.

tusooaw added a comment.Apr 9 2019, 6:57 PM

As a side-note: it is technically enough to just swap restoreClones() and addNode() for the purpose of preventing data loss. However, when people are merging layers, chances are they think the merged layer should "inherit" all properties of the individual layers before, including clones. This is useful since we don't seem to have ways to paste things into the current layer (they go to new layers instead).

dkazakov requested changes to this revision.Apr 9 2019, 7:44 PM
dkazakov added a subscriber: dkazakov.

Hi, @tusooaw!

The patch looks fine, though there are two issues. See the attached video to see illustration of these issues:

  1. [disputable] When merging the source of the clone layer, I think, the clone layer should automatically reincarnate into a paint layer and detach from its source. At least the user will not see unexpected color blobs on the canvas.
  1. [bug] When undoing the merge, the clone layer is not updated, keeping the color blob on the canvas.

merge_down_clones-2019-04-09_22.39.32.mkv933 KBDownload

This revision now requires changes to proceed.Apr 9 2019, 7:44 PM
dkazakov added a comment.EditedApr 15 2019, 1:03 PM

Just an idea:

  1. Implement GUI for changing the source of the clone layer (we wanted to do that for years)
  2. When the user wants to keep the clone, he does the following:
    • presses Ctrl+G to group the merged layers
    • retargets the clone to this new group
    • merges the source layers (the clone is intact, because it is not connected to them)

More ideas:

  • show a dialog that "some clone layers are going to be reincarnated into paint layers"
  • or better just show a floating message
tusooaw added a comment.Apr 15 2019, 9:40 PM
In D20314#450551, @dkazakov wrote:

Just an idea:

  1. Implement GUI for changing the source of the clone layer (we wanted to do that for years)
  2. When the user wants to keep the clone, he does the following:
    • presses Ctrl+G to group the merged layers
    • retargets the clone to this new group
    • merges the source layers (the clone is intact, because it is not connected to them)

      More ideas:
  3. show a dialog that "some clone layers are going to be reincarnated into paint layers"
  4. or better just show a floating message

Oh that is a good idea. I will look into it after I finish my exams.

tusooaw updated this revision to Diff 56539.Apr 18 2019, 3:14 PM
tusooaw edited the summary of this revision. (Show Details)
tusooaw edited the test plan for this revision. (Show Details)
In D20314#450884, @tusooaw wrote:
In D20314#450551, @dkazakov wrote:

Just an idea:

  1. Implement GUI for changing the source of the clone layer (we wanted to do that for years)
  2. When the user wants to keep the clone, he does the following:
    • presses Ctrl+G to group the merged layers
    • retargets the clone to this new group
    • merges the source layers (the clone is intact, because it is not connected to them)

      More ideas:
  3. show a dialog that "some clone layers are going to be reincarnated into paint layers"
  4. or better just show a floating message

Oh that is a good idea. I will look into it after I finish my exams.

These features will be submitted in another Diff. This Diff will be only for solving data loss.

dkazakov accepted this revision.Apr 20 2019, 9:02 AM

The patch works perfectly fine now and does exactly what is declared! Please push! :)

This revision is now accepted and ready to land.Apr 20 2019, 9:02 AM
Closed by commit R37:2239e9550e35: Fix undo merging a layer that is cloned (authored by tusooaw, committed by dkazakov). · Explain WhyApr 20 2019, 12:31 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 56626

View Options

libs/image/commands/kis_image_layer_remove_command_impl.cpp

Loading...