wayland/shortcuts-inhibit: code review feedback

Author: bbedward

src/wayland/shortcuts_inhibit/inhibitor.cpp

@@ -3,7 +3,6 @@
 #include <private/qwaylandwindow_p.h>
 #include <qlogging.h>
 #include <qobject.h>
-#include <qtmetamacros.h>
 
 #include "../../window/proxywindow.hpp"
 #include "../../window/windowinterface.hpp"
@@ -24,35 +23,18 @@ ShortcutsInhibitor::~ShortcutsInhibitor() {
 	auto* manager = impl::ShortcutsInhibitManager::instance();
 	if (!manager) return;
 
-	QObject::disconnect(this->inhibitor, nullptr, this, nullptr);
 	manager->unrefShortcutsInhibitor(this->inhibitor);
 }
 
 QObject* ShortcutsInhibitor::window() const { return this->bWindowObject; }
 
-void ShortcutsInhibitor::setWindow(QObject* window) {
-	if (window == this->bWindowObject) return;
-
-	auto* proxyWindow = qobject_cast<ProxyWindowBase*>(window);
-
-	if (proxyWindow == nullptr) {
-		if (auto* iface = qobject_cast<WindowInterface*>(window)) {
-			proxyWindow = iface->proxyWindow();
-		}
-	}
-
-	this->bWindowObject = proxyWindow ? window : nullptr;
-}
-
-bool ShortcutsInhibitor::isActive() const {
-	return this->inhibitor ? this->inhibitor->isActive() : false;
-}
+void ShortcutsInhibitor::setWindow(QObject* window) { this->bWindowObject = window; }
 
 void ShortcutsInhibitor::boundWindowChanged() {
 	auto* window = this->bBoundWindow.value();
 	auto* proxyWindow = qobject_cast<ProxyWindowBase*>(window);
 
-	if (proxyWindow == nullptr) {
+	if (!proxyWindow) {
 		if (auto* iface = qobject_cast<WindowInterface*>(window)) {
 			proxyWindow = iface->proxyWindow();
 		}
@@ -90,8 +72,6 @@ void ShortcutsInhibitor::boundWindowChanged() {
 
 		this->onWindowVisibilityChanged();
 	}
-
-	emit this->windowChanged();
 }
 
 void ShortcutsInhibitor::onWindowDestroyed() {
@@ -107,6 +87,7 @@ void ShortcutsInhibitor::onWindowVisibilityChanged() {
 	if (!window->handle()) window->create();
 
 	auto* waylandWindow = dynamic_cast<QWaylandWindow*>(window->handle());
+	if (!waylandWindow) return;
 	if (waylandWindow == this->mWaylandWindow) return;
 	this->mWaylandWindow = waylandWindow;
 
@@ -147,19 +128,15 @@ void ShortcutsInhibitor::onWaylandSurfaceCreated() {
 	}
 
 	if (this->inhibitor) {
-		QObject::disconnect(this->inhibitor, nullptr, this, nullptr);
-		manager->unrefShortcutsInhibitor(this->inhibitor);
-		this->inhibitor = nullptr;
+		qFatal("ShortcutsInhibitor: inhibitor already exists when creating surface");
 	}
+
 	this->inhibitor = manager->createShortcutsInhibitor(this->mWaylandWindow);
 
 	if (this->inhibitor) {
-		QObject::connect(
-		    this->inhibitor,
-		    &impl::ShortcutsInhibitor::activeChanged,
-		    this,
-		    &ShortcutsInhibitor::activeChanged
-		);
+		this->bActive.setBinding([this]() {
+			return this->inhibitor ? this->inhibitor->bindableActive().value() : false;
+		});
 	}
 }
 
@@ -169,12 +146,8 @@ void ShortcutsInhibitor::onWaylandSurfaceDestroyed() {
 	auto* manager = impl::ShortcutsInhibitManager::instance();
 	if (!manager) return;
 
-	QObject::disconnect(this->inhibitor, nullptr, this, nullptr);
-	auto wasActive = this->inhibitor->isActive();
 	manager->unrefShortcutsInhibitor(this->inhibitor);
 	this->inhibitor = nullptr;
-
-	if (wasActive) { emit this->activeChanged(); }
 }
 
 } // namespace qs::wayland::shortcuts_inhibit

src/wayland/shortcuts_inhibit/inhibitor.hpp

@@ -38,7 +38,7 @@ class ShortcutsInhibitor: public QObject {
 	/// The compositor may deactivate the inhibitor if the user requests normal shortcuts to be restored.
 	/// When inactive, there is no way to programmatically reactivate it - the user must do so through
 	/// compositor-specific mechanisms.
-	Q_PROPERTY(bool active READ isActive NOTIFY activeChanged);
+	Q_PROPERTY(bool active READ default NOTIFY activeChanged BINDABLE bindableActive);
 	// clang-format on
 
 public:
@@ -50,7 +50,7 @@ class ShortcutsInhibitor: public QObject {
 	void setWindow(QObject* window);
 
 	[[nodiscard]] QBindable<bool> bindableEnabled() { return &this->bEnabled; }
-	[[nodiscard]] bool isActive() const;
+	[[nodiscard]] QBindable<bool> bindableActive() { return &this->bActive; }
 
 signals:
 	void enabledChanged();
@@ -75,6 +75,7 @@ private slots:
 	Q_OBJECT_BINDABLE_PROPERTY(ShortcutsInhibitor, bool, bEnabled, &ShortcutsInhibitor::enabledChanged);
 	Q_OBJECT_BINDABLE_PROPERTY(ShortcutsInhibitor, QObject*, bWindowObject, &ShortcutsInhibitor::windowChanged);
 	Q_OBJECT_BINDABLE_PROPERTY(ShortcutsInhibitor, QObject*, bBoundWindow, &ShortcutsInhibitor::boundWindowChanged);
+	Q_OBJECT_BINDABLE_PROPERTY(ShortcutsInhibitor, bool, bActive, &ShortcutsInhibitor::activeChanged);
 	// clang-format on
 };
 

src/wayland/shortcuts_inhibit/proto.cpp

@@ -6,7 +6,7 @@
 #include <private/qwaylandwindow_p.h>
 #include <qlogging.h>
 #include <qloggingcategory.h>
-#include <qtmetamacros.h>
+#include <qpair.h>
 #include <qwaylandclientextension.h>
 
 #include "../../core/logcat.hpp"
@@ -40,40 +40,46 @@ ShortcutsInhibitManager::createShortcutsInhibitor(QtWaylandClient::QWaylandWindo
 	auto* wlSurface = surface->surface();
 
 	if (this->inhibitors.contains(wlSurface)) {
-		auto* inhibitor = this->inhibitors.value(wlSurface);
-		this->refCounts[inhibitor]++;
-		qCDebug(logShortcutsInhibit) << "Reusing existing inhibitor" << inhibitor << "for surface"
-		                             << wlSurface << "- refcount:" << this->refCounts[inhibitor];
-		return inhibitor;
+		auto& pair = this->inhibitors[wlSurface];
+		pair.second++;
+		qCDebug(logShortcutsInhibit) << "Reusing existing inhibitor" << pair.first << "for surface"
+		                             << wlSurface << "- refcount:" << pair.second;
+		return pair.first;
 	}
 
 	auto* inhibitor =
 	    new ShortcutsInhibitor(this->inhibit_shortcuts(wlSurface, inputDevice->object()), wlSurface);
-	this->inhibitors.insert(wlSurface, inhibitor);
-	this->refCounts.insert(inhibitor, 1);
+	this->inhibitors.insert(wlSurface, qMakePair(inhibitor, 1));
 	qCDebug(logShortcutsInhibit) << "Created inhibitor" << inhibitor << "for surface" << wlSurface;
 	return inhibitor;
 }
 
 void ShortcutsInhibitManager::refShortcutsInhibitor(ShortcutsInhibitor* inhibitor) {
-	if (inhibitor && this->refCounts.contains(inhibitor)) {
-		this->refCounts[inhibitor]++;
-		qCDebug(logShortcutsInhibit) << "Incremented refcount for inhibitor" << inhibitor
-		                             << "- refcount:" << this->refCounts[inhibitor];
-	}
+	if (!inhibitor) return;
+
+	auto* surface = inhibitor->surface();
+	if (!this->inhibitors.contains(surface)) return;
+
+	auto& pair = this->inhibitors[surface];
+	pair.second++;
+	qCDebug(logShortcutsInhibit) << "Incremented refcount for inhibitor" << inhibitor
+	                             << "- refcount:" << pair.second;
 }
 
 void ShortcutsInhibitManager::unrefShortcutsInhibitor(ShortcutsInhibitor* inhibitor) {
-	if (!inhibitor || !this->refCounts.contains(inhibitor)) return;
+	if (!inhibitor) return;
+
+	auto* surface = inhibitor->surface();
+	if (!this->inhibitors.contains(surface)) return;
 
-	this->refCounts[inhibitor]--;
+	auto& pair = this->inhibitors[surface];
+	pair.second--;
 	qCDebug(logShortcutsInhibit) << "Decremented refcount for inhibitor" << inhibitor
-	                             << "- refcount:" << this->refCounts[inhibitor];
+	                             << "- refcount:" << pair.second;
 
-	if (this->refCounts[inhibitor] <= 0) {
+	if (pair.second <= 0) {
 		qCDebug(logShortcutsInhibit) << "Refcount reached 0, destroying inhibitor" << inhibitor;
-		this->inhibitors.remove(inhibitor->surface());
-		this->refCounts.remove(inhibitor);
+		this->inhibitors.remove(surface);
 		delete inhibitor;
 	}
 }
@@ -85,14 +91,12 @@ ShortcutsInhibitor::~ShortcutsInhibitor() {
 
 void ShortcutsInhibitor::zwp_keyboard_shortcuts_inhibitor_v1_active() {
 	qCDebug(logShortcutsInhibit) << "Inhibitor became active" << this;
-	this->mActive = true;
-	emit this->activeChanged();
+	this->bActive = true;
 }
 
 void ShortcutsInhibitor::zwp_keyboard_shortcuts_inhibitor_v1_inactive() {
 	qCDebug(logShortcutsInhibit) << "Inhibitor became inactive" << this;
-	this->mActive = false;
-	emit this->activeChanged();
+	this->bActive = false;
 }
 
 } // namespace qs::wayland::shortcuts_inhibit::impl

src/wayland/shortcuts_inhibit/proto.hpp

@@ -1,6 +1,8 @@
 #pragma once
 
 #include <private/qwaylandwindow_p.h>
+#include <qpair.h>
+#include <qproperty.h>
 #include <qtclasshelpermacros.h>
 #include <qwayland-keyboard-shortcuts-inhibit-unstable-v1.h>
 #include <qwaylandclientextension.h>
@@ -24,8 +26,7 @@ class ShortcutsInhibitManager
 	static ShortcutsInhibitManager* instance();
 
 private:
-	QHash<wl_surface*, ShortcutsInhibitor*> inhibitors;
-	QHash<ShortcutsInhibitor*, int> refCounts;
+	QHash<wl_surface*, QPair<ShortcutsInhibitor*, int>> inhibitors;
 };
 
 class ShortcutsInhibitor
@@ -41,7 +42,8 @@ class ShortcutsInhibitor
 	~ShortcutsInhibitor() override;
 	Q_DISABLE_COPY_MOVE(ShortcutsInhibitor);
 
-	[[nodiscard]] bool isActive() const { return this->mActive; }
+	[[nodiscard]] QBindable<bool> bindableActive() { return &this->bActive; }
+	[[nodiscard]] bool isActive() const { return this->bActive; }
 	[[nodiscard]] wl_surface* surface() const { return this->mSurface; }
 
 signals:
@@ -52,8 +54,8 @@ class ShortcutsInhibitor
 	void zwp_keyboard_shortcuts_inhibitor_v1_inactive() override;
 
 private:
-	bool mActive = false;
 	wl_surface* mSurface;
+	Q_OBJECT_BINDABLE_PROPERTY(ShortcutsInhibitor, bool, bActive, &ShortcutsInhibitor::activeChanged);
 };
 
 } // namespace qs::wayland::shortcuts_inhibit::impl