From ca1c72dd16699a5b8250e235f27e67f0f78adae9 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Thu, 23 Sep 2021 11:56:39 +0300 Subject: [PATCH] wayland: Fix cross cursor in Xwayland apps Startup code in plasmashell was changed so xsetroot is not called anymore, which is sort of fine. Unfortunately (or not?), it exposed a bug in kwin. Cursor::x11Cursor() only works in the standalone X11 session. On Wayland, Cursor::x11Cursor() will return XCB_NONE which results in seeing cross cursor when there should be arrow cursor. This change moves xcb_cursor_t look up code from X11Cursor to the base Cursor class. In hindsight, I would like to introduce a window manager class where the xcb cursor and other x11 specific code can be moved in the future for better encapsulation of platform-specific code. CCBUG: 442539 --- autotests/CMakeLists.txt | 2 + src/CMakeLists.txt | 1 + src/cursor.cpp | 49 +++++++++++++------ src/cursor.h | 13 +---- .../platforms/x11/standalone/CMakeLists.txt | 2 +- .../platforms/x11/standalone/x11cursor.cpp | 44 ----------------- .../platforms/x11/standalone/x11cursor.h | 4 -- 7 files changed, 40 insertions(+), 75 deletions(-) diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt index 3be89c879c..573543f497 100644 --- a/autotests/CMakeLists.txt +++ b/autotests/CMakeLists.txt @@ -190,6 +190,8 @@ target_link_libraries(testScriptedEffectLoader KF5::Notifications KF5::Package + XCB::CURSOR + kwineffects kwin4_effect_builtins ) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index e0fe644230..e755f8f227 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -212,6 +212,7 @@ target_link_libraries(kwin Plasma::KWaylandServer XCB::COMPOSITE + XCB::CURSOR XCB::DAMAGE XCB::GLX XCB::ICCCM diff --git a/src/cursor.cpp b/src/cursor.cpp index 12c0b98bfb..e04d72277a 100644 --- a/src/cursor.cpp +++ b/src/cursor.cpp @@ -24,6 +24,8 @@ #include #include +#include + namespace KWin { Cursors *Cursors::s_self = nullptr; @@ -125,6 +127,7 @@ void Cursor::updateTheme(const QString &name, int size) if (m_themeName != name || m_themeSize != size) { m_themeName = name; m_themeSize = size; + m_cursors.clear(); Q_EMIT themeChanged(); } } @@ -180,26 +183,44 @@ void Cursor::updateCursor(const QImage &image, const QPoint &hotspot) Q_EMIT cursorChanged(); } -xcb_cursor_t Cursor::getX11Cursor(CursorShape shape) -{ - Q_UNUSED(shape) - return XCB_CURSOR_NONE; -} - -xcb_cursor_t Cursor::getX11Cursor(const QByteArray &name) -{ - Q_UNUSED(name) - return XCB_CURSOR_NONE; -} - xcb_cursor_t Cursor::x11Cursor(CursorShape shape) { - return getX11Cursor(shape); + return x11Cursor(shape.name()); } xcb_cursor_t Cursor::x11Cursor(const QByteArray &name) { - return getX11Cursor(name); + Q_ASSERT(kwinApp()->x11Connection()); + auto it = m_cursors.constFind(name); + if (it != m_cursors.constEnd()) { + return it.value(); + } + + if (name.isEmpty()) { + return XCB_CURSOR_NONE; + } + + xcb_cursor_context_t *ctx; + if (xcb_cursor_context_new(kwinApp()->x11Connection(), kwinApp()->x11DefaultScreen(), &ctx) < 0) { + return XCB_CURSOR_NONE; + } + + xcb_cursor_t cursor = xcb_cursor_load_cursor(ctx, name.constData()); + if (cursor == XCB_CURSOR_NONE) { + const auto &names = Cursor::cursorAlternativeNames(name); + for (const QByteArray &cursorName : names) { + cursor = xcb_cursor_load_cursor(ctx, cursorName.constData()); + if (cursor != XCB_CURSOR_NONE) { + break; + } + } + } + if (cursor != XCB_CURSOR_NONE) { + m_cursors.insert(name, cursor); + } + + xcb_cursor_context_free(ctx); + return cursor; } void Cursor::doSetPos() diff --git a/src/cursor.h b/src/cursor.h index 4d73e47c15..744e5677fd 100644 --- a/src/cursor.h +++ b/src/cursor.h @@ -189,18 +189,6 @@ Q_SIGNALS: void rendered(const QRect &geometry); protected: - /** - * Called from x11Cursor to actually retrieve the X11 cursor. Base implementation returns - * a null cursor, an implementing subclass should implement this method if it can provide X11 - * mouse cursors. - */ - virtual xcb_cursor_t getX11Cursor(CursorShape shape); - /** - * Called from x11Cursor to actually retrieve the X11 cursor. Base implementation returns - * a null cursor, an implementing subclass should implement this method if it can provide X11 - * mouse cursors. - */ - virtual xcb_cursor_t getX11Cursor(const QByteArray &name); /** * Performs the actual warping of the cursor. */ @@ -251,6 +239,7 @@ private Q_SLOTS: private: void updateTheme(const QString &name, int size); void loadThemeFromKConfig(); + QHash m_cursors; QPoint m_pos; QPoint m_hotspot; QImage m_image; diff --git a/src/plugins/platforms/x11/standalone/CMakeLists.txt b/src/plugins/platforms/x11/standalone/CMakeLists.txt index fc17a5374d..899b0b0dc3 100644 --- a/src/plugins/platforms/x11/standalone/CMakeLists.txt +++ b/src/plugins/platforms/x11/standalone/CMakeLists.txt @@ -17,7 +17,7 @@ set(X11PLATFORM_SOURCES add_library(KWinX11Platform MODULE ${X11PLATFORM_SOURCES}) set_target_properties(KWinX11Platform PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin/org.kde.kwin.platforms/") -target_link_libraries(KWinX11Platform eglx11common kwin kwinxrenderutils SceneOpenGLBackend VsyncSupport Qt::X11Extras XCB::CURSOR KF5::Crash X11::X11) +target_link_libraries(KWinX11Platform eglx11common kwin kwinxrenderutils SceneOpenGLBackend VsyncSupport Qt::X11Extras KF5::Crash X11::X11) if (X11_Xi_FOUND) target_sources(KWinX11Platform PRIVATE xinputintegration.cpp) target_link_libraries(KWinX11Platform X11::Xi) diff --git a/src/plugins/platforms/x11/standalone/x11cursor.cpp b/src/plugins/platforms/x11/standalone/x11cursor.cpp index 852f0e660f..7529a1852e 100644 --- a/src/plugins/platforms/x11/standalone/x11cursor.cpp +++ b/src/plugins/platforms/x11/standalone/x11cursor.cpp @@ -16,8 +16,6 @@ #include #include -#include - namespace KWin { @@ -37,8 +35,6 @@ X11Cursor::X11Cursor(QObject *parent, bool xInputSupport) m_mousePollingTimer->setInterval(50); connect(m_mousePollingTimer, &QTimer::timeout, this, &X11Cursor::mousePolled); - connect(this, &Cursor::themeChanged, this, [this] { m_cursors.clear(); }); - if (m_hasXInput) { connect(qApp->eventDispatcher(), &QAbstractEventDispatcher::aboutToBlock, this, &X11Cursor::aboutToBlock); } @@ -134,46 +130,6 @@ void X11Cursor::mousePolled() } } -xcb_cursor_t X11Cursor::getX11Cursor(CursorShape shape) -{ - return getX11Cursor(shape.name()); -} - -xcb_cursor_t X11Cursor::getX11Cursor(const QByteArray &name) -{ - auto it = m_cursors.constFind(name); - if (it != m_cursors.constEnd()) { - return it.value(); - } - return createCursor(name); -} - -xcb_cursor_t X11Cursor::createCursor(const QByteArray &name) -{ - if (name.isEmpty()) { - return XCB_CURSOR_NONE; - } - xcb_cursor_context_t *ctx; - if (xcb_cursor_context_new(kwinApp()->x11Connection(), kwinApp()->x11DefaultScreen(), &ctx) < 0) { - return XCB_CURSOR_NONE; - } - xcb_cursor_t cursor = xcb_cursor_load_cursor(ctx, name.constData()); - if (cursor == XCB_CURSOR_NONE) { - const auto &names = cursorAlternativeNames(name); - for (auto cit = names.begin(); cit != names.end(); ++cit) { - cursor = xcb_cursor_load_cursor(ctx, (*cit).constData()); - if (cursor != XCB_CURSOR_NONE) { - break; - } - } - } - if (cursor != XCB_CURSOR_NONE) { - m_cursors.insert(name, cursor); - } - xcb_cursor_context_free(ctx); - return cursor; -} - void X11Cursor::notifyCursorChanged() { if (!isCursorTracking()) { diff --git a/src/plugins/platforms/x11/standalone/x11cursor.h b/src/plugins/platforms/x11/standalone/x11cursor.h index adf302b493..ad97e7b722 100644 --- a/src/plugins/platforms/x11/standalone/x11cursor.h +++ b/src/plugins/platforms/x11/standalone/x11cursor.h @@ -35,8 +35,6 @@ public: void notifyCursorChanged(); protected: - xcb_cursor_t getX11Cursor(CursorShape shape) override; - xcb_cursor_t getX11Cursor(const QByteArray &name) override; void doSetPos() override; void doGetPos() override; void doStartMousePolling() override; @@ -54,8 +52,6 @@ private Q_SLOTS: void mousePolled(); void aboutToBlock(); private: - xcb_cursor_t createCursor(const QByteArray &name); - QHash m_cursors; xcb_timestamp_t m_timeStamp; uint16_t m_buttonMask; QTimer *m_resetTimeStampTimer;