diff --git a/client.cpp b/client.cpp index f117815223..dedffbe05a 100644 --- a/client.cpp +++ b/client.cpp @@ -426,7 +426,7 @@ void Client::destroyClient() if (moveResizeMode) emit clientFinishUserMovedResized(this); emit windowClosed(this, del); - finishCompositing(); + finishCompositing(ReleaseReason::Destroyed); RuleBook::self()->discardUsed(this, true); // Remove ForceTemporarily rules StackingUpdatesBlocker blocker(workspace()); if (moveResizeMode) diff --git a/client.h b/client.h index ab6fd83395..96dd948ed3 100644 --- a/client.h +++ b/client.h @@ -490,7 +490,7 @@ public: bool hiddenPreview() const; ///< Window is mapped in order to get a window pixmap virtual bool setupCompositing(); - virtual void finishCompositing(); + void finishCompositing(ReleaseReason releaseReason = ReleaseReason::Release) override; void setBlockingCompositing(bool block); inline bool isBlockingCompositing() { return blocks_compositing; } diff --git a/composite.cpp b/composite.cpp index 187d05f58f..d5a730a89d 100644 --- a/composite.cpp +++ b/composite.cpp @@ -897,7 +897,7 @@ bool Toplevel::setupCompositing() return true; } -void Toplevel::finishCompositing() +void Toplevel::finishCompositing(ReleaseReason releaseReason) { if (damage_handle == XCB_NONE) return; @@ -907,7 +907,9 @@ void Toplevel::finishCompositing() delete effect_window; } - xcb_damage_destroy(connection(), damage_handle); + if (releaseReason != ReleaseReason::Destroyed) { + xcb_damage_destroy(connection(), damage_handle); + } damage_handle = XCB_NONE; damage_region = QRegion(); @@ -1153,9 +1155,9 @@ bool Client::setupCompositing() return true; } -void Client::finishCompositing() +void Client::finishCompositing(ReleaseReason releaseReason) { - Toplevel::finishCompositing(); + Toplevel::finishCompositing(releaseReason); updateVisibility(); if (!deleting) { // only recreate the decoration if we are not shutting down completely diff --git a/events.cpp b/events.cpp index 4f13abc138..c40a87befb 100644 --- a/events.cpp +++ b/events.cpp @@ -1566,10 +1566,30 @@ bool Unmanaged::windowEvent(xcb_generic_event_t *e) } const uint8_t eventType = e->response_type & ~0x80; switch (eventType) { - case XCB_UNMAP_NOTIFY: - workspace()->updateFocusMousePosition(Cursor::pos()); - release(); + case XCB_DESTROY_NOTIFY: + release(ReleaseReason::Destroyed); break; + case XCB_UNMAP_NOTIFY:{ + workspace()->updateFocusMousePosition(Cursor::pos()); + + // unmap notify might have been emitted due to a destroy notify + // but unmap notify gets emitted before the destroy notify, nevertheless at this + // point the window is already destroyed. This means any XCB request with the window + // will cause an error. + // To not run into these errors we try to wait for the destroy notify. For this we + // generate a round trip to the X server and wait a very short time span before + // handling the release. + updateXTime(); + // using 1 msec to not just move it at the end of the event loop but add an very short + // timespan to cover cases like unmap() followed by destroy(). The only other way to + // ensure that the window is not destroyed when we do the release handling is to grab + // the XServer which we do not want to do for an Unmanaged. The timespan of 1 msec is + // short enough to not cause problems in the close window animations. + // It's of course still possible that we miss the destroy in which case non-fatal + // X errors are reported to the event loop and logged by Qt. + QTimer::singleShot(1, this, SLOT(release())); + break; + } case XCB_CONFIGURE_NOTIFY: configureNotifyEvent(reinterpret_cast(e)); break; diff --git a/tests/unmapdestroytest.qml b/tests/unmapdestroytest.qml new file mode 100644 index 0000000000..363f58cf89 --- /dev/null +++ b/tests/unmapdestroytest.qml @@ -0,0 +1,86 @@ +/* + * Copyright 2014 Martin Gräßlin + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License or (at your option) version 3 or any later version + * accepted by the membership of KDE e.V. (or its successor approved + * by the membership of KDE e.V.), which shall act as a proxy + * defined in Section 14 of version 3 of the license. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . +*/ +import QtQuick 2.1 +import QtQuick.Window 2.1 +import QtQuick.Layouts 1.1 +import QtQuick.Controls 1.1 + +/* + * Small test application to test the difference between unmap and destroy. + * The window has two buttons: one called "Unmap" which will hide() the QWindow, + * one called "Destroy" which will close() the QWindow. + * + * The test application can be run with qmlscene: + * + * @code + * qmlscene unmapdestroytest.qml + * @endcode + * + * In order to test different modes, the test application understands some arguments: + * --unmanaged Creates an override redirect window + * --frameless Creates a frameless window (comparable Plasma Dialog) + */ + +Window { + id: window + visible: false + x: 0 + y: 0 + width: layout.implicitWidth + height: layout.implicitHeight + color: "black" + + RowLayout { + id: layout + Button { + Timer { + id: timer + interval: 2000 + running: false + repeat: false + onTriggered: window.show() + } + text: "unmap" + onClicked: { + timer.start(); + window.hide(); + } + } + Button { + text: "destroy" + onClicked: window.close() + } + } + + Component.onCompleted: { + var flags = Qt.Window; + for (var i = 0; i < Qt.application.arguments.length; ++i) { + var argument = Qt.application.arguments[i]; + if (argument == "--unmanaged") { + flags = flags | Qt.BypassWindowManagerHint; + } + if (argument == "--frameless") { + flags = flags | Qt.FramelessWindowHint + } + } + window.flags = flags; + window.visible = true; + } +} diff --git a/toplevel.h b/toplevel.h index 8fe138975b..305f667be7 100644 --- a/toplevel.h +++ b/toplevel.h @@ -48,6 +48,15 @@ class ClientMachine; class EffectWindowImpl; class Shadow; +/** + * Enum to describe the reason why a Toplevel has to be released. + */ +enum class ReleaseReason { + Release, ///< Normal Release after e.g. an Unmap notify event (window still valid) + Destroyed, ///< Release after an Destroy notify event (window no longer valid) + KWinShutsDown ///< Release on KWin Shutdown (window still valid) +}; + class Toplevel : public QObject, public KDecorationDefines { @@ -245,7 +254,7 @@ public: int depth() const; bool hasAlpha() const; virtual bool setupCompositing(); - virtual void finishCompositing(); + virtual void finishCompositing(ReleaseReason releaseReason = ReleaseReason::Release); bool updateUnredirectedState(); bool unredirected() const; void suspendUnredirect(bool suspend); diff --git a/unmanaged.cpp b/unmanaged.cpp index db5ab7c5bf..5f32007a98 100644 --- a/unmanaged.cpp +++ b/unmanaged.cpp @@ -84,20 +84,20 @@ bool Unmanaged::track(Window w) return true; } -void Unmanaged::release(bool on_shutdown) +void Unmanaged::release(ReleaseReason releaseReason) { Deleted* del = NULL; - if (!on_shutdown) { + if (releaseReason != ReleaseReason::KWinShutsDown) { del = Deleted::create(this); } emit windowClosed(this, del); - finishCompositing(); - if (!QWidget::find(window())) { // don't affect our own windows + finishCompositing(releaseReason); + if (!QWidget::find(window()) && releaseReason != ReleaseReason::Destroyed) { // don't affect our own windows if (Xcb::Extensions::self()->isShapeAvailable()) xcb_shape_select_input(connection(), window(), false); Xcb::selectInput(window(), XCB_EVENT_MASK_NO_EVENT); } - if (!on_shutdown) { + if (releaseReason != ReleaseReason::KWinShutsDown) { workspace()->removeUnmanaged(this); addWorkspaceRepaint(del->visibleRect()); disownDataPassedToDeleted(); diff --git a/unmanaged.h b/unmanaged.h index 7dbecc8879..6e63e69a5d 100644 --- a/unmanaged.h +++ b/unmanaged.h @@ -35,7 +35,6 @@ class Unmanaged public: explicit Unmanaged(); bool windowEvent(xcb_generic_event_t *e); - void release(bool on_shutdown = false); bool track(Window w); static void deleteUnmanaged(Unmanaged* c); virtual int desktop() const; @@ -51,6 +50,8 @@ public: void sendPointerButtonEvent(uint32_t button, InputRedirection::PointerButtonState state) override; void sendPointerAxisEvent(InputRedirection::PointerAxis axis, qreal delta) override; void sendKeybordKeyEvent(uint32_t key, InputRedirection::KeyboardKeyState state) override; +public Q_SLOTS: + void release(ReleaseReason releaseReason = ReleaseReason::Release); protected: virtual void debug(QDebug& stream) const; virtual bool shouldUnredirect() const; diff --git a/workspace.cpp b/workspace.cpp index 8840591699..80f9634a2e 100644 --- a/workspace.cpp +++ b/workspace.cpp @@ -438,7 +438,7 @@ Workspace::~Workspace() desktops.removeAll(c); } for (UnmanagedList::iterator it = unmanaged.begin(), end = unmanaged.end(); it != end; ++it) - (*it)->release(true); + (*it)->release(ReleaseReason::KWinShutsDown); xcb_delete_property(connection(), rootWindow(), atoms->kwin_running); delete RuleBook::self();