[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-commits
Subject: [kwin] /: Try to wait for DESTROY_NOTIFY before releasing an Unmanaged
From: Martin_Gräßlin <mgraesslin () kde ! org>
Date: 2014-04-16 11:36:42
Message-ID: E1WaO8w-00021i-SY () scm ! kde ! org
[Download RAW message or body]
Git commit 93e5ebac63486678caa1be86c96a157d049e5564 by Martin Gräßlin.
Committed on 07/04/2014 at 14:23.
Pushed by graesslin into branch 'master'.
Try to wait for DESTROY_NOTIFY before releasing an Unmanaged
So far the Unmanaged got released after an XCB_UNMAP_NOTIFY. This event
gets created after xcb_unmap_window or after xcb_destroy_window. In the
latter case the window is already distroyed and any of KWin's cleanup
calls will cause a BadWindow (or similar) error.
The idea to circumvent these errors is to try to wait for the
DESTROY_NOTIFY event. To do so the processing of the release is slightly
delayed. If KWin gets the destroy notify before the delay times out the
Unamanged gets released immediately but with a Destroy flag. For this a
new enum ReleaseToplevel is introduced and Unmanage::release takes this
as an argument instead of the bool which indicated OnShutdown. Also this
enum is added to Toplevel::finishCompositing so that it can ignore the
destroyed case and not generate an error.
REVIEW: 117422
M +1 -1 client.cpp
M +1 -1 client.h
M +6 -4 composite.cpp
M +22 -2 events.cpp
A +86 -0 tests/unmapdestroytest.qml [License: GPL (v2/3)]
M +10 -1 toplevel.h
M +5 -5 unmanaged.cpp
M +2 -1 unmanaged.h
M +1 -1 workspace.cpp
http://commits.kde.org/kwin/93e5ebac63486678caa1be86c96a157d049e5564
diff --git a/client.cpp b/client.cpp
index f117815..dedffbe 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 ab6fd83..96dd948 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 187d05f..d5a730a 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 4f13abc..c40a87b 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:
+ case XCB_DESTROY_NOTIFY:
+ release(ReleaseReason::Destroyed);
+ break;
+ case XCB_UNMAP_NOTIFY:{
workspace()->updateFocusMousePosition(Cursor::pos());
- release();
+
+ // 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<xcb_configure_notify_event_t*>(e));
break;
diff --git a/tests/unmapdestroytest.qml b/tests/unmapdestroytest.qml
new file mode 100644
index 0000000..363f58c
--- /dev/null
+++ b/tests/unmapdestroytest.qml
@@ -0,0 +1,86 @@
+/*
+ * Copyright 2014 Martin Gräßlin <mgraesslin@kde.org>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+*/
+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 8fe1389..305f667 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 db5ab7c..5f32007 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 7dbecc8..6e63e69 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 8840591..80f9634 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();
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic