Fix crash clicking and deleting layers from timeline sequentially (and quickly

Reported in https://igarastudio.zendesk.com/agent/tickets/656

src/app/CMakeLists.txt

@@ -599,6 +599,7 @@ add_library(app-lib
   util/filetoks.cpp
   util/freetype_utils.cpp
   util/layer_boundaries.cpp
+  util/layer_utils.cpp
   util/msk_file.cpp
   util/new_image_from_mask.cpp
   util/pal_ops.cpp

src/app/active_site_handler.cpp

@@ -1,5 +1,5 @@
 // Aseprite
-// Copyright (C) 2019  Igara Studio S.A.
+// Copyright (C) 2019-2020  Igara Studio S.A.
 //
 // This program is distributed under the terms of
 // the End-User License Agreement for Aseprite.
@@ -12,6 +12,7 @@
 #include "app/doc.h"
 #include "app/doc_event.h"
 #include "app/site.h"
+#include "app/util/layer_utils.h"
 #include "doc/layer.h"
 
 namespace app {
@@ -97,30 +98,20 @@ void ActiveSiteHandler::onAddFrame(DocEvent& ev)
   data.frame = ev.frame();
 }
 
-// TODO similar to Timeline::onBeforeRemoveLayer()
+// TODO similar to Timeline::onBeforeRemoveLayer() and Editor::onBeforeRemoveLayer()
 void ActiveSiteHandler::onBeforeRemoveLayer(DocEvent& ev)
 {
   Data& data = getData(ev.document());
+  doc::Layer* selectedLayer = (data.layer != doc::NullId ?
+                               doc::get<doc::Layer>(data.layer):
+                               nullptr);
+  if (!selectedLayer)
+    return;
 
-  Layer* layer = ev.layer();
-
-  // If the layer that was removed is the selected one
-  ASSERT(layer);
-  if (layer && data.layer == layer->id()) {
-    LayerGroup* parent = layer->parent();
-    Layer* layer_select = nullptr;
-
-    // Select previous layer, or next layer, or the parent (if it is
-    // not the main layer of sprite set).
-    if (layer->getPrevious()) {
-      layer_select = layer->getPrevious();
-    }
-    else if (layer->getNext())
-      layer_select = layer->getNext();
-    else if (parent != layer->sprite()->root())
-      layer_select = parent;
-
-    data.layer = (layer_select ? layer_select->id(): 0);
+  doc::Layer* layerToSelect = candidate_if_layer_is_deleted(selectedLayer, ev.layer());
+  if (selectedLayer != layerToSelect) {
+    data.layer = (layerToSelect ? layerToSelect->id():
+                                  doc::NullId);
   }
 }
 

src/app/cmd_transaction.cpp

@@ -1,5 +1,5 @@
 // Aseprite
-// Copyright (C) 2019  Igara Studio S.A.
+// Copyright (C) 2019-2020  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -88,10 +88,7 @@ std::istream* CmdTransaction::documentRangeAfterExecute() const
 
 void CmdTransaction::onExecute()
 {
-  CmdSequence::onExecute();
-
-  // The execution of CmdTransaction is called by Transaction at the
-  // very beginning, just to save the current sprite position.
+  // Save the current site and doc range
   m_spritePositionBefore = calcSpritePosition();
 #ifdef ENABLE_UI
   if (isDocRangeEnabled()) {
@@ -100,6 +97,9 @@ void CmdTransaction::onExecute()
   }
 #endif
 
+  // Execute the sequence of "cmds"
+  CmdSequence::onExecute();
+
   if (m_changeSavedState)
     ++(*m_savedCounter);
 }

src/app/doc_range.cpp

@@ -1,4 +1,5 @@
 // Aseprite
+// Copyright (C) 2020  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -10,6 +11,7 @@
 
 #include "app/doc_range.h"
 
+#include "app/util/layer_utils.h"
 #include "base/serialization.h"
 #include "doc/cel.h"
 #include "doc/image.h"
@@ -93,6 +95,23 @@ void DocRange::selectLayers(const SelectedLayers& selLayers)
     m_selectedLayers.insert(layer);
 }
 
+void DocRange::eraseAndAdjust(const Layer* layer)
+{
+  if (!enabled())
+    return;
+
+  if (m_selectingFromLayer)
+    m_selectingFromLayer = candidate_if_layer_is_deleted(m_selectingFromLayer, layer);
+
+  SelectedLayers copy = m_selectedLayers;
+  m_selectedLayers.clear();
+  for (Layer* selectedLayer : copy) {
+    Layer* layerToSelect = candidate_if_layer_is_deleted(selectedLayer, layer);
+    if (layerToSelect)
+      m_selectedLayers.insert(layerToSelect);
+  }
+}
+
 bool DocRange::contains(const Layer* layer) const
 {
   if (enabled())

src/app/doc_range.h

@@ -1,4 +1,5 @@
 // Aseprite
+// Copyright (C) 2020  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -48,6 +49,12 @@ namespace app {
     bool contains(const Layer* layer,
                   const frame_t frame) const;
 
+    // If the range includes the given layer, it will be erased from
+    // the selection and other candidates might be selected (e.g. a
+    // sibling, parent, etc.) using the
+    // candidate_if_layer_is_deleted() function.
+    void eraseAndAdjust(const Layer* layer);
+
     void clearRange();
     void startRange(Layer* fromLayer, frame_t fromFrame, Type type);
     void endRange(Layer* toLayer, frame_t toFrame);

src/app/ui/doc_view.cpp

@@ -394,33 +394,6 @@ void DocView::onAddLayer(DocEvent& ev)
   }
 }
 
-// TODO why note move this code to Editor::onBeforeRemoveLayer?
-void DocView::onBeforeRemoveLayer(DocEvent& ev)
-{
-  Sprite* sprite = ev.sprite();
-  Layer* layer = ev.layer();
-
-  // If the layer that was removed is the selected one in the editor,
-  // or is an ancestor of the selected one.
-  if ((m_editor->layer() == layer) ||
-      (m_editor->layer() &&
-       m_editor->layer()->hasAncestor(layer))) {
-    LayerGroup* parent = layer->parent();
-    Layer* layer_select = NULL;
-
-    // Select previous layer, or next layer, or the parent (if it is
-    // not the main layer of sprite set).
-    if (layer->getPrevious())
-      layer_select = layer->getPrevious();
-    else if (layer->getNext())
-      layer_select = layer->getNext();
-    else if (parent != sprite->root())
-      layer_select = parent;
-
-    m_editor->setLayer(layer_select);
-  }
-}
-
 void DocView::onAddFrame(DocEvent& ev)
 {
   if (current_editor == m_editor)

src/app/ui/doc_view.h

@@ -1,4 +1,5 @@
 // Aseprite
+// Copyright (C) 2020  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -72,7 +73,6 @@ namespace app {
     void onSpritePixelsModified(DocEvent& ev) override;
     void onLayerMergedDown(DocEvent& ev) override;
     void onAddLayer(DocEvent& ev) override;
-    void onBeforeRemoveLayer(DocEvent& ev) override;
     void onAddFrame(DocEvent& ev) override;
     void onRemoveFrame(DocEvent& ev) override;
     void onAddCel(DocEvent& ev) override;

src/app/ui/editor/editor.cpp

@@ -51,6 +51,7 @@
 #include "app/ui/timeline/timeline.h"
 #include "app/ui/toolbar.h"
 #include "app/ui_context.h"
+#include "app/util/layer_utils.h"
 #include "base/bind.h"
 #include "base/chrono.h"
 #include "base/clamp.h"
@@ -2141,9 +2142,16 @@ void Editor::onSpritePixelRatioChanged(DocEvent& ev)
   invalidate();
 }
 
+// TODO similar to ActiveSiteHandler::onBeforeRemoveLayer() and Timeline::onBeforeRemoveLayer()
 void Editor::onBeforeRemoveLayer(DocEvent& ev)
 {
   m_showGuidesThisCel = nullptr;
+
+  // If the layer that was removed is the selected one in the editor,
+  // or is an ancestor of the selected one.
+  Layer* layerToSelect = candidate_if_layer_is_deleted(layer(), ev.layer());
+  if (layer() != layerToSelect)
+    setLayer(layerToSelect);
 }
 
 void Editor::onRemoveCel(DocEvent& ev)

src/app/ui/timeline/timeline.cpp

@@ -1,5 +1,5 @@
 // Aseprite
-// Copyright (C) 2018-2019  Igara Studio S.A.
+// Copyright (C) 2018-2020  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -42,6 +42,7 @@
 #include "app/ui_context.h"
 #include "app/util/clipboard.h"
 #include "app/util/layer_boundaries.h"
+#include "app/util/layer_utils.h"
 #include "app/util/readable_time.h"
 #include "base/bind.h"
 #include "base/clamp.h"
@@ -1730,28 +1731,21 @@ void Timeline::onAddLayer(DocEvent& ev)
   invalidate();
 }
 
-// TODO similar to ActiveSiteHandler::onBeforeRemoveLayer()
+// TODO similar to ActiveSiteHandler::onBeforeRemoveLayer() and Editor::onBeforeRemoveLayer()
 void Timeline::onBeforeRemoveLayer(DocEvent& ev)
 {
-  Sprite* sprite = ev.sprite();
-  Layer* layer = ev.layer();
+  Layer* layerToSelect = candidate_if_layer_is_deleted(m_layer, ev.layer());
+  if (m_layer != layerToSelect)
+    setLayer(layerToSelect);
 
-  // If the layer that was removed is the selected one
-  if (layer == getLayer()) {
-    LayerGroup* parent = layer->parent();
-    Layer* layer_select = NULL;
+  // Remove layer from ranges
+  m_range.eraseAndAdjust(ev.layer());
+  m_startRange.eraseAndAdjust(ev.layer());
+  m_dropRange.eraseAndAdjust(ev.layer());
 
-    // Select previous layer, or next layer, or the parent (if it is
-    // not the main layer of sprite set).
-    if (layer->getPrevious())
-      layer_select = layer->getPrevious();
-    else if (layer->getNext())
-      layer_select = layer->getNext();
-    else if (parent != sprite->root())
-      layer_select = parent;
-
-    setLayer(layer_select);
-  }
+  ASSERT(!m_range.contains(ev.layer()));
+  ASSERT(!m_startRange.contains(ev.layer()));
+  ASSERT(!m_dropRange.contains(ev.layer()));
 }
 
 // We have to regenerate the layer rows (m_rows) after the layer is

src/app/util/layer_utils.cpp

@@ -0,0 +1,41 @@
+// Aseprite
+// Copyright (C) 2020  Igara Studio S.A.
+//
+// This program is distributed under the terms of
+// the End-User License Agreement for Aseprite.
+
+#include "app/util/layer_utils.h"
+
+#include "doc/layer.h"
+#include "doc/sprite.h"
+
+namespace app {
+
+using namespace doc;
+
+Layer* candidate_if_layer_is_deleted(
+  const Layer* selectedLayer,
+  const Layer* layerToDelete)
+{
+  const Layer* layerToSelect = selectedLayer;
+
+  if ((selectedLayer == layerToDelete) ||
+      (selectedLayer &&
+       selectedLayer->hasAncestor(layerToDelete))) {
+    Sprite* sprite = selectedLayer->sprite();
+    LayerGroup* parent = layerToDelete->parent();
+
+    // Select previous layer, or next layer, or the parent (if it is
+    // not the main layer of sprite set).
+    if (layerToDelete->getPrevious())
+      layerToSelect = layerToDelete->getPrevious();
+    else if (layerToDelete->getNext())
+      layerToSelect = layerToDelete->getNext();
+    else if (parent != sprite->root())
+      layerToSelect = parent;
+  }
+
+  return const_cast<Layer*>(layerToSelect);
+}
+
+} // namespace app

src/app/util/layer_utils.h

@@ -0,0 +1,26 @@
+// Aseprite
+// Copyright (C) 2020  Igara Studio S.A.
+//
+// This program is distributed under the terms of
+// the End-User License Agreement for Aseprite.
+
+#ifndef APP_LAYER_UTILS_H_INCLUDED
+#define APP_LAYER_UTILS_H_INCLUDED
+#pragma once
+
+namespace doc {
+  class Layer;
+}
+
+namespace app {
+
+  // Calculates a possible candidate to be selected in case that we
+  // have a specific "selectedLayer" and are going to delete the given
+  // "layerToDelete".
+  doc::Layer* candidate_if_layer_is_deleted(
+    const doc::Layer* selectedLayer,
+    const doc::Layer* layerToDelete);
+
+} // namespace app
+
+#endif

src/doc/selected_layers.cpp

@@ -1,5 +1,6 @@
 // Aseprite Document Library
-// Copyright (c) 2016, 2018 David Capello
+// Copyright (C) 2020  Igara Studio S.A.
+// Copyright (C) 2016-2018  David Capello
 //
 // This file is released under the terms of the MIT license.
 // Read LICENSE.txt for more information.
@@ -34,14 +35,14 @@ void SelectedLayers::insert(Layer* layer)
   m_set.insert(layer);
 }
 
-void SelectedLayers::erase(Layer* layer)
+void SelectedLayers::erase(const Layer* layer)
 {
-  m_set.erase(layer);
+  m_set.erase(const_cast<Layer*>(layer));
 }
 
-bool SelectedLayers::contains(Layer* layer) const
+bool SelectedLayers::contains(const Layer* layer) const
 {
-  return m_set.find(layer) != m_set.end();
+  return m_set.find(const_cast<Layer*>(layer)) != m_set.end();
 }
 
 bool SelectedLayers::hasSameParent() const

src/doc/selected_layers.h

@@ -1,5 +1,6 @@
 // Aseprite Document Library
-// Copyright (c) 2016-2018 David Capello
+// Copyright (C) 2020  Igara Studio S.A.
+// Copyright (C) 2016-2018  David Capello
 //
 // This file is released under the terms of the MIT license.
 // Read LICENSE.txt for more information.
@@ -34,9 +35,9 @@ namespace doc {
 
     void clear();
     void insert(Layer* layer);
-    void erase(Layer* layer);
+    void erase(const Layer* layer);
 
-    bool contains(Layer* layer) const;
+    bool contains(const Layer* layer) const;
     bool hasSameParent() const;
     LayerList toLayerList() const;
     LayerList toAllLayersList() const;