NOISSUE Comment and bugfix the Resource system

8 files changed, 120 insertions, 81 deletions

diff --git a/logic/resources/IconResourceHandler.cpp b/logic/resources/IconResourceHandler.cpp
index d47dcc3d..99353b6c 100644
--- a/logic/resources/IconResourceHandler.cpp
+++ b/logic/resources/IconResourceHandler.cpp
@@ -15,6 +15,7 @@ void IconResourceHandler::setTheme(const QString &theme)
 {
 	m_theme = theme;
 
+	// notify everyone
 	for (auto handler : m_iconHandlers)
 	{
 		std::shared_ptr<IconResourceHandler> ptr = handler.lock();
@@ -28,6 +29,7 @@ void IconResourceHandler::setTheme(const QString &theme)
 void IconResourceHandler::init(std::shared_ptr<ResourceHandler> &ptr)
 {
 	m_iconHandlers.append(std::dynamic_pointer_cast<IconResourceHandler>(ptr));
+	// we always have a result, so lets report it now!
 	setResult(get());
 }
 
diff --git a/logic/resources/IconResourceHandler.h b/logic/resources/IconResourceHandler.h
index dedfecb2..6f933ad4 100644
--- a/logic/resources/IconResourceHandler.h
+++ b/logic/resources/IconResourceHandler.h
@@ -9,14 +9,17 @@ class IconResourceHandler : public ResourceHandler
 public:
 	explicit IconResourceHandler(const QString &key);
 
+	/// Sets the current theme and notifies all IconResourceHandlers of the change
 	static void setTheme(const QString &theme);
 
 private:
+	// we need to keep track of all IconResourceHandlers so that we can update them if the theme changes
 	void init(std::shared_ptr<ResourceHandler> &ptr) override;
+	static QList<std::weak_ptr<IconResourceHandler>> m_iconHandlers;
 
 	QString m_key;
 	static QString m_theme;
-	static QList<std::weak_ptr<IconResourceHandler>> m_iconHandlers;
 
+	// the workhorse, returns QVariantMap (filename => size) for m_key/m_theme
 	QVariant get() const;
 };
diff --git a/logic/resources/Resource.cpp b/logic/resources/Resource.cpp
index 16ed3d2d..3cc6b648 100644
--- a/logic/resources/Resource.cpp
+++ b/logic/resources/Resource.cpp
@@ -6,12 +6,16 @@
 #include "IconResourceHandler.h"
 #include "ResourceObserver.h"
 
+// definition of static members of Resource
 QMap<QString, std::function<std::shared_ptr<ResourceHandler>(const QString &)>> Resource::m_handlers;
 QMap<QPair<int, int>, std::function<QVariant(QVariant)>> Resource::m_transfomers;
 QMap<QString, std::weak_ptr<Resource>> Resource::m_resources;
 
 Resource::Resource(const QString &resource)
+	: m_resource(resource)
 {
+	// register default handlers
+	// QUESTION: move elsewhere?
 	if (!m_handlers.contains("web"))
 	{
 		registerHandler<WebResourceHandler>("web");
@@ -21,33 +25,48 @@ Resource::Resource(const QString &resource)
 		registerHandler<IconResourceHandler>("icon");
 	}
 
+	// a valid resource identifier has the format <id>:<data>
 	Q_ASSERT(resource.contains(':'));
+	// "parse" the resource identifier into id and data
 	const QString resourceId = resource.left(resource.indexOf(':'));
+	const QString resourceData = resource.mid(resource.indexOf(':') + 1);
+
+	// create and set up the handler
 	Q_ASSERT(m_handlers.contains(resourceId));
-	m_handler = m_handlers.value(resourceId)(resource.mid(resource.indexOf(':') + 1));
+	m_handler = m_handlers.value(resourceId)(resourceData);
+	Q_ASSERT(m_handler);
 	m_handler->init(m_handler);
 	m_handler->setResource(this);
-	Q_ASSERT(m_handler);
 }
 Resource::~Resource()
 {
 	qDeleteAll(m_observers);
 }
 
-Resource::Ptr Resource::create(const QString &resource)
+Resource::Ptr Resource::create(const QString &resource, Ptr placeholder)
 {
-	Resource::Ptr ptr = m_resources.contains(resource)
-			? m_resources.value(resource).lock()
+	const QString storageId = storageIdentifier(resource, placeholder);
+
+	// do we already have a resource? even if m_resources contains it it might not be valid any longer (weak_ptr)
+	Resource::Ptr ptr = m_resources.contains(storageId)
+			? m_resources.value(storageId).lock()
 			: nullptr;
+	// did we have one? and is it still valid?
 	if (!ptr)
 	{
+		/* We don't want Resource to have a public constructor, but std::make_shared needs it,
+		 * so we create a subclass of Resource here that exposes the constructor as public.
+		 * The alternative would be making the allocator for std::make_shared a friend, but it
+		 * differs between different STL implementations, so that would be a pain.
+		 */
 		struct ConstructableResource : public Resource
 		{
 			explicit ConstructableResource(const QString &resource)
 				: Resource(resource) {}
 		};
 		ptr = std::make_shared<ConstructableResource>(resource);
-		m_resources.insert(resource, ptr);
+		ptr->m_placeholder = placeholder;
+		m_resources.insert(storageId, ptr);
 	}
 	return ptr;
 }
@@ -56,39 +75,35 @@ Resource::Ptr Resource::applyTo(ResourceObserver *observer)
 {
 	m_observers.append(observer);
 	observer->setSource(shared_from_this()); // give the observer a shared_ptr for us so we don't get deleted
-	observer->resourceUpdated();
-	return shared_from_this();
+	observer->resourceUpdated(); // ask the observer to poll us immediently, we might already have data
+	return shared_from_this(); // allow chaining
 }
 Resource::Ptr Resource::applyTo(QObject *target, const char *property)
 {
-	// the cast to ResourceObserver* is required to ensure the right overload gets choosen
+	// the cast to ResourceObserver* is required to ensure the right overload gets choosen,
+	// since QObjectResourceObserver also inherits from QObject
 	return applyTo(static_cast<ResourceObserver *>(new QObjectResourceObserver(target, property)));
 }
 
-Resource::Ptr Resource::placeholder(Resource::Ptr other)
-{
-	m_placeholder = other;
-	for (ResourceObserver *observer : m_observers)
-	{
-		observer->resourceUpdated();
-	}
-	return shared_from_this();
-}
-
 QVariant Resource::getResourceInternal(const int typeId) const
 {
+	// no result (yet), but a placeholder? delegate to the placeholder.
 	if (m_handler->result().isNull() && m_placeholder)
 	{
 		return m_placeholder->getResourceInternal(typeId);
 	}
 	const QVariant variant = m_handler->result();
 	const auto typePair = qMakePair(int(variant.type()), typeId);
+
+	// do we have an explicit transformer? use it.
 	if (m_transfomers.contains(typePair))
 	{
 		return m_transfomers.value(typePair)(variant);
 	}
 	else
 	{
+		// we do not have an explicit transformer, so we just pass the QVariant, which will automatically
+		// transform some types for us (different numbers to each other etc.)
 		return variant;
 	}
 }
@@ -119,3 +134,15 @@ void Resource::notifyObserverDeleted(ResourceObserver *observer)
 {
 	m_observers.removeAll(observer);
 }
+
+QString Resource::storageIdentifier(const QString &id, Resource::Ptr placeholder)
+{
+	if (placeholder)
+	{
+		return id + '#' + storageIdentifier(placeholder->m_resource, placeholder->m_placeholder);
+	}
+	else
+	{
+		return id;
+	}
+}
diff --git a/logic/resources/Resource.h b/logic/resources/Resource.h
index d566b2a2..40a6d871 100644
--- a/logic/resources/Resource.h
+++ b/logic/resources/Resource.h
@@ -7,24 +7,10 @@
 #include <memory>
 
 #include "ResourceObserver.h"
+#include "TypeMagic.h"
 
 class ResourceHandler;
 
-namespace Detail
-{
-template <typename T> struct Function : public Function<decltype(&T::operator())> {};
-template <typename Ret, typename Arg> struct Function<Ret(*)(Arg)> : public Function<Ret(Arg)> {};
-template <typename Ret, typename Arg> struct Function<Ret(Arg)>
-{
-	using ReturnType = Ret;
-	using Argument = Arg;
-};
-template <class C, typename Ret, typename Arg> struct Function<Ret(C::*)(Arg)> : public Function<Ret(Arg)> {};
-template <class C, typename Ret, typename Arg> struct Function<Ret(C::*)(Arg) const> : public Function<Ret(Arg)> {};
-template <typename F> struct Function<F&> : public Function<F> {};
-template <typename F> struct Function<F&&> : public Function<F> {};
-}
-
 /** Frontend class for resources
  *
  * Usage:
@@ -39,10 +25,11 @@ template <typename F> struct Function<F&&> : public Function<F> {};
  *	placeholder (if present). This means a resource stays valid while it's still used ("applied to" etc.)
  *	by something. When nothing uses it anymore it gets deleted.
  *
- *	\note Always pass resource around using ResourcePtr! Copy and move constructors are disabled for a reason.
+ *	@note Always pass resource around using Resource::Ptr! Copy and move constructors are disabled for a reason.
  */
 class Resource : public std::enable_shared_from_this<Resource>
 {
+	// only allow creation from Resource::create and disallow passing around non-pointers
 	explicit Resource(const QString &resource);
 	Resource(const Resource &) = delete;
 	Resource(Resource &&) = delete;
@@ -51,11 +38,9 @@ public:
 
 	~Resource();
 
-	/// The returned pointer needs to be stored until either Resource::then is called, or it is used as the argument to Resource::placeholder.
-	static Ptr create(const QString &resource);
-
-	/// This can e.g. be used to set a local icon as the placeholder while a slow (remote) icon is fetched
-	Ptr placeholder(Ptr other);
+	/// The returned pointer needs to be stored until either Resource::applyTo or Resource::then is called, or it is passed as
+	/// a placeholder to Resource::create itself.
+	static Ptr create(const QString &resource, Ptr placeholder = nullptr);
 
 	/// Use these functions to specify what should happen when e.g. the resource changes
 	Ptr applyTo(ResourceObserver *observer);
@@ -63,30 +48,49 @@ public:
 	template<typename Func>
 	Ptr then(Func &&func)
 	{
-		using Arg = typename std::remove_cv<
-			typename std::remove_reference<typename Detail::Function<Func>::Argument>::type
-		>::type;
-		return applyTo(new FunctionResourceObserver<
-					   typename Detail::Function<Func>::ReturnType,
-					   Arg, Func
-					   >(std::forward<Func>(func)));
+		// Arg will be the functions argument with references and cv-qualifiers (const, volatile) removed
+		using Arg = TypeMagic::CleanType<typename TypeMagic::Function<Func>::Argument>;
+		// Ret will be the functions return type
+		using Ret = typename TypeMagic::Function<Func>::ReturnType;
+
+		// FunctionResourceObserver<ReturnType, ArgumentType, FunctionSignature>
+		return applyTo(new FunctionResourceObserver<Ret, Arg, Func>(std::forward<Func>(func)));
 	}
 
 	/// Retrieve the currently active resource. If it's type is different from T a conversion will be attempted.
 	template<typename T>
 	T getResource() const { return getResourceInternal(qMetaTypeId<T>()).template value<T>(); }
+
+	/// @internal Used by ResourceObserver and ResourceProxyModel
 	QVariant getResourceInternal(const int typeId) const;
 
+	/** Register a new ResourceHandler. T needs to inherit from ResourceHandler
+	 * Usage: Resource::registerHandler<MyResourceHandler>("myid");
+	 */
 	template<typename T>
 	static void registerHandler(const QString &id)
 	{
 		m_handlers.insert(id, [](const QString &res) { return std::make_shared<T>(res); });
 	}
+	/** Register a new resource transformer
+	 * Resource transformers are functions that are responsible for converting between different types,
+	 * for example converting from a QByteArray to a QPixmap. They are registered "externally" because not
+	 * all types might be available in this library, for example gui types like QPixmap.
+	 *
+	 * Usage: Resource::registerTransformer([](const InputType &type) { return OutputType(type); });
+	 *   This assumes that OutputType has a constructor that takes InputType as an argument. More
+	 *   complicated transformers can of course also be registered.
+	 *
+	 * When a ResourceObserver requests a type that's different from the actual resource type, a matching
+	 * transformer will be looked up from the list of transformers.
+	 * @note Only one-stage transforms will be performed (you can't registerTransformers for QString => int
+	 *       and int => float and expect QString to automatically be transformed into a float.
+	 */
 	template<typename Func>
 	static void registerTransformer(Func &&func)
 	{
-		using Out = typename Detail::Function<Func>::ReturnType;
-		using In = typename std::remove_cv<typename std::remove_reference<typename Detail::Function<Func>::Argument>::type>::type;
+		using Out = typename TypeMagic::Function<Func>::ReturnType;
+		using In = TypeMagic::CleanType<typename TypeMagic::Function<Func>::Argument>;
 		static_assert(!std::is_same<Out, In>::value, "It does not make sense to transform a value to itself");
 		m_transfomers.insert(qMakePair(qMetaTypeId<In>(), qMetaTypeId<Out>()), [func](const QVariant &in)
 		{
@@ -94,23 +98,33 @@ public:
 		});
 	}
 
-private:
+private: // half private, implementation details
 	friend class ResourceHandler;
+	// the following three functions are called by ResourceHandlers
+	/** Notifies the observers. They will call Resource::getResourceInternal which will call ResourceHandler::result
+	 * or delegate to it's placeholder.
+	 */
 	void reportResult();
 	void reportFailure(const QString &reason);
 	void reportProgress(const int progress);
 
 	friend class ResourceObserver;
+	/// Removes observer from the list of observers so that we don't attempt to notify something that doesn't exist
 	void notifyObserverDeleted(ResourceObserver *observer);
 
-private:
+private: // truly private
 	QList<ResourceObserver *> m_observers;
 	std::shared_ptr<ResourceHandler> m_handler = nullptr;
 	Ptr m_placeholder = nullptr;
+	const QString m_resource;
+
+	static QString storageIdentifier(const QString &id, Ptr placeholder = nullptr);
+	QString storageIdentifier() const;
 
 	// a list of resource handler factories, registered using registerHandler
 	static QMap<QString, std::function<std::shared_ptr<ResourceHandler>(const QString &)>> m_handlers;
 	// a list of resource transformers, registered using registerTransformer
 	static QMap<QPair<int, int>, std::function<QVariant(QVariant)>> m_transfomers;
+	// a list of resources so that we can reuse them
 	static QMap<QString, std::weak_ptr<Resource>> m_resources;
 };
diff --git a/logic/resources/ResourceHandler.h b/logic/resources/ResourceHandler.h
index c1105efc..a4f638ec 100644
--- a/logic/resources/ResourceHandler.h
+++ b/logic/resources/ResourceHandler.h
@@ -17,7 +17,8 @@ public:
 	virtual ~ResourceHandler() {}
 
 	void setResource(Resource *resource) { m_resource = resource; }
-	// reimplement this if you need to do something after you have been put in a shared pointer
+	/// reimplement this if you need to do something after you have been put in a shared pointer
+	// we do this instead of inheriting from std::enable_shared_from_this
 	virtual void init(std::shared_ptr<ResourceHandler>&) {}
 
 	QVariant result() const { return m_result; }
diff --git a/logic/resources/ResourceObserver.h b/logic/resources/ResourceObserver.h
index 27430d42..ef946c32 100644
--- a/logic/resources/ResourceObserver.h
+++ b/logic/resources/ResourceObserver.h
@@ -51,6 +51,11 @@ private:
 	QMetaProperty m_property;
 };
 
+/** Observer for functions, lambdas etc.
+ * Template arguments:
+ *   * We need Ret and Arg in order to create the std::function
+ *   * We need Func in order to std::forward the function
+ */
 template <typename Ret, typename Arg, typename Func>
 class FunctionResourceObserver : public ResourceObserver
 {
diff --git a/logic/resources/ResourceProxyModel.cpp b/logic/resources/ResourceProxyModel.cpp
index 6ff11367..f026d9a9 100644
--- a/logic/resources/ResourceProxyModel.cpp
+++ b/logic/resources/ResourceProxyModel.cpp
@@ -5,8 +5,6 @@
 #include "Resource.h"
 #include "ResourceObserver.h"
 
-//Q_DECLARE_METATYPE(QVector<int>)
-
 class ModelResourceObserver : public ResourceObserver
 {
 public:
@@ -20,6 +18,7 @@ public:
 	{
 		if (m_index.isValid())
 		{
+			// the resource changed, pretend to be the model and notify the views of the update. they will re-poll the model which will return the new resource value
 			QMetaObject::invokeMethod(const_cast<QAbstractItemModel *>(m_index.model()),
 									  "dataChanged", Qt::QueuedConnection,
 									  Q_ARG(QModelIndex, m_index), Q_ARG(QModelIndex, m_index), Q_ARG(QVector<int>, QVector<int>() << m_role));
@@ -39,24 +38,29 @@ ResourceProxyModel::ResourceProxyModel(const int resultTypeId, QObject *parent)
 QVariant ResourceProxyModel::data(const QModelIndex &proxyIndex, int role) const
 {
 	const QModelIndex mapped = mapToSource(proxyIndex);
+	// valid cell that's a Qt::DecorationRole and that contains a non-empty string
 	if (mapped.isValid() && role == Qt::DecorationRole && !mapToSource(proxyIndex).data(role).toString().isEmpty())
 	{
+		// do we already have a resource for this index?
 		if (!m_resources.contains(mapped))
 		{
-			Resource::Ptr res = Resource::create(mapToSource(proxyIndex).data(role).toString())
-					->applyTo(new ModelResourceObserver(proxyIndex, role));
-
-			const QVariant placeholder = mapped.data(PlaceholderRole);
-			if (!placeholder.isNull() && placeholder.type() == QVariant::String)
+			Resource::Ptr placeholder;
+			const QVariant placeholderIdentifier = mapped.data(PlaceholderRole);
+			if (!placeholderIdentifier.isNull() && placeholderIdentifier.type() == QVariant::String)
 			{
-				res->placeholder(Resource::create(placeholder.toString()));
+				placeholder = Resource::create(placeholderIdentifier.toString());
 			}
 
+			// create the Resource and apply the observer for models
+			Resource::Ptr res = Resource::create(mapToSource(proxyIndex).data(role).toString(), placeholder)
+					->applyTo(new ModelResourceObserver(proxyIndex, role));
+
 			m_resources.insert(mapped, res);
 		}
 
 		return m_resources.value(mapped)->getResourceInternal(m_resultTypeId);
 	}
+	// otherwise fall back to the source model
 	return mapped.data(role);
 }
 
@@ -70,7 +74,8 @@ void ResourceProxyModel::setSourceModel(QAbstractItemModel *model)
 	{
 		connect(model, &QAbstractItemModel::dataChanged, this, [this](const QModelIndex &tl, const QModelIndex &br, const QVector<int> &roles)
 		{
-			if (roles.contains(Qt::DecorationRole) || roles.isEmpty())
+			// invalidate resources so that they will be re-created
+			if (roles.contains(Qt::DecorationRole) || roles.contains(PlaceholderRole) || roles.isEmpty())
 			{
 				const QItemSelectionRange range(tl, br);
 				for (const QModelIndex &index : range.indexes())
@@ -78,25 +83,6 @@ void ResourceProxyModel::setSourceModel(QAbstractItemModel *model)
 					m_resources.remove(index);
 				}
 			}
-			else if (roles.contains(PlaceholderRole))
-			{
-				const QItemSelectionRange range(tl, br);
-				for (const QModelIndex &index : range.indexes())
-				{
-					if (m_resources.contains(index))
-					{
-						const QVariant placeholder = index.data(PlaceholderRole);
-						if (!placeholder.isNull() && placeholder.type() == QVariant::String)
-						{
-							m_resources.value(index)->placeholder(Resource::create(placeholder.toString()));
-						}
-						else
-						{
-							m_resources.value(index)->placeholder(nullptr);
-						}
-					}
-				}
-			}
 		});
 	}
 	QIdentityProxyModel::setSourceModel(model);
diff --git a/logic/resources/ResourceProxyModel.h b/logic/resources/ResourceProxyModel.h
index 9db09545..f9a83d1d 100644
--- a/logic/resources/ResourceProxyModel.h
+++ b/logic/resources/ResourceProxyModel.h
@@ -20,6 +20,7 @@ public:
 	QVariant data(const QModelIndex &proxyIndex, int role) const override;
 	void setSourceModel(QAbstractItemModel *model) override;
 
+	/// Helper function, usage: m_view->setModel(ResourceProxyModel::mixin<QIcon>(m_model));
 	template <typename T>
 	static QAbstractItemModel *mixin(QAbstractItemModel *model)
 	{