core/qsmenu!: improve menu layout change UX

outfoxxed · outfoxxed · commit a053373d575b · 2024-12-13T01:30:11.000-08:00
Exposes QsMenuOpener.children as an ObjectModel instead of a list to
allow smoother layout change handling in custom menu renderers.

Fixes QsMenuAnchor/platform menus closing whenever menu content changes.
diff --git a/src/core/model.cpp b/src/core/model.cpp
@@ -71,6 +71,22 @@ bool UntypedObjectModel::removeObject(const QObject* object) {
 	return true;
 }
 
+void UntypedObjectModel::diffUpdate(const QVector<QObject*>& newValues) {
+	for (qsizetype i = 0; i < this->valuesList.length();) {
+		if (newValues.contains(this->valuesList.at(i))) i++;
+		else this->removeAt(i);
+	}
+
+	qsizetype oi = 0;
+	for (auto* object: newValues) {
+		if (this->valuesList.length() == oi || this->valuesList.at(oi) != object) {
+			this->insertObject(object, oi);
+		}
+
+		oi++;
+	}
+}
+
 qsizetype UntypedObjectModel::indexOf(QObject* object) { return this->valuesList.indexOf(object); }
 
 UntypedObjectModel* UntypedObjectModel::emptyInstance() {
diff --git a/src/core/model.hpp b/src/core/model.hpp
@@ -73,6 +73,9 @@ class UntypedObjectModel: public QAbstractListModel {
 	void insertObject(QObject* object, qsizetype index = -1);
 	bool removeObject(const QObject* object);
 
+	// Assumes only one instance of a specific value
+	void diffUpdate(const QVector<QObject*>& newValues);
+
 	QVector<QObject*> valuesList;
 
 private:
@@ -97,6 +100,11 @@ class ObjectModel: public UntypedObjectModel {
 
 	void removeObject(const T* object) { this->UntypedObjectModel::removeObject(object); }
 
+	// Assumes only one instance of a specific value
+	void diffUpdate(const QVector<T*>& newValues) {
+		this->UntypedObjectModel::diffUpdate(*std::bit_cast<const QVector<QObject*>*>(&newValues));
+	}
+
 	static ObjectModel<T>* emptyInstance() {
 		return static_cast<ObjectModel<T>*>(UntypedObjectModel::emptyInstance());
 	}
diff --git a/src/core/platformmenu.cpp b/src/core/platformmenu.cpp
@@ -20,6 +20,7 @@
 #include "../window/proxywindow.hpp"
 #include "../window/windowinterface.hpp"
 #include "iconprovider.hpp"
+#include "model.hpp"
 #include "platformmenu_p.hpp"
 #include "popupanchor.hpp"
 #include "qsmenu.hpp"
@@ -61,6 +62,7 @@ PlatformMenuEntry::PlatformMenuEntry(QsMenuEntry* menu): QObject(menu), menu(men
 	QObject::connect(menu, &QsMenuEntry::buttonTypeChanged, this, &PlatformMenuEntry::onButtonTypeChanged);
 	QObject::connect(menu, &QsMenuEntry::checkStateChanged, this, &PlatformMenuEntry::onCheckStateChanged);
 	QObject::connect(menu, &QsMenuEntry::hasChildrenChanged, this, &PlatformMenuEntry::relayoutParent);
+	QObject::connect(menu->children(), &UntypedObjectModel::valuesChanged, this, &PlatformMenuEntry::relayout);
 	// clang-format on
 }
 
@@ -178,10 +180,10 @@ void PlatformMenuEntry::relayout() {
 			this->qmenu->setIcon(getCurrentEngineImageAsIcon(icon));
 		}
 
-		auto children = this->menu->children();
-		auto len = children.count(&children);
+		const auto& children = this->menu->children()->valueList();
+		auto len = children.count();
 		for (auto i = 0; i < len; i++) {
-			auto* child = children.at(&children, i);
+			auto* child = children.at(i);
 
 			auto* instance = new PlatformMenuEntry(child);
 			QObject::connect(instance, &QObject::destroyed, this, &PlatformMenuEntry::onChildDestroyed);
diff --git a/src/core/qsmenu.cpp b/src/core/qsmenu.cpp
@@ -4,8 +4,8 @@
 #include <qobject.h>
 #include <qqmllist.h>
 #include <qtmetamacros.h>
-#include <qtypes.h>
 
+#include "model.hpp"
 #include "platformmenu.hpp"
 
 using namespace qs::menu::platform;
@@ -34,15 +34,6 @@ void QsMenuEntry::display(QObject* parentWindow, int relativeX, int relativeY) {
 	if (!success) delete platform;
 }
 
-QQmlListProperty<QsMenuEntry> QsMenuEntry::emptyChildren(QObject* parent) {
-	return QQmlListProperty<QsMenuEntry>(
-	    parent,
-	    nullptr,
-	    &QsMenuEntry::childCount,
-	    &QsMenuEntry::childAt
-	);
-}
-
 void QsMenuEntry::ref() {
 	this->refcount++;
 	if (this->refcount == 1) emit this->opened();
@@ -53,7 +44,9 @@ void QsMenuEntry::unref() {
 	if (this->refcount == 0) emit this->closed();
 }
 
-QQmlListProperty<QsMenuEntry> QsMenuEntry::children() { return QsMenuEntry::emptyChildren(this); }
+ObjectModel<QsMenuEntry>* QsMenuEntry::children() {
+	return ObjectModel<QsMenuEntry>::emptyInstance();
+}
 
 QsMenuOpener::~QsMenuOpener() {
 	if (this->mMenu) {
@@ -83,13 +76,6 @@ void QsMenuOpener::setMenu(QsMenuHandle* menu) {
 	if (menu != nullptr) {
 		auto onMenuChanged = [this, menu]() {
 			if (menu->menu()) {
-				QObject::connect(
-				    menu->menu(),
-				    &QsMenuEntry::childrenChanged,
-				    this,
-				    &QsMenuOpener::childrenChanged
-				);
-
 				menu->menu()->ref();
 			}
 
@@ -113,19 +99,12 @@ void QsMenuOpener::onMenuDestroyed() {
 	emit this->childrenChanged();
 }
 
-QQmlListProperty<QsMenuEntry> QsMenuOpener::children() {
+ObjectModel<QsMenuEntry>* QsMenuOpener::children() {
 	if (this->mMenu && this->mMenu->menu()) {
 		return this->mMenu->menu()->children();
 	} else {
-		return QsMenuEntry::emptyChildren(this);
+		return ObjectModel<QsMenuEntry>::emptyInstance();
 	}
 }
 
-qsizetype QsMenuEntry::childCount(QQmlListProperty<QsMenuEntry>* /*property*/) { return 0; }
-
-QsMenuEntry*
-QsMenuEntry::childAt(QQmlListProperty<QsMenuEntry>* /*property*/, qsizetype /*index*/) {
-	return nullptr;
-}
-
 } // namespace qs::menu
diff --git a/src/core/qsmenu.hpp b/src/core/qsmenu.hpp
@@ -9,6 +9,7 @@
 #include <qtypes.h>
 
 #include "doc.hpp"
+#include "model.hpp"
 
 namespace qs::menu {
 
@@ -107,9 +108,7 @@ class QsMenuEntry: public QsMenuHandle {
 	void ref();
 	void unref();
 
-	[[nodiscard]] virtual QQmlListProperty<QsMenuEntry> children();
-
-	static QQmlListProperty<QsMenuEntry> emptyChildren(QObject* parent);
+	[[nodiscard]] virtual ObjectModel<QsMenuEntry>* children();
 
 signals:
 	/// Send a trigger/click signal to the menu entry.
@@ -125,12 +124,8 @@ class QsMenuEntry: public QsMenuHandle {
 	void buttonTypeChanged();
 	void checkStateChanged();
 	void hasChildrenChanged();
-	QSDOC_HIDE void childrenChanged();
 
 private:
-	static qsizetype childCount(QQmlListProperty<QsMenuEntry>* property);
-	static QsMenuEntry* childAt(QQmlListProperty<QsMenuEntry>* property, qsizetype index);
-
 	qsizetype refcount = 0;
 };
 
@@ -140,7 +135,8 @@ class QsMenuOpener: public QObject {
 	/// The menu to retrieve children from.
 	Q_PROPERTY(qs::menu::QsMenuHandle* menu READ menu WRITE setMenu NOTIFY menuChanged);
 	/// The children of the given menu.
-	Q_PROPERTY(QQmlListProperty<qs::menu::QsMenuEntry> children READ children NOTIFY childrenChanged);
+	QSDOC_TYPE_OVERRIDE(ObjectModel<qs::menu::QsMenuEntry>*);
+	Q_PROPERTY(UntypedObjectModel* children READ children NOTIFY childrenChanged);
 	QML_ELEMENT;
 
 public:
@@ -151,7 +147,7 @@ class QsMenuOpener: public QObject {
 	[[nodiscard]] QsMenuHandle* menu() const;
 	void setMenu(QsMenuHandle* menu);
 
-	[[nodiscard]] QQmlListProperty<QsMenuEntry> children();
+	[[nodiscard]] ObjectModel<QsMenuEntry>* children();
 
 signals:
 	void menuChanged();
diff --git a/src/dbus/dbusmenu/dbusmenu.cpp b/src/dbus/dbusmenu/dbusmenu.cpp
@@ -21,6 +21,7 @@
 #include <qvariant.h>
 
 #include "../../core/iconimageprovider.hpp"
+#include "../../core/model.hpp"
 #include "../../core/qsmenu.hpp"
 #include "../../dbus/properties.hpp"
 #include "dbus_menu.h"
@@ -95,22 +96,8 @@ void DBusMenuItem::updateLayout() const {
 
 bool DBusMenuItem::hasChildren() const { return this->displayChildren || this->id == 0; }
 
-QQmlListProperty<QsMenuEntry> DBusMenuItem::children() {
-	return QQmlListProperty<QsMenuEntry>(
-	    this,
-	    nullptr,
-	    &DBusMenuItem::childrenCount,
-	    &DBusMenuItem::childAt
-	);
-}
-
-qsizetype DBusMenuItem::childrenCount(QQmlListProperty<QsMenuEntry>* property) {
-	return reinterpret_cast<DBusMenuItem*>(property->object)->enabledChildren.count();
-}
-
-QsMenuEntry* DBusMenuItem::childAt(QQmlListProperty<QsMenuEntry>* property, qsizetype index) {
-	auto* item = reinterpret_cast<DBusMenuItem*>(property->object);
-	return item->menu->items.value(item->enabledChildren.at(index));
+ObjectModel<QsMenuEntry>* DBusMenuItem::children() {
+	return reinterpret_cast<ObjectModel<QsMenuEntry>*>(&this->enabledChildren);
 }
 
 void DBusMenuItem::updateProperties(const QVariantMap& properties, const QStringList& removed) {
@@ -270,14 +257,13 @@ void DBusMenuItem::updateProperties(const QVariantMap& properties, const QString
 }
 
 void DBusMenuItem::onChildrenUpdated() {
-	this->enabledChildren.clear();
-
+	QVector<DBusMenuItem*> children;
 	for (auto child: this->mChildren) {
 		auto* item = this->menu->items.value(child);
-		if (item->visible) this->enabledChildren.push_back(child);
+		if (item->visible) children.append(item);
 	}
 
-	emit this->childrenChanged();
+	this->enabledChildren.diffUpdate(children);
 }
 
 QDebug operator<<(QDebug debug, DBusMenuItem* item) {
@@ -388,7 +374,7 @@ void DBusMenu::updateLayoutRecursive(
 			    [&](const DBusMenuLayout& layout) { return layout.id == *iter; }
 			);
 
-			if (existing == layout.children.end()) {
+			if (!item->mShowChildren || existing == layout.children.end()) {
 				qCDebug(logDbusMenu) << "Removing missing layout item" << this->items.value(*iter) << "from"
 				                     << item;
 				this->removeRecursive(*iter);
@@ -402,15 +388,23 @@ void DBusMenu::updateLayoutRecursive(
 		for (const auto& child: layout.children) {
 			if (item->mShowChildren && !item->mChildren.contains(child.id)) {
 				qCDebug(logDbusMenu) << "Creating new layout item" << child.id << "in" << item;
-				item->mChildren.push_back(child.id);
+				// item->mChildren.push_back(child.id);
 				this->items.insert(child.id, nullptr);
 				childrenChanged = true;
 			}
 
 			this->updateLayoutRecursive(child, item, depth - 1);
 		}
 
-		if (childrenChanged) item->onChildrenUpdated();
+		if (childrenChanged) {
+			// reset to preserve order
+			item->mChildren.clear();
+			for (const auto& child: layout.children) {
+				item->mChildren.push_back(child.id);
+			}
+
+			item->onChildrenUpdated();
+		}
 	}
 
 	if (item->mShowChildren && !item->childrenLoaded) {
@@ -554,6 +548,7 @@ void DBusMenuHandle::onMenuPathChanged() {
 		this->mMenu->setParent(this);
 
 		QObject::connect(&this->mMenu->rootItem, &DBusMenuItem::layoutUpdated, this, [this]() {
+			QObject::disconnect(&this->mMenu->rootItem, &DBusMenuItem::layoutUpdated, this, nullptr);
 			this->loaded = true;
 			emit this->menuChanged();
 		});
diff --git a/src/dbus/dbusmenu/dbusmenu.hpp b/src/dbus/dbusmenu/dbusmenu.hpp
@@ -15,6 +15,7 @@
 
 #include "../../core/doc.hpp"
 #include "../../core/imageprovider.hpp"
+#include "../../core/model.hpp"
 #include "../../core/qsmenu.hpp"
 #include "../properties.hpp"
 #include "dbus_menu_types.hpp"
@@ -65,7 +66,7 @@ class DBusMenuItem: public QsMenuEntry {
 	[[nodiscard]] bool isShowingChildren() const;
 	void setShowChildrenRecursive(bool showChildren);
 
-	[[nodiscard]] QQmlListProperty<menu::QsMenuEntry> children() override;
+	[[nodiscard]] ObjectModel<QsMenuEntry>* children() override;
 
 	void updateProperties(const QVariantMap& properties, const QStringList& removed = {});
 	void onChildrenUpdated();
@@ -96,11 +97,8 @@ private slots:
 	menu::QsMenuButtonType::Enum mButtonType = menu::QsMenuButtonType::None;
 	Qt::CheckState mCheckState = Qt::Unchecked;
 	bool displayChildren = false;
-	QVector<qint32> enabledChildren;
+	ObjectModel<DBusMenuItem> enabledChildren {this};
 	DBusMenuItem* parentMenu = nullptr;
-
-	static qsizetype childrenCount(QQmlListProperty<menu::QsMenuEntry>* property);
-	static menu::QsMenuEntry* childAt(QQmlListProperty<menu::QsMenuEntry>* property, qsizetype index);
 };
 
 QDebug operator<<(QDebug debug, DBusMenuItem* item);
