Skip to content
  • Agata Cacko's avatar
    Fix assert on New Layer From Visible on invisible active layer · c1ab1ee9
    Agata Cacko authored
    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.
    
    CCBUG:359707
    CCBUG:395903
    CCBUG:406697
    
    BUG:428683
    c1ab1ee9