Scripting: Changed editable wrappers ownership (#3843)

* Each editable wrapper created from C++ is now owned by the object it
  is wrapping, rather than the JS engine. This avoids them from being
  garbage collected and related issues.

* Editable wrappers created from JavaScript are instead owned by the JS
  engine, and they own the object they are wrapping.

* Where needed, ownership is transferred from C++ to JS or the other way
  around.

* Refactor: Merged Editable...::release with Editable...::attach, since 
  they served mostly the same purpose.

* Refactor: Removed EditableManager.

* Removed no longer needed Qt.qml-private dependency.

* Fixed TiledMap.selectedArea being null after assigning a map created
  from script to tiled.activeAsset.

Fixes #3565

.github/workflows/codeql-analysis.yml

@@ -44,7 +44,7 @@ jobs:
     - name: Install dependencies
       run: |
         sudo apt update
-        sudo apt install qtbase5-dev libqt5svg5-dev qttools5-dev-tools zlib1g-dev qtdeclarative5-dev qtdeclarative5-private-dev qtbase5-private-dev qbs python3-dev
+        sudo apt install qtbase5-dev libqt5svg5-dev qttools5-dev-tools zlib1g-dev qtdeclarative5-dev qbs python3-dev
 
     - name: Setup qbs
       run: |

.github/workflows/ubuntu-22.04.yml

@@ -32,7 +32,7 @@ jobs:
     - name: Install dependencies
       run: |
         sudo apt update
-        sudo apt install qtbase5-dev libqt5svg5-dev qttools5-dev-tools zlib1g-dev qtdeclarative5-dev qtdeclarative5-private-dev qtbase5-private-dev qbs python3-dev
+        sudo apt install qtbase5-dev libqt5svg5-dev qttools5-dev-tools zlib1g-dev qtdeclarative5-dev qbs python3-dev
 
     - name: Setup qbs
       run: |

README.md

@@ -45,7 +45,7 @@ Compiling Tiled
 Before you can compile Tiled, you must ensure the Qt (>= 5.12) development
 libraries have been installed as well as the Qbs build tool:
 
-* On Ubuntu/Debian: `sudo apt install qtbase5-dev qtbase5-private-dev libqt5svg5-dev qttools5-dev-tools zlib1g-dev qtdeclarative5-dev qtdeclarative5-private-dev qbs`
+* On Ubuntu/Debian: `sudo apt install qtbase5-dev libqt5svg5-dev qttools5-dev-tools zlib1g-dev qtdeclarative5-dev qbs`
 * On Fedora:        `sudo dnf builddep tiled`
 * On Arch Linux:    `sudo pacman -S qt qt5-tools qbs`
 * On macOS with [Homebrew](https://brew.sh/):

snap/snapcraft.yaml

@@ -59,8 +59,6 @@ parts:
       - qbs
       - libqt5svg5-dev
       - qtdeclarative5-dev
-      - qtdeclarative5-private-dev
-      - qtbase5-private-dev
       - zlib1g-dev
       - libzstd-dev
     stage-packages:

src/libtiled/object.cpp

@@ -34,8 +34,11 @@
 namespace Tiled {
 
 SharedPropertyTypes Object::mPropertyTypes;
+
 Object::~Object()
-{}
+{
+    delete mEditable;
+}
 
 /**
  * Returns the value of the property \a name, taking into account that it may
@@ -87,19 +90,19 @@ QVariantMap Object::resolvedProperties() const
 
     if (auto type = propertyTypes().findClassFor(objectClassName, *this))
         Tiled::mergeProperties(allProperties, type->members);
-    
+
     if (typeId() == Object::MapObjectType) {
         auto mapObject = static_cast<const MapObject*>(this);
 
         if (const Tile *tile = mapObject->cell().tile())
             Tiled::mergeProperties(allProperties, tile->properties());
-        
+
         if (const MapObject *templateObject = mapObject->templateObject())
             Tiled::mergeProperties(allProperties, templateObject->properties());
     }
 
     Tiled::mergeProperties(allProperties, properties());
-    
+
     return allProperties;
 }
 

src/libtiled/object.h

@@ -31,6 +31,8 @@
 #include "properties.h"
 #include "propertytype.h"
 
+#include <QPointer>
+
 namespace Tiled {
 
 /**
@@ -152,6 +154,12 @@ class TILEDSHARED_EXPORT Object
     QString mClassName;
     Properties mProperties;
 
+    /**
+     * The editable wrapper created for this object.
+     */
+    QPointer<QObject> mEditable;
+    friend class EditableObject;
+
     static SharedPropertyTypes mPropertyTypes;
 };
 

src/libtiled/tilesetmanager.cpp

@@ -35,6 +35,8 @@
 #include "tileanimationdriver.h"
 #include "tilesetformat.h"
 
+#include <QDebug>
+
 namespace Tiled {
 
 TilesetManager *TilesetManager::mInstance;
@@ -57,7 +59,10 @@ TilesetManager::TilesetManager():
 TilesetManager::~TilesetManager()
 {
     // Assert that there are no remaining tileset instances
-    Q_ASSERT(mTilesets.isEmpty());
+    if (!mTilesets.isEmpty()) {
+        qWarning() << "TilesetManager: There are still" << mTilesets.size()
+                   << "tilesets loaded at exit!";
+    }
 }
 
 /**

src/tiled/addremovelayer.cpp

@@ -43,18 +43,17 @@ AddRemoveLayer::AddRemoveLayer(MapDocument *mapDocument,
 
 AddRemoveLayer::~AddRemoveLayer()
 {
-    delete mLayer;
 }
 
 void AddRemoveLayer::addLayer()
 {
-    mMapDocument->layerModel()->insertLayer(mParentLayer, mIndex, mLayer);
-    mLayer = nullptr;
+    mMapDocument->layerModel()->insertLayer(mParentLayer, mIndex,
+                                            mLayer.release());
 }
 
 void AddRemoveLayer::removeLayer()
 {
-    mLayer = mMapDocument->layerModel()->takeLayerAt(mParentLayer, mIndex);
+    mLayer.reset(mMapDocument->layerModel()->takeLayerAt(mParentLayer, mIndex));
 }
 
 AddLayer::AddLayer(MapDocument *mapDocument,

src/tiled/addremovelayer.h

@@ -24,6 +24,8 @@
 
 #include <QUndoCommand>
 
+#include <memory>
+
 namespace Tiled {
 
 class GroupLayer;
@@ -48,7 +50,7 @@ class AddRemoveLayer : public QUndoCommand
     void removeLayer();
 
     MapDocument *mMapDocument;
-    Layer *mLayer;
+    std::unique_ptr<Layer> mLayer;
     GroupLayer *mParentLayer;
     int mIndex;
 };

src/tiled/editablegrouplayer.cpp

@@ -22,7 +22,6 @@
 
 #include "addremovelayer.h"
 #include "addremovetileset.h"
-#include "editablemanager.h"
 #include "editablemap.h"
 #include "scriptmanager.h"
 
@@ -43,11 +42,10 @@ EditableGroupLayer::EditableGroupLayer(EditableMap *map, GroupLayer *groupLayer,
 QList<QObject *> EditableGroupLayer::layers()
 {
     QList<QObject *> editables;
-    auto &editableManager = EditableManager::instance();
     auto editableMap = map();
 
     for (const auto layer : groupLayer()->layers())
-        editables.append(editableManager.editableLayer(editableMap, layer));
+        editables.append(EditableLayer::get(editableMap, layer));
 
     return editables;
 }
@@ -60,7 +58,7 @@ EditableLayer *EditableGroupLayer::layerAt(int index)
     }
 
     Layer *layer = groupLayer()->layerAt(index);
-    return EditableManager::instance().editableLayer(map(), layer);
+    return EditableLayer::get(map(), layer);
 }
 
 void EditableGroupLayer::removeLayerAt(int index)
@@ -73,7 +71,7 @@ void EditableGroupLayer::removeLayerAt(int index)
     if (MapDocument *doc = mapDocument())
         asset()->push(new RemoveLayer(doc, index, groupLayer()));
     else if (!checkReadOnly())
-        EditableManager::instance().release(groupLayer()->takeLayerAt(index));
+        EditableLayer::release(groupLayer()->takeLayerAt(index));
 }
 
 void EditableGroupLayer::removeLayer(EditableLayer *editableLayer)
@@ -100,7 +98,7 @@ void EditableGroupLayer::insertLayerAt(int index, EditableLayer *editableLayer)
     }
 
     if (!editableLayer) {
-        ScriptManager::instance().throwError(QCoreApplication::translate("Script Errors", "Invalid argument"));
+        ScriptManager::instance().throwNullArgError(1);
         return;
     }
 
@@ -124,12 +122,17 @@ void EditableGroupLayer::insertLayerAt(int index, EditableLayer *editableLayer)
             map->addTilesets(tilesets);
 
         // ownership moves to the group layer
-        groupLayer()->insertLayer(index, editableLayer->release());
+        groupLayer()->insertLayer(index, editableLayer->attach(asset()));
     }
 }
 
 void EditableGroupLayer::addLayer(EditableLayer *editableLayer)
 {
+    if (!editableLayer) {
+        ScriptManager::instance().throwNullArgError(0);
+        return;
+    }
+
     insertLayerAt(layerCount(), editableLayer);
 }
 

src/tiled/editablelayer.cpp

@@ -22,16 +22,17 @@
 
 #include "changelayer.h"
 #include "editablegrouplayer.h"
-#include "editablemanager.h"
+#include "editableimagelayer.h"
 #include "editablemap.h"
+#include "editableobjectgroup.h"
+#include "editabletilelayer.h"
 
 namespace Tiled {
 
 EditableLayer::EditableLayer(std::unique_ptr<Layer> layer, QObject *parent)
     : EditableObject(nullptr, layer.get(), parent)
 {
     mDetachedLayer = std::move(layer);
-    EditableManager::instance().mEditables.insert(this->layer(), this);
 }
 
 EditableLayer::EditableLayer(EditableAsset *asset, Layer *layer, QObject *parent)
@@ -41,7 +42,9 @@ EditableLayer::EditableLayer(EditableAsset *asset, Layer *layer, QObject *parent
 
 EditableLayer::~EditableLayer()
 {
-    EditableManager::instance().remove(this);
+    // Prevent owned object from trying to delete us again
+    if (mDetachedLayer)
+        setObject(nullptr);
 }
 
 EditableMap *EditableLayer::map() const
@@ -53,7 +56,7 @@ EditableMap *EditableLayer::map() const
 EditableGroupLayer *EditableLayer::parentLayer() const
 {
     GroupLayer *parent = layer()->parentLayer();
-    return static_cast<EditableGroupLayer*>(EditableManager::instance().editableLayer(map(), parent));
+    return static_cast<EditableGroupLayer*>(EditableLayer::get(map(), parent));
 }
 
 bool EditableLayer::isSelected() const
@@ -70,47 +73,85 @@ bool EditableLayer::isSelected() const
 void EditableLayer::detach()
 {
     Q_ASSERT(asset());
-
-    EditableManager::instance().remove(this);
     setAsset(nullptr);
 
+    if (!moveOwnershipToJavaScript())
+        return;
+
     mDetachedLayer.reset(layer()->clone());
 //    mDetachedLayer->resetIds();
     setObject(mDetachedLayer.get());
-    EditableManager::instance().mEditables.insert(layer(), this);
 }
 
 /**
  * Turns this stand-alone layer into a reference, with the layer now owned by
- * the given asset.
+ * a map, group layer or tile (in case of object group).
+ *
+ * The given \a asset may be a nullptr in case the layer is added to a group
+ * layer which isn't part of a map.
+ *
+ * Returns nullptr if the editable wasn't owning its layer.
  */
-void EditableLayer::attach(EditableAsset *asset)
+Layer *EditableLayer::attach(EditableAsset *asset)
 {
-    Q_ASSERT(!this->asset() && asset);
+    Q_ASSERT(!this->asset());
 
     setAsset(asset);
-    mDetachedLayer.release();
+    moveOwnershipToCpp();
+    return mDetachedLayer.release();
 }
 
 /**
- * Take ownership of the referenced layer.
+ * Take ownership of the referenced layer or delete it.
  */
-void EditableLayer::hold()
+void EditableLayer::hold(std::unique_ptr<Layer> layer)
 {
-    Q_ASSERT(!asset());         // if asset exists, it holds the layer (possibly indirectly)
     Q_ASSERT(!mDetachedLayer);  // can't already be holding the layer
+    Q_ASSERT(this->layer() == layer.get());
 
-    mDetachedLayer.reset(layer());
+    if (!moveOwnershipToJavaScript())
+        return;
+
+    setAsset(nullptr);
+    mDetachedLayer = std::move(layer);
 }
 
-/**
- * Release ownership of the referenced layer.
- */
-Layer *EditableLayer::release()
+EditableLayer *EditableLayer::get(EditableMap *map, Layer *layer)
 {
-    Q_ASSERT(isOwning());
+    if (!layer)
+        return nullptr;
 
-    return mDetachedLayer.release();
+    auto editable = find(layer);
+    if (editable)
+        return editable;
+
+    Q_ASSERT(!map || layer->map() == map->map());
+
+    switch (layer->layerType()) {
+    case Layer::TileLayerType:
+        editable = new EditableTileLayer(map, static_cast<TileLayer*>(layer));
+        break;
+    case Layer::ObjectGroupType:
+        editable = new EditableObjectGroup(map, static_cast<ObjectGroup*>(layer));
+        break;
+    case Layer::ImageLayerType:
+        editable = new EditableImageLayer(map, static_cast<ImageLayer*>(layer));
+        break;
+    case Layer::GroupLayerType:
+        editable = new EditableGroupLayer(map, static_cast<GroupLayer*>(layer));
+        break;
+    }
+
+    editable->moveOwnershipToCpp();
+
+    return editable;
+}
+
+void EditableLayer::release(Layer *layer)
+{
+    std::unique_ptr<Layer> owned { layer };
+    if (auto editable = EditableLayer::find(layer))
+        editable->hold(std::move(owned));
 }
 
 void EditableLayer::setName(const QString &name)

src/tiled/editablelayer.h

@@ -88,11 +88,14 @@ class EditableLayer : public EditableObject
     Layer *layer() const;
 
     void detach();
-    void attach(EditableAsset *asset);
-    void hold();
-    Layer *release();
+    Layer *attach(EditableAsset *asset);
+    void hold(std::unique_ptr<Layer> layer);
     bool isOwning() const;
 
+    static EditableLayer *find(Layer *layer);
+    static EditableLayer *get(EditableMap *map, Layer *layer);
+    static void release(Layer *layer);
+
 public slots:
     void setName(const QString &name);
     void setOpacity(qreal opacity);
@@ -182,6 +185,11 @@ inline bool EditableLayer::isOwning() const
     return mDetachedLayer.get() == layer();
 }
 
+inline EditableLayer *EditableLayer::find(Layer *layer)
+{
+    return static_cast<EditableLayer*>(EditableObject::find(layer));
+}
+
 } // namespace Tiled
 
 Q_DECLARE_METATYPE(Tiled::EditableLayer*)