fix: don't give shared pointer to obj. external to the model

It causes some weird problems and adds refcounting overhead.

Signed-off-by: flow <flowlnlnln@gmail.com>

8 files changed, 52 insertions, 47 deletions

diff --git a/launcher/minecraft/mod/Mod.h b/launcher/minecraft/mod/Mod.h
index 0835e3b1..49326c83 100644
--- a/launcher/minecraft/mod/Mod.h
+++ b/launcher/minecraft/mod/Mod.h
@@ -47,6 +47,7 @@ class Mod : public Resource
     Q_OBJECT
 public:
     using Ptr = shared_qobject_ptr<Mod>;
+    using WeakPtr = QPointer<Mod>;
 
     Mod() = default;
     Mod(const QFileInfo &file);
diff --git a/launcher/minecraft/mod/ModFolderModel.cpp b/launcher/minecraft/mod/ModFolderModel.cpp
index c316e710..8bdab16e 100644
--- a/launcher/minecraft/mod/ModFolderModel.cpp
+++ b/launcher/minecraft/mod/ModFolderModel.cpp
@@ -226,9 +226,9 @@ bool ModFolderModel::stopWatching()
     return ResourceFolderModel::stopWatching({ m_dir.absolutePath(), indexDir().absolutePath() });
 }
 
-auto ModFolderModel::selectedMods(QModelIndexList& indexes) -> QList<Mod::Ptr>
+auto ModFolderModel::selectedMods(QModelIndexList& indexes) -> QList<Mod*>
 {
-    QList<Mod::Ptr> selected_resources;
+    QList<Mod*> selected_resources;
     for (auto i : indexes) {
         if(i.column() != 0)
             continue;
@@ -238,12 +238,13 @@ auto ModFolderModel::selectedMods(QModelIndexList& indexes) -> QList<Mod::Ptr>
     return selected_resources;
 }
 
-auto ModFolderModel::allMods() -> QList<Mod::Ptr>
+auto ModFolderModel::allMods() -> QList<Mod*>
 {
-    QList<Mod::Ptr> mods;
+    QList<Mod*> mods;
 
-    for (auto res : m_resources)
+    for (auto& res : m_resources) {
         mods.append(static_cast<Mod*>(res.get()));
+    }
 
     return mods;
 }
diff --git a/launcher/minecraft/mod/ModFolderModel.h b/launcher/minecraft/mod/ModFolderModel.h
index b1f30710..7fe830c2 100644
--- a/launcher/minecraft/mod/ModFolderModel.h
+++ b/launcher/minecraft/mod/ModFolderModel.h
@@ -101,8 +101,8 @@ public:
 
     QDir indexDir() { return { QString("%1/.index").arg(dir().absolutePath()) }; }
 
-    auto selectedMods(QModelIndexList& indexes) -> QList<Mod::Ptr>;
-    auto allMods() -> QList<Mod::Ptr>;
+    auto selectedMods(QModelIndexList& indexes) -> QList<Mod*>;
+    auto allMods() -> QList<Mod*>;
 
     RESOURCE_HELPERS(Mod)
 
diff --git a/launcher/minecraft/mod/Resource.h b/launcher/minecraft/mod/Resource.h
index bec7490f..e76bc49e 100644
--- a/launcher/minecraft/mod/Resource.h
+++ b/launcher/minecraft/mod/Resource.h
@@ -3,6 +3,7 @@
 #include <QDateTime>
 #include <QFileInfo>
 #include <QObject>
+#include <QPointer>
 
 #include "QObjectPtr.h"
 
@@ -31,6 +32,7 @@ class Resource : public QObject {
     Q_DISABLE_COPY(Resource)
    public:
     using Ptr = shared_qobject_ptr<Resource>;
+    using WeakPtr = QPointer<Resource>;
 
     Resource(QObject* parent = nullptr);
     Resource(QFileInfo file_info);
diff --git a/launcher/minecraft/mod/ResourceFolderModel.cpp b/launcher/minecraft/mod/ResourceFolderModel.cpp
index 982915e2..c27a5e2d 100644
--- a/launcher/minecraft/mod/ResourceFolderModel.cpp
+++ b/launcher/minecraft/mod/ResourceFolderModel.cpp
@@ -135,7 +135,7 @@ bool ResourceFolderModel::installResource(QString original_path)
 
 bool ResourceFolderModel::uninstallResource(QString file_name)
 {
-    for (auto resource : m_resources) {
+    for (auto& resource : m_resources) {
         if (resource->fileinfo().fileName() == file_name)
             return resource->destroy();
     }
@@ -155,7 +155,7 @@ bool ResourceFolderModel::deleteResources(const QModelIndexList& indexes)
             continue;
         }
 
-        auto resource = m_resources.at(i.row());
+        auto& resource = m_resources.at(i.row());
         resource->destroy();
     }
     return true;
@@ -183,7 +183,7 @@ bool ResourceFolderModel::update()
     return true;
 }
 
-void ResourceFolderModel::resolveResource(Resource::Ptr res)
+void ResourceFolderModel::resolveResource(Resource::WeakPtr res)
 {
     if (!res->shouldResolve()) {
         return;
diff --git a/launcher/minecraft/mod/ResourceFolderModel.h b/launcher/minecraft/mod/ResourceFolderModel.h
index 986d9885..59d2388a 100644
--- a/launcher/minecraft/mod/ResourceFolderModel.h
+++ b/launcher/minecraft/mod/ResourceFolderModel.h
@@ -15,10 +15,10 @@ class QSortFilterProxyModel;
 
 /** A basic model for external resources.
  *
- *  To implement one such model, you need to implement, at the very minimum:
- *  - columnCount: The number of columns in your model.
- *  - data: How the model data is displayed and accessed.
- *  - headerData: Display properties of the header.
+ *  This model manages a list of resources. As such, external users of such resources do not own them,
+ *  and the resource's lifetime is contingent on the model's lifetime.
+ *
+ *  TODO: Make the resources unique pointers accessible through weak pointers.
  */
 class ResourceFolderModel : public QAbstractListModel {
     Q_OBJECT
@@ -62,7 +62,7 @@ class ResourceFolderModel : public QAbstractListModel {
     virtual bool update();
 
     /** Creates a new parse task, if needed, for 'res' and start it.*/
-    virtual void resolveResource(Resource::Ptr res);
+    virtual void resolveResource(Resource::WeakPtr res);
 
     [[nodiscard]] size_t size() const { return m_resources.size(); };
     [[nodiscard]] bool empty() const { return size() == 0; }
@@ -151,7 +151,7 @@ class ResourceFolderModel : public QAbstractListModel {
      *
      *  This usually calls static_cast on the specific Task type returned by createUpdateTask,
      *  so care must be taken in such cases.
-     *  TODO: Figure out a way to express this relationship better without templated classes (Q_OBJECT macro dissalows that).
+     *  TODO: Figure out a way to express this relationship better without templated classes (Q_OBJECT macro disallows that).
      */
     virtual void onUpdateSucceeded();
     virtual void onUpdateFailed() {}
@@ -189,33 +189,34 @@ class ResourceFolderModel : public QAbstractListModel {
 };
 
 /* A macro to define useful functions to handle Resource* -> T* more easily on derived classes */
-#define RESOURCE_HELPERS(T)                                                                                                        \
-    [[nodiscard]] T* operator[](size_t index)                                                                                      \
-    {                                                                                                                              \
-        return static_cast<T*>(m_resources[index].get());                                                                          \
-    }                                                                                                                              \
-    [[nodiscard]] T* at(size_t index)                                                                                              \
-    {                                                                                                                              \
-        return static_cast<T*>(m_resources[index].get());                                                                          \
-    }                                                                                                                              \
-    [[nodiscard]] const T* at(size_t index) const                                                                                  \
-    {                                                                                                                              \
-        return static_cast<const T*>(m_resources.at(index).get());                                                                 \
-    }                                                                                                                              \
-    [[nodiscard]] T* first()                                                                                                       \
-    {                                                                                                                              \
-        return static_cast<T*>(m_resources.first().get());                                                                         \
-    }                                                                                                                              \
-    [[nodiscard]] T* last()                                                                                                        \
-    {                                                                                                                              \
-        return static_cast<T*>(m_resources.last().get());                                                                          \
-    }                                                                                                                              \
-    [[nodiscard]] T* find(QString id)                                                                                              \
-    {                                                                                                                              \
-        auto iter = std::find_if(m_resources.begin(), m_resources.end(), [&](Resource::Ptr r) { return r->internal_id() == id; }); \
-        if (iter == m_resources.end())                                                                                             \
-            return nullptr;                                                                                                        \
-        return static_cast<T*>((*iter).get());                                                                                     \
+#define RESOURCE_HELPERS(T)                                                                       \
+    [[nodiscard]] T* operator[](size_t index)                                                     \
+    {                                                                                             \
+        return static_cast<T*>(m_resources[index].get());                                         \
+    }                                                                                             \
+    [[nodiscard]] T* at(size_t index)                                                             \
+    {                                                                                             \
+        return static_cast<T*>(m_resources[index].get());                                         \
+    }                                                                                             \
+    [[nodiscard]] const T* at(size_t index) const                                                 \
+    {                                                                                             \
+        return static_cast<const T*>(m_resources.at(index).get());                                \
+    }                                                                                             \
+    [[nodiscard]] T* first()                                                                      \
+    {                                                                                             \
+        return static_cast<T*>(m_resources.first().get());                                        \
+    }                                                                                             \
+    [[nodiscard]] T* last()                                                                       \
+    {                                                                                             \
+        return static_cast<T*>(m_resources.last().get());                                         \
+    }                                                                                             \
+    [[nodiscard]] T* find(QString id)                                                             \
+    {                                                                                             \
+        auto iter = std::find_if(m_resources.constBegin(), m_resources.constEnd(),                \
+                                 [&](Resource::Ptr const& r) { return r->internal_id() == id; }); \
+        if (iter == m_resources.constEnd())                                                       \
+            return nullptr;                                                                       \
+        return static_cast<T*>((*iter).get());                                                    \
     }
 
 /* Template definition to avoid some code duplication */
@@ -231,7 +232,7 @@ void ResourceFolderModel::applyUpdates(QSet<QString>& current_set, QSet<QString>
             auto row = m_resources_index[kept];
 
             auto new_resource = new_resources[kept];
-            auto current_resource = m_resources[row];
+            auto const& current_resource = m_resources[row];
 
             if (new_resource->dateTimeChanged() == current_resource->dateTimeChanged()) {
                 // no significant change, ignore...
@@ -301,7 +302,7 @@ void ResourceFolderModel::applyUpdates(QSet<QString>& current_set, QSet<QString>
     {
         m_resources_index.clear();
         int idx = 0;
-        for (auto mod : m_resources) {
+        for (auto const& mod : m_resources) {
             m_resources_index[mod->internal_id()] = idx;
             idx++;
         }
diff --git a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h
index 0fd5c292..2944c747 100644
--- a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h
+++ b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h
@@ -17,7 +17,7 @@ class BasicFolderLoadTask : public Task
     Q_OBJECT
 public:
     struct Result {
-        QMap<QString, Resource::Ptr> resources;
+        QMap<QString, Resource*> resources;
     };
     using ResultPtr = std::shared_ptr<Result>;
 
diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp
index e8180c11..44cb1d5f 100644
--- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp
+++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp
@@ -52,7 +52,7 @@ void ModFolderLoadTask::executeTask()
     // Read JAR files that don't have metadata
     m_mods_dir.refresh();
     for (auto entry : m_mods_dir.entryInfoList()) {
-        Mod::Ptr mod(new Mod(entry));
+        Mod* mod(new Mod(entry));
 
         if (mod->enabled()) {
             if (m_result->mods.contains(mod->internal_id())) {