core/icon: stop reusing image ids (dbusmenu, notifications)

Fixes issues caused by the QML engine caching old pixmaps using the
same IDs as new ones, notably dbusmenu icons.

Author: outfoxxed

src/core/imageprovider.cpp

@@ -1,16 +1,21 @@
 #include "imageprovider.hpp"
 
+#include <qcontainerfwd.h>
 #include <qdebug.h>
 #include <qimage.h>
 #include <qlogging.h>
 #include <qmap.h>
 #include <qobject.h>
 #include <qpixmap.h>
 #include <qqmlengine.h>
+#include <qtypes.h>
 
 namespace {
 
+namespace {
 QMap<QString, QsImageHandle*> liveImages; // NOLINT
+quint32 handleIndex = 0;                  // NOLINT
+} // namespace
 
 void parseReq(const QString& req, QString& target, QString& param) {
 	auto splitIdx = req.indexOf('/');
@@ -24,14 +29,9 @@ void parseReq(const QString& req, QString& target, QString& param) {
 
 } // namespace
 
-QsImageHandle::QsImageHandle(QQmlImageProviderBase::ImageType type, QObject* parent)
-    : QObject(parent)
-    , type(type) {
-	{
-		auto dbg = QDebug(&this->id);
-		dbg.nospace() << static_cast<void*>(this);
-	}
-
+QsImageHandle::QsImageHandle(QQmlImageProviderBase::ImageType type)
+    : type(type)
+    , id(QString::number(++handleIndex)) {
 	liveImages.insert(this->id, this);
 }
 
@@ -85,3 +85,9 @@ QsPixmapProvider::requestPixmap(const QString& id, QSize* size, const QSize& req
 		return QPixmap();
 	}
 }
+
+QString QsIndexedImageHandle::url() const {
+	return this->QsImageHandle::url() % '/' % QString::number(this->changeIndex);
+}
+
+void QsIndexedImageHandle::imageChanged() { ++this->changeIndex; }

src/core/imageprovider.hpp

@@ -20,15 +20,13 @@ class QsPixmapProvider: public QQuickImageProvider {
 	QPixmap requestPixmap(const QString& id, QSize* size, const QSize& requestedSize) override;
 };
 
-class QsImageHandle: public QObject {
-	Q_OBJECT;
-
+class QsImageHandle {
 public:
-	explicit QsImageHandle(QQmlImageProviderBase::ImageType type, QObject* parent = nullptr);
-	~QsImageHandle() override;
+	explicit QsImageHandle(QQmlImageProviderBase::ImageType type);
+	virtual ~QsImageHandle();
 	Q_DISABLE_COPY_MOVE(QsImageHandle);
 
-	[[nodiscard]] QString url() const;
+	[[nodiscard]] virtual QString url() const;
 
 	virtual QImage requestImage(const QString& id, QSize* size, const QSize& requestedSize);
 	virtual QPixmap requestPixmap(const QString& id, QSize* size, const QSize& requestedSize);
@@ -37,3 +35,14 @@ class QsImageHandle: public QObject {
 	QQmlImageProviderBase::ImageType type;
 	QString id;
 };
+
+class QsIndexedImageHandle: public QsImageHandle {
+public:
+	explicit QsIndexedImageHandle(QQmlImageProviderBase::ImageType type): QsImageHandle(type) {}
+
+	[[nodiscard]] QString url() const override;
+	void imageChanged();
+
+private:
+	quint32 changeIndex = 0;
+};

src/dbus/dbusmenu/dbusmenu.cpp

@@ -59,8 +59,8 @@ QString DBusMenuItem::icon() const {
 		    this->iconName,
 		    this->menu->iconThemePath.value().join(':')
 		);
-	} else if (this->image != nullptr) {
-		return this->image->url();
+	} else if (this->image.hasData()) {
+		return this->image.url();
 	} else return nullptr;
 }
 
@@ -113,7 +113,7 @@ void DBusMenuItem::updateProperties(const QVariantMap& properties, const QString
 	auto originalEnabled = this->mEnabled;
 	auto originalVisible = this->visible;
 	auto originalIconName = this->iconName;
-	auto* originalImage = this->image;
+	auto imageChanged = false;
 	auto originalIsSeparator = this->mSeparator;
 	auto originalButtonType = this->mButtonType;
 	auto originalToggleState = this->mCheckState;
@@ -173,12 +173,16 @@ void DBusMenuItem::updateProperties(const QVariantMap& properties, const QString
 	if (iconData.canConvert<QByteArray>()) {
 		auto data = iconData.value<QByteArray>();
 		if (data.isEmpty()) {
-			this->image = nullptr;
-		} else if (this->image == nullptr || this->image->data != data) {
-			this->image = new DBusMenuPngImage(data, this);
+			imageChanged = this->image.hasData();
+			this->image.data.clear();
+		} else if (!this->image.hasData() || this->image.data != data) {
+			imageChanged = true;
+			this->image.data = data;
+			this->image.imageChanged();
 		}
 	} else if (removed.isEmpty() || removed.contains("icon-data")) {
-		this->image = nullptr;
+		imageChanged = this->image.hasData();
+		image.data.clear();
 	}
 
 	auto type = properties.value("type");
@@ -239,17 +243,13 @@ void DBusMenuItem::updateProperties(const QVariantMap& properties, const QString
 	if (this->mSeparator != originalIsSeparator) emit this->isSeparatorChanged();
 	if (this->displayChildren != originalDisplayChildren) emit this->hasChildrenChanged();
 
-	if (this->iconName != originalIconName || this->image != originalImage) {
-		if (this->image != originalImage) {
-			delete originalImage;
-		}
-
+	if (this->iconName != originalIconName || imageChanged) {
 		emit this->iconChanged();
 	}
 
 	qCDebug(logDbusMenu).nospace() << "Updated properties of " << this << " { label=" << this->mText
 	                               << ", enabled=" << this->mEnabled << ", visible=" << this->visible
-	                               << ", iconName=" << this->iconName << ", iconData=" << this->image
+	                               << ", iconName=" << this->iconName << ", iconData=" << &this->image
 	                               << ", separator=" << this->mSeparator
 	                               << ", toggleType=" << this->mButtonType
 	                               << ", toggleState=" << this->mCheckState

src/dbus/dbusmenu/dbusmenu.hpp

@@ -30,7 +30,17 @@ namespace qs::dbus::dbusmenu {
 using menu::QsMenuEntry;
 
 class DBusMenu;
-class DBusMenuPngImage;
+class DBusMenuItem;
+
+class DBusMenuPngImage: public QsIndexedImageHandle {
+public:
+	explicit DBusMenuPngImage(): QsIndexedImageHandle(QQuickImageProvider::Image) {}
+
+	[[nodiscard]] bool hasData() const { return !data.isEmpty(); }
+	QImage requestImage(const QString& id, QSize* size, const QSize& requestedSize) override;
+
+	QByteArray data;
+};
 
 ///! Menu item shared by an external program.
 /// Menu item shared by an external program via the
@@ -93,7 +103,7 @@ private slots:
 	bool visible = true;
 	bool mSeparator = false;
 	QString iconName;
-	DBusMenuPngImage* image = nullptr;
+	DBusMenuPngImage image;
 	menu::QsMenuButtonType::Enum mButtonType = menu::QsMenuButtonType::None;
 	Qt::CheckState mCheckState = Qt::Unchecked;
 	bool displayChildren = false;
@@ -156,17 +166,6 @@ private slots:
 
 QDebug operator<<(QDebug debug, DBusMenu* menu);
 
-class DBusMenuPngImage: public QsImageHandle {
-public:
-	explicit DBusMenuPngImage(QByteArray data, DBusMenuItem* parent)
-	    : QsImageHandle(QQuickImageProvider::Image, parent)
-	    , data(std::move(data)) {}
-
-	QImage requestImage(const QString& id, QSize* size, const QSize& requestedSize) override;
-
-	QByteArray data;
-};
-
 class DBusMenuHandle;
 
 QDebug operator<<(QDebug debug, const DBusMenuHandle* handle);

src/services/notifications/dbusimage.hpp

@@ -1,7 +1,5 @@
 #pragma once
 
-#include <utility>
-
 #include <qdbusargument.h>
 #include <qimage.h>
 #include <qobject.h>
@@ -23,14 +21,22 @@ struct DBusNotificationImage {
 const QDBusArgument& operator>>(const QDBusArgument& argument, DBusNotificationImage& pixmap);
 const QDBusArgument& operator<<(QDBusArgument& argument, const DBusNotificationImage& pixmap);
 
-class NotificationImage: public QsImageHandle {
+class NotificationImage: public QsIndexedImageHandle {
 public:
-	explicit NotificationImage(DBusNotificationImage image, QObject* parent)
-	    : QsImageHandle(QQuickAsyncImageProvider::Image, parent)
-	    , image(std::move(image)) {}
+	explicit NotificationImage(): QsIndexedImageHandle(QQuickAsyncImageProvider::Image) {}
+
+	[[nodiscard]] bool hasData() const { return !this->image.data.isEmpty(); }
+	void clear() { this->image.data.clear(); }
+
+	[[nodiscard]] DBusNotificationImage& writeImage() {
+		this->imageChanged();
+		return this->image;
+	}
 
 	QImage requestImage(const QString& id, QSize* size, const QSize& requestedSize) override;
 
+private:
 	DBusNotificationImage image;
 };
+
 } // namespace qs::service::notifications

src/services/notifications/notification.cpp

@@ -1,5 +1,4 @@
 #include "notification.hpp"
-#include <utility>
 
 #include <qcontainerfwd.h>
 #include <qdbusargument.h>
@@ -117,21 +116,20 @@ void Notification::updateProperties(
 
 	QString imagePath;
 
-	if (!imageDataName.isEmpty()) {
+	if (imageDataName.isEmpty()) {
+		this->mImagePixmap.clear();
+	} else {
 		auto value = hints.value(imageDataName).value<QDBusArgument>();
-		DBusNotificationImage image;
-		value >> image;
-		if (this->mImagePixmap) this->mImagePixmap->deleteLater();
-		this->mImagePixmap = new NotificationImage(std::move(image), this);
-		imagePath = this->mImagePixmap->url();
+		value >> this->mImagePixmap.writeImage();
+		imagePath = this->mImagePixmap.url();
 	}
 
 	// don't store giant byte arrays longer than necessary
 	hints.remove("image-data");
 	hints.remove("image_data");
 	hints.remove("icon_data");
 
-	if (!this->mImagePixmap) {
+	if (!this->mImagePixmap.hasData()) {
 		QString imagePathName;
 		if (hints.contains("image-path")) imagePathName = "image-path";
 		else if (hints.contains("image_path")) imagePathName = "image_path";

src/services/notifications/notification.hpp

@@ -12,11 +12,10 @@
 
 #include "../../core/retainable.hpp"
 #include "../../core/util.hpp"
+#include "dbusimage.hpp"
 
 namespace qs::service::notifications {
 
-class NotificationImage;
-
 ///! The urgency level of a Notification.
 /// See @@Notification.urgency.
 class NotificationUrgency: public QObject {
@@ -187,7 +186,7 @@ class Notification
 	quint32 mId;
 	NotificationCloseReason::Enum mCloseReason = NotificationCloseReason::Dismissed;
 	bool mLastGeneration = false;
-	NotificationImage* mImagePixmap = nullptr;
+	NotificationImage mImagePixmap;
 	QList<NotificationAction*> mActions;
 
 	// clang-format off

src/services/status_notifier/item.cpp

@@ -282,7 +282,7 @@ void StatusNotifierItem::onGetAllFailed() const {
 }
 
 TrayImageHandle::TrayImageHandle(StatusNotifierItem* item)
-    : QsImageHandle(QQmlImageProviderBase::Pixmap, item)
+    : QsImageHandle(QQmlImageProviderBase::Pixmap)
     , item(item) {}
 
 QPixmap