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>
This commit is contained in:
flow 2022-08-11 18:24:26 -03:00
parent 97a74d5c1f
commit 92aa24ae34
No known key found for this signature in database
GPG Key ID: 8D0F221F0A59F469
10 changed files with 58 additions and 54 deletions

View File

@ -47,6 +47,7 @@ class Mod : public Resource
Q_OBJECT Q_OBJECT
public: public:
using Ptr = shared_qobject_ptr<Mod>; using Ptr = shared_qobject_ptr<Mod>;
using WeakPtr = QPointer<Mod>;
Mod() = default; Mod() = default;
Mod(const QFileInfo &file); Mod(const QFileInfo &file);

View File

@ -226,9 +226,9 @@ bool ModFolderModel::stopWatching()
return ResourceFolderModel::stopWatching({ m_dir.absolutePath(), indexDir().absolutePath() }); 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) { for (auto i : indexes) {
if(i.column() != 0) if(i.column() != 0)
continue; continue;
@ -238,12 +238,13 @@ auto ModFolderModel::selectedMods(QModelIndexList& indexes) -> QList<Mod::Ptr>
return selected_resources; 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())); mods.append(static_cast<Mod*>(res.get()));
}
return mods; return mods;
} }

View File

@ -101,8 +101,8 @@ public:
QDir indexDir() { return { QString("%1/.index").arg(dir().absolutePath()) }; } QDir indexDir() { return { QString("%1/.index").arg(dir().absolutePath()) }; }
auto selectedMods(QModelIndexList& indexes) -> QList<Mod::Ptr>; auto selectedMods(QModelIndexList& indexes) -> QList<Mod*>;
auto allMods() -> QList<Mod::Ptr>; auto allMods() -> QList<Mod*>;
RESOURCE_HELPERS(Mod) RESOURCE_HELPERS(Mod)

View File

@ -3,6 +3,7 @@
#include <QDateTime> #include <QDateTime>
#include <QFileInfo> #include <QFileInfo>
#include <QObject> #include <QObject>
#include <QPointer>
#include "QObjectPtr.h" #include "QObjectPtr.h"
@ -31,6 +32,7 @@ class Resource : public QObject {
Q_DISABLE_COPY(Resource) Q_DISABLE_COPY(Resource)
public: public:
using Ptr = shared_qobject_ptr<Resource>; using Ptr = shared_qobject_ptr<Resource>;
using WeakPtr = QPointer<Resource>;
Resource(QObject* parent = nullptr); Resource(QObject* parent = nullptr);
Resource(QFileInfo file_info); Resource(QFileInfo file_info);

View File

@ -135,7 +135,7 @@ bool ResourceFolderModel::installResource(QString original_path)
bool ResourceFolderModel::uninstallResource(QString file_name) bool ResourceFolderModel::uninstallResource(QString file_name)
{ {
for (auto resource : m_resources) { for (auto& resource : m_resources) {
if (resource->fileinfo().fileName() == file_name) if (resource->fileinfo().fileName() == file_name)
return resource->destroy(); return resource->destroy();
} }
@ -155,7 +155,7 @@ bool ResourceFolderModel::deleteResources(const QModelIndexList& indexes)
continue; continue;
} }
auto resource = m_resources.at(i.row()); auto& resource = m_resources.at(i.row());
resource->destroy(); resource->destroy();
} }
return true; return true;
@ -183,7 +183,7 @@ bool ResourceFolderModel::update()
return true; return true;
} }
void ResourceFolderModel::resolveResource(Resource::Ptr res) void ResourceFolderModel::resolveResource(Resource::WeakPtr res)
{ {
if (!res->shouldResolve()) { if (!res->shouldResolve()) {
return; return;

View File

@ -15,10 +15,10 @@ class QSortFilterProxyModel;
/** A basic model for external resources. /** A basic model for external resources.
* *
* To implement one such model, you need to implement, at the very minimum: * This model manages a list of resources. As such, external users of such resources do not own them,
* - columnCount: The number of columns in your model. * and the resource's lifetime is contingent on the model's lifetime.
* - data: How the model data is displayed and accessed. *
* - headerData: Display properties of the header. * TODO: Make the resources unique pointers accessible through weak pointers.
*/ */
class ResourceFolderModel : public QAbstractListModel { class ResourceFolderModel : public QAbstractListModel {
Q_OBJECT Q_OBJECT
@ -62,7 +62,7 @@ class ResourceFolderModel : public QAbstractListModel {
virtual bool update(); virtual bool update();
/** Creates a new parse task, if needed, for 'res' and start it.*/ /** 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]] size_t size() const { return m_resources.size(); };
[[nodiscard]] bool empty() const { return size() == 0; } [[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, * This usually calls static_cast on the specific Task type returned by createUpdateTask,
* so care must be taken in such cases. * 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 onUpdateSucceeded();
virtual void onUpdateFailed() {} 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 */ /* A macro to define useful functions to handle Resource* -> T* more easily on derived classes */
#define RESOURCE_HELPERS(T) \ #define RESOURCE_HELPERS(T) \
[[nodiscard]] T* operator[](size_t index) \ [[nodiscard]] T* operator[](size_t index) \
{ \ { \
return static_cast<T*>(m_resources[index].get()); \ return static_cast<T*>(m_resources[index].get()); \
} \ } \
[[nodiscard]] T* at(size_t index) \ [[nodiscard]] T* at(size_t index) \
{ \ { \
return static_cast<T*>(m_resources[index].get()); \ return static_cast<T*>(m_resources[index].get()); \
} \ } \
[[nodiscard]] const T* at(size_t index) const \ [[nodiscard]] const T* at(size_t index) const \
{ \ { \
return static_cast<const T*>(m_resources.at(index).get()); \ return static_cast<const T*>(m_resources.at(index).get()); \
} \ } \
[[nodiscard]] T* first() \ [[nodiscard]] T* first() \
{ \ { \
return static_cast<T*>(m_resources.first().get()); \ return static_cast<T*>(m_resources.first().get()); \
} \ } \
[[nodiscard]] T* last() \ [[nodiscard]] T* last() \
{ \ { \
return static_cast<T*>(m_resources.last().get()); \ return static_cast<T*>(m_resources.last().get()); \
} \ } \
[[nodiscard]] T* find(QString id) \ [[nodiscard]] T* find(QString id) \
{ \ { \
auto iter = std::find_if(m_resources.begin(), m_resources.end(), [&](Resource::Ptr r) { return r->internal_id() == id; }); \ auto iter = std::find_if(m_resources.constBegin(), m_resources.constEnd(), \
if (iter == m_resources.end()) \ [&](Resource::Ptr const& r) { return r->internal_id() == id; }); \
return nullptr; \ if (iter == m_resources.constEnd()) \
return static_cast<T*>((*iter).get()); \ return nullptr; \
return static_cast<T*>((*iter).get()); \
} }
/* Template definition to avoid some code duplication */ /* 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 row = m_resources_index[kept];
auto new_resource = new_resources[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()) { if (new_resource->dateTimeChanged() == current_resource->dateTimeChanged()) {
// no significant change, ignore... // no significant change, ignore...
@ -301,7 +302,7 @@ void ResourceFolderModel::applyUpdates(QSet<QString>& current_set, QSet<QString>
{ {
m_resources_index.clear(); m_resources_index.clear();
int idx = 0; int idx = 0;
for (auto mod : m_resources) { for (auto const& mod : m_resources) {
m_resources_index[mod->internal_id()] = idx; m_resources_index[mod->internal_id()] = idx;
idx++; idx++;
} }

View File

@ -17,7 +17,7 @@ class BasicFolderLoadTask : public Task
Q_OBJECT Q_OBJECT
public: public:
struct Result { struct Result {
QMap<QString, Resource::Ptr> resources; QMap<QString, Resource*> resources;
}; };
using ResultPtr = std::shared_ptr<Result>; using ResultPtr = std::shared_ptr<Result>;

View File

@ -52,7 +52,7 @@ void ModFolderLoadTask::executeTask()
// Read JAR files that don't have metadata // Read JAR files that don't have metadata
m_mods_dir.refresh(); m_mods_dir.refresh();
for (auto entry : m_mods_dir.entryInfoList()) { for (auto entry : m_mods_dir.entryInfoList()) {
Mod::Ptr mod(new Mod(entry)); Mod* mod(new Mod(entry));
if (mod->enabled()) { if (mod->enabled()) {
if (m_result->mods.contains(mod->internal_id())) { if (m_result->mods.contains(mod->internal_id())) {

View File

@ -36,7 +36,7 @@ static ModAPI::ModLoaderTypes mcLoaders(BaseInstance* inst)
ModUpdateDialog::ModUpdateDialog(QWidget* parent, ModUpdateDialog::ModUpdateDialog(QWidget* parent,
BaseInstance* instance, BaseInstance* instance,
const std::shared_ptr<ModFolderModel> mods, const std::shared_ptr<ModFolderModel> mods,
QList<Mod::Ptr>& search_for) QList<Mod*>& search_for)
: ReviewMessageBox(parent, tr("Confirm mods to update"), "") : ReviewMessageBox(parent, tr("Confirm mods to update"), "")
, m_parent(parent) , m_parent(parent)
, m_mod_model(mods) , m_mod_model(mods)
@ -226,9 +226,8 @@ auto ModUpdateDialog::ensureMetadata() -> bool
}; };
for (auto candidate : m_candidates) { for (auto candidate : m_candidates) {
auto* candidate_ptr = candidate.get();
if (candidate->status() != ModStatus::NoMetadata) { if (candidate->status() != ModStatus::NoMetadata) {
onMetadataEnsured(candidate_ptr); onMetadataEnsured(candidate);
continue; continue;
} }
@ -236,7 +235,7 @@ auto ModUpdateDialog::ensureMetadata() -> bool
continue; continue;
if (confirm_rest) { if (confirm_rest) {
addToTmp(candidate_ptr, provider_rest); addToTmp(candidate, provider_rest);
should_try_others.insert(candidate->internal_id(), try_others_rest); should_try_others.insert(candidate->internal_id(), try_others_rest);
continue; continue;
} }
@ -261,7 +260,7 @@ auto ModUpdateDialog::ensureMetadata() -> bool
should_try_others.insert(candidate->internal_id(), response.try_others); should_try_others.insert(candidate->internal_id(), response.try_others);
if (confirmed) if (confirmed)
addToTmp(candidate_ptr, response.chosen); addToTmp(candidate, response.chosen);
} }
if (!modrinth_tmp.empty()) { if (!modrinth_tmp.empty()) {

View File

@ -19,7 +19,7 @@ class ModUpdateDialog final : public ReviewMessageBox {
explicit ModUpdateDialog(QWidget* parent, explicit ModUpdateDialog(QWidget* parent,
BaseInstance* instance, BaseInstance* instance,
const std::shared_ptr<ModFolderModel> mod_model, const std::shared_ptr<ModFolderModel> mod_model,
QList<Mod::Ptr>& search_for); QList<Mod*>& search_for);
void checkCandidates(); void checkCandidates();
@ -46,7 +46,7 @@ class ModUpdateDialog final : public ReviewMessageBox {
const std::shared_ptr<ModFolderModel> m_mod_model; const std::shared_ptr<ModFolderModel> m_mod_model;
QList<Mod::Ptr>& m_candidates; QList<Mod*>& m_candidates;
QList<Mod*> m_modrinth_to_update; QList<Mod*> m_modrinth_to_update;
QList<Mod*> m_flame_to_update; QList<Mod*> m_flame_to_update;