From 5bc432f289807a2231325a9addc77fe1ca8b0a58 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=ADn=20Capello?= <martin@igara.com>
Date: Wed, 7 Jun 2023 10:37:39 -0300
Subject: [PATCH] Delete unused tilesets after deleting tilemaps (fix #3876)

---
 data/pref.xml                         |   3 +
 data/strings/en.ini                   |   8 ++
 data/widgets/options.xml              |   2 +
 src/app/cmd/add_tileset.cpp           |   2 +-
 src/app/commands/cmd_remove_layer.cpp |  76 ++++++++++++++++++
 src/doc/layer.cpp                     |  11 +++
 src/doc/layer.h                       |   1 +
 src/doc/selected_layers.cpp           |  18 +++++
 src/doc/selected_layers.h             |   1 +
 src/doc/sprite.cpp                    |   7 ++
 src/doc/sprite.h                      |   1 +
 src/doc/tileset.cpp                   |  14 ++++
 src/doc/tileset.h                     |   3 +
 src/doc/tilesets.h                    |  39 +++++++--
 tests/scripts/tilemap.lua             | 110 ++++++++++++++++++++++++++
 15 files changed, 287 insertions(+), 9 deletions(-)

diff --git a/data/pref.xml b/data/pref.xml
index b4b82c31d..16f1110f7 100644
--- a/data/pref.xml
+++ b/data/pref.xml
@@ -429,6 +429,9 @@
       <option id="base_index" type="int" default="1" />
       <option id="cache_compressed_tilesets" type="bool" default="true" />
     </section>
+    <section id="tilemap">
+      <option id="show_delete_unused_tileset_alert" type="bool" default="true" />
+    </section>
   </global>
 
   <tool>
diff --git a/data/strings/en.ini b/data/strings/en.ini
index 66ad96ebb..d29c47d1b 100644
--- a/data/strings/en.ini
+++ b/data/strings/en.ini
@@ -55,6 +55,13 @@ Automatic Remap
 ||&OK||&Cancel"
 END
 cannot_delete_all_layers = Error<<You cannot delete all layers.||&OK
+deleting_tilemaps_will_delete_tilesets = <<<END
+Warning
+<<Deleting the following layers will delete their tilesets:
+<<'{0}'
+<<Do you want to continue anyway?
+||&OK||&Cancel
+END
 cannot_file_overwrite_on_export = <<<END
 Overwrite Warning
 <<You cannot Export with the same name (overwrite the original file).
@@ -1698,6 +1705,7 @@ file_format_doesnt_support_alert = Show warning when saving a file with unsuppor
 export_animation_in_sequence_alert = Show warning when saving an animation as a sequence of static images
 overwrite_files_on_export_alert = Show warning when overwriting files on File > Export
 overwrite_files_on_export_sprite_sheet_alert = Show warning when overwriting files on Export Sprite Sheet
+delete_tilemap_delete_unused_tileset_alert = Show warning when deleting a tilemap will delete its unused tileset
 image_format_alerts = Show options when saving files:
 advanced_mode_alert = Show alert when we enter to Advanced Mode
 invalid_fg_bg_color_alert = Show alert when drawing with index out of palette bounds
diff --git a/data/widgets/options.xml b/data/widgets/options.xml
index 9b7da391c..4e0504821 100644
--- a/data/widgets/options.xml
+++ b/data/widgets/options.xml
@@ -458,6 +458,8 @@
                  pref="export_file.show_overwrite_files_alert" />
           <check id="overwrite_files_on_export_sprite_sheet_alert" text="@.overwrite_files_on_export_sprite_sheet_alert"
                  pref="sprite_sheet.show_overwrite_files_alert" />
+          <check id="delete_tilemap_delete_unused_tileset_alert" text="@.delete_tilemap_delete_unused_tileset_alert"
+                 pref="tilemap.show_delete_unused_tileset_alert" />
           <check id="advanced_mode_alert" text="@.advanced_mode_alert"
                  pref="advanced_mode.show_alert" />
           <check id="invalid_fg_bg_color_alert" text="@.invalid_fg_bg_color_alert"
diff --git a/src/app/cmd/add_tileset.cpp b/src/app/cmd/add_tileset.cpp
index 3f50d23d7..fb6cbaeec 100644
--- a/src/app/cmd/add_tileset.cpp
+++ b/src/app/cmd/add_tileset.cpp
@@ -78,7 +78,7 @@ void AddTileset::addTileset(doc::Tileset* tileset)
   if (m_tilesetIndex == -1)
     m_tilesetIndex = sprite->tilesets()->add(tileset);
   else
-    sprite->tilesets()->set(m_tilesetIndex, tileset);
+    sprite->tilesets()->add(m_tilesetIndex, tileset);
 
   sprite->incrementVersion();
   sprite->tilesets()->incrementVersion();
diff --git a/src/app/commands/cmd_remove_layer.cpp b/src/app/commands/cmd_remove_layer.cpp
index 6e4944fc0..bbced5df3 100644
--- a/src/app/commands/cmd_remove_layer.cpp
+++ b/src/app/commands/cmd_remove_layer.cpp
@@ -11,20 +11,77 @@
 
 #include "app/app.h"
 #include "app/commands/command.h"
+#include "app/cmd/remove_tileset.h"
 #include "app/context_access.h"
 #include "app/doc_api.h"
 #include "app/i18n/strings.h"
 #include "app/modules/gui.h"
 #include "app/tx.h"
+#include "app/pref/preferences.h"
+#include "app/ui/optional_alert.h"
 #include "app/ui/status_bar.h"
 #include "doc/layer.h"
+#include "doc/layer_tilemap.h"
 #include "doc/sprite.h"
+#include "doc/tilesets.h"
 #include "fmt/format.h"
 #include "ui/alert.h"
 #include "ui/widget.h"
 
 namespace app {
 
+// Calculates the list of unused tileset indexes (returned in tsiToDelete parameter)
+// once the layers specified are removed.
+// Also, if the UI is available, shows a warning message about the deletion of unused
+// tilesets.
+// This function returns true in any of the following:
+// - There won't be deletion of tilesets, this means tsiToDelete is empty.
+// - The user accepts continuing despite the warning.
+// - There is no UI available.
+static bool continue_deleting_unused_tilesets(
+  Context* ctx, Sprite* sprite, const LayerList layers,
+  std::set<tileset_index, std::greater<tileset_index>>& tsiToDelete)
+{
+  std::vector<LayerTilemap*> tilemaps;
+  std::map<doc::tileset_index, int> timesSelected;
+  std::string layerNames;
+  for (auto layer : layers) {
+    if (layer->isTilemap()) {
+      auto tilemap = static_cast<LayerTilemap*>(layer);
+      timesSelected[tilemap->tilesetIndex()]++;
+      tilemaps.push_back(tilemap);
+    }
+  }
+  for (auto tilemap : tilemaps) {
+    auto ts = sprite->tilesets()->get(tilemap->tilesetIndex());
+    if (ts->tilemapsCount() == timesSelected[tilemap->tilesetIndex()]) {
+      tsiToDelete.insert(tilemap->tilesetIndex());
+      layerNames += tilemap->name() + ", ";
+    }
+  }
+
+#ifdef ENABLE_UI
+  // Just continue if UI is not available.
+  if (!ctx->isUIAvailable())
+    return true;
+
+  // Remove last ", "
+  if (!layerNames.empty()) {
+    layerNames = layerNames.substr(0, layerNames.length() - 2);
+  }
+
+  std::string message;
+  if (tsiToDelete.size() >= 1)
+    message = fmt::format(Strings::alerts_deleting_tilemaps_will_delete_tilesets(), layerNames);
+
+  return tsiToDelete.empty() ||
+         app::OptionalAlert::show(
+          Preferences::instance().tilemap.showDeleteUnusedTilesetAlert, 1, message) == 1;
+#else
+  return true;
+#endif
+}
+
 class RemoveLayerCommand : public Command {
 public:
   RemoveLayerCommand();
@@ -55,6 +112,11 @@ void RemoveLayerCommand::onExecute(Context* context)
   {
     Tx tx(writer.context(), "Remove Layer");
     DocApi api = document->getApi(tx);
+    // We need to remove all the tilesets after the tilemaps are deleted
+    // and in descending tileset index order, otherwise the tileset indexes
+    // get mixed up. This is the reason we use a tileset_index set with
+    // the std::greater Compare.
+    std::set<tileset_index, std::greater<tileset_index>> tsiToDelete;
 
     const Site* site = writer.site();
     if (site->inTimeline() &&
@@ -73,6 +135,10 @@ void RemoveLayerCommand::onExecute(Context* context)
         return;
       }
 
+      if (!continue_deleting_unused_tilesets(context, sprite, selLayers.toAllTilemaps(), tsiToDelete)) {
+        return;
+      }
+
       for (Layer* layer : selLayers) {
         api.removeLayer(layer);
       }
@@ -84,10 +150,20 @@ void RemoveLayerCommand::onExecute(Context* context)
       }
 
       Layer* layer = writer.layer();
+      if (layer->isTilemap() && !continue_deleting_unused_tilesets(context, sprite, {layer}, tsiToDelete)) {
+        return;
+      }
+
       layerName = layer->name();
       api.removeLayer(layer);
     }
 
+    if (!tsiToDelete.empty()) {
+      for (tileset_index tsi : tsiToDelete) {
+        tx(new cmd::RemoveTileset(sprite, tsi));
+      }
+    }
+
     tx.commit();
   }
 
diff --git a/src/doc/layer.cpp b/src/doc/layer.cpp
index 8bbf61014..734b93c43 100644
--- a/src/doc/layer.cpp
+++ b/src/doc/layer.cpp
@@ -529,6 +529,17 @@ void LayerGroup::allBrowsableLayers(LayerList& list) const
   }
 }
 
+void LayerGroup::allTilemaps(LayerList& list) const
+{
+  for (Layer* child : m_layers) {
+    if (child->isGroup())
+      static_cast<LayerGroup*>(child)->allTilemaps(list);
+
+    if (child->isTilemap())
+      list.push_back(child);
+  }
+}
+
 void LayerGroup::getCels(CelList& cels) const
 {
   for (const Layer* layer : m_layers)
diff --git a/src/doc/layer.h b/src/doc/layer.h
index b8e153f3d..dc291673f 100644
--- a/src/doc/layer.h
+++ b/src/doc/layer.h
@@ -211,6 +211,7 @@ namespace doc {
     void allVisibleLayers(LayerList& list) const;
     void allVisibleReferenceLayers(LayerList& list) const;
     void allBrowsableLayers(LayerList& list) const;
+    void allTilemaps(LayerList& list) const;
 
     void getCels(CelList& cels) const override;
     void displaceFrames(frame_t fromThis, frame_t delta) override;
diff --git a/src/doc/selected_layers.cpp b/src/doc/selected_layers.cpp
index 3fa67d3d0..567b40162 100644
--- a/src/doc/selected_layers.cpp
+++ b/src/doc/selected_layers.cpp
@@ -99,6 +99,24 @@ LayerList SelectedLayers::toBrowsableLayerList() const
   return output;
 }
 
+LayerList SelectedLayers::toAllTilemaps() const
+{
+  LayerList output;
+
+  if (empty())
+    return output;
+
+  for (Layer* layer : *this) {
+    if (layer->isGroup()) {
+      auto group = static_cast<LayerGroup*>(layer);
+      group->allTilemaps(output);
+    } else if (layer->isTilemap())
+      output.push_back(layer);
+  }
+
+  return output;
+}
+
 void SelectedLayers::removeChildrenIfParentIsSelected()
 {
   SelectedLayers removeThese;
diff --git a/src/doc/selected_layers.h b/src/doc/selected_layers.h
index 81228c959..92bcf4298 100644
--- a/src/doc/selected_layers.h
+++ b/src/doc/selected_layers.h
@@ -41,6 +41,7 @@ namespace doc {
     bool hasSameParent() const;
     LayerList toBrowsableLayerList() const;
     LayerList toAllLayersList() const;
+    LayerList toAllTilemaps() const;
 
     void removeChildrenIfParentIsSelected();
     void expandCollapsedGroups();
diff --git a/src/doc/sprite.cpp b/src/doc/sprite.cpp
index 0f7ede387..68623fdb6 100644
--- a/src/doc/sprite.cpp
+++ b/src/doc/sprite.cpp
@@ -736,6 +736,13 @@ LayerList Sprite::allBrowsableLayers() const
   return list;
 }
 
+LayerList Sprite::allTilemaps() const
+{
+  LayerList list;
+  m_root->allTilemaps(list);
+  return list;
+}
+
 CelsRange Sprite::cels() const
 {
   SelectedFrames selFrames;
diff --git a/src/doc/sprite.h b/src/doc/sprite.h
index a91d7fde3..12efdf741 100644
--- a/src/doc/sprite.h
+++ b/src/doc/sprite.h
@@ -209,6 +209,7 @@ namespace doc {
     LayerList allVisibleLayers() const;
     LayerList allVisibleReferenceLayers() const;
     LayerList allBrowsableLayers() const;
+    LayerList allTilemaps() const;
 
     CelsRange cels() const;
     CelsRange cels(frame_t frame) const;
diff --git a/src/doc/tileset.cpp b/src/doc/tileset.cpp
index ee0f035fe..e93a3717b 100644
--- a/src/doc/tileset.cpp
+++ b/src/doc/tileset.cpp
@@ -10,6 +10,9 @@
 
 #include "doc/tileset.h"
 
+#include "doc/tilesets.h"
+#include "doc/layer.h"
+#include "doc/layer_tilemap.h"
 #include "base/mem_utils.h"
 #include "doc/primitives.h"
 #include "doc/remap.h"
@@ -406,4 +409,15 @@ TilesetHashTable& Tileset::hashTable()
   return m_hash;
 }
 
+int Tileset::tilemapsCount() const {
+  auto tsi = sprite()->tilesets()->getIndex(this);
+  int count = 0;
+  for (auto layer : sprite()->allLayers()) {
+    if (layer->isTilemap() && static_cast<LayerTilemap*>(layer)->tilesetIndex() == tsi) {
+      count++;
+    }
+  }
+  return count;
+}
+
 } // namespace doc
diff --git a/src/doc/tileset.h b/src/doc/tileset.h
index 58f656006..3be9a8eed 100644
--- a/src/doc/tileset.h
+++ b/src/doc/tileset.h
@@ -131,6 +131,9 @@ namespace doc {
     // have to regenerate the empty tile with that new mask color.
     void notifyRegenerateEmptyTile();
 
+    // Returns the number of tilemap layers that are referencing this tileset.
+    int tilemapsCount() const;
+
 #ifdef _DEBUG
     void assertValidHashTable();
 #endif
diff --git a/src/doc/tilesets.h b/src/doc/tilesets.h
index 4155a70a0..355128cbd 100644
--- a/src/doc/tilesets.h
+++ b/src/doc/tilesets.h
@@ -8,6 +8,8 @@
 #define DOC_TILESETS_H_INCLUDED
 #pragma once
 
+#include "doc/layer_tilemap.h"
+#include "doc/sprite.h"
 #include "doc/tileset.h"
 
 #include <vector>
@@ -40,7 +42,7 @@ namespace doc {
         return nullptr;
     }
 
-    tileset_index getIndex(Tileset *tileset) {
+    tileset_index getIndex(const Tileset *tileset) {
       for (tileset_index i = 0; i < size(); ++i) {
         if (m_tilesets[i] == tileset) {
           return i;
@@ -55,23 +57,44 @@ namespace doc {
       m_tilesets[tsi] = tileset;
     }
 
+    void add(const tileset_index tsi, Tileset* tileset) {
+      if (tsi >= m_tilesets.size()) {
+        m_tilesets.push_back(tileset);
+      }
+      else {
+        m_tilesets.insert(m_tilesets.begin()+tsi, tileset);
+        // Update tileset indexes of the affected tilemaps. We have to shift the indexes
+        // for all the tilemaps pointing to a tileset index equals or greater than the added one.
+        shiftTilesetIndexes(tileset->sprite(), tsi, 1);
+      }
+    }
+
     void erase(const tileset_index tsi) {
-      // Do not m_tilesets.erase() the tileset so other tilesets
-      // indexes/IDs are kept intact.
+      // When tsi is the last one, other tilemaps tilesets
+      // indexes are not affected.
       if (tsi == size()-1) {
         m_tilesets.erase(--m_tilesets.end());
       }
       else {
-        // TODO Should we keep the empty slot? Or should we update all
-        //      indexes (even from external files?). Having a nullptr
-        //      tileset in the sprite adds a lot of complexity (each
-        //      for-loop must check the tileset)
-        m_tilesets[tsi] = nullptr;
+        auto ts = m_tilesets[tsi];
+        m_tilesets.erase(m_tilesets.begin()+tsi);
+        // Update tileset indexes of the affected tilemaps. We have to shift the indexes
+        // for all the tilemaps pointing to a tileset index greater than the deleted one.
+        shiftTilesetIndexes(ts->sprite(), tsi+1, -1);
       }
     }
 
   private:
     Array m_tilesets;
+
+    void shiftTilesetIndexes(Sprite *sprite, tileset_index pos, int n) {
+      for (auto layer : sprite->allTilemaps()) {
+        auto tilemap = static_cast<LayerTilemap*>(layer);
+        if (tilemap->tilesetIndex() >= pos) {
+          tilemap->setTilesetIndex(tilemap->tilesetIndex()+n);
+        }
+      }
+    }
   };
 
 } // namespace doc
diff --git a/tests/scripts/tilemap.lua b/tests/scripts/tilemap.lua
index babf48467..af1acec37 100644
--- a/tests/scripts/tilemap.lua
+++ b/tests/scripts/tilemap.lua
@@ -1117,3 +1117,113 @@ do
   expect_eq(app.activeLayer.cels[1].image.width, 1) -- width in tilemap terms
   expect_eq(app.activeLayer.cels[1].image.height, 2) -- height in tilemap terms
 end
+
+----------------------------------------------------------------------
+-- Tests removal of unused tilesets when deleting tilemaps
+----------------------------------------------------------------------
+
+do
+  local spr = Sprite(32, 32, ColorMode.INDEXED)
+  assert(spr.layers[1].isImage)
+  assert(not spr.layers[1].isTilemap)
+
+  -- Create some tilemaps
+  app.command.NewLayer{ tilemap=true }
+  assert(#spr.layers == 2)
+  local tilemapLay1 = spr.layers[2]
+  assert(tilemapLay1.isImage)
+  assert(tilemapLay1.isTilemap)
+  assert(#spr.tilesets == 1)
+  assert(spr.tilesets[1] == tilemapLay1.tileset)
+
+  app.command.NewLayer{ tilemap=true }
+  assert(#spr.layers == 3)
+  local tilemapLay2 = spr.layers[3]
+  assert(tilemapLay2.isImage)
+  assert(tilemapLay2.isTilemap)
+  assert(#spr.tilesets == 2)
+  assert(spr.tilesets[2] == tilemapLay2.tileset)
+
+  app.command.NewLayer{ tilemap=true }
+  assert(#spr.layers == 4)
+  local tilemapLay3 = spr.layers[4]
+  assert(tilemapLay3.isImage)
+  assert(tilemapLay3.isTilemap)
+  assert(#spr.tilesets == 3)
+  assert(spr.tilesets[3] == tilemapLay3.tileset)
+
+  -- Remove tilemap 2 and check that a tilemap was removed and
+  -- tilesets of remaining tilemaps are correct.
+  app.range.layers = { tilemapLay2 }
+  app.command.RemoveLayer()
+  assert(#spr.layers == 3)
+  assert(#spr.tilesets == 2)
+  assert(spr.tilesets[1] == tilemapLay1.tileset)
+  assert(spr.tilesets[2] == tilemapLay3.tileset)
+
+  -- Undo tilemap removal and check that it goes back to
+  -- previous state.
+  app.undo()
+
+  assert(#spr.layers == 4)
+  assert(#spr.tilesets == 3)
+  assert(spr.tilesets[1] == tilemapLay1.tileset)
+  assert(spr.tilesets[2] == tilemapLay2.tileset)
+  assert(spr.tilesets[3] == tilemapLay3.tileset)
+
+  -- Try removing 2 tilemaps now
+  app.range.layers = { tilemapLay1, tilemapLay2 }
+  app.command.RemoveLayer()
+  assert(#spr.layers == 2)
+  assert(#spr.tilesets == 1)
+  assert(spr.tilesets[1] == tilemapLay3.tileset)
+
+  app.undo()
+
+  assert(#spr.layers == 4)
+  assert(#spr.tilesets == 3)
+  assert(spr.tilesets[1] == tilemapLay1.tileset)
+  assert(spr.tilesets[2] == tilemapLay2.tileset)
+  assert(spr.tilesets[3] == tilemapLay3.tileset)
+
+  -- Assign same tileset to tilemap 1 and tilemap 3.
+  local oldTilemapLay3Tileset = tilemapLay3.tileset
+  tilemapLay3.tileset = tilemapLay1.tileset
+  -- We have to manually delete tilemap 3 tileset because
+  -- assigning a different tileset doesn't check for/remove
+  -- unused tilesets (TODO: should we add this?)
+  spr:deleteTileset(oldTilemapLay3Tileset)
+
+  assert(#spr.tilesets == 2)
+  assert(spr.tilesets[1] == tilemapLay1.tileset)
+  assert(spr.tilesets[2] == tilemapLay2.tileset)
+  assert(spr.tilesets[1] == tilemapLay3.tileset)
+
+  -- Remove tilemap 1 and check that no tileset was removed.
+  app.range.layers = { tilemapLay1 }
+  app.command.RemoveLayer()
+  assert(#spr.layers == 3)
+  assert(#spr.tilesets == 2)
+  assert(spr.tilesets[2] == tilemapLay2.tileset)
+  assert(spr.tilesets[1] == tilemapLay3.tileset)
+
+  -- Remove tilemap 3 and check that the tileset was removed now.
+  app.range.layers = { tilemapLay3 }
+  app.command.RemoveLayer()
+  assert(#spr.layers == 2)
+  assert(#spr.tilesets == 1)
+  assert(spr.tilesets[1] == tilemapLay2.tileset)
+
+  -- Undo all
+  app.undo()
+  app.undo()
+  app.undo()
+  -- Manually re-assign its tileset to tilemap 3.
+  tilemapLay3.tileset = spr.tilesets[3]
+
+  assert(#spr.layers == 4)
+  assert(#spr.tilesets == 3)
+  assert(spr.tilesets[1] == tilemapLay1.tileset)
+  assert(spr.tilesets[2] == tilemapLay2.tileset)
+  assert(spr.tilesets[3] == tilemapLay3.tileset)
+end
\ No newline at end of file