From 2d4b67d3614b9b888693d10d6af980505d0b4170 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Mon, 14 Jan 2013 09:20:59 +0100 Subject: [PATCH] Do not use a shared pointer for the Xcb::Wrapper It's not really needed, the required functionality can be achieved in a more implicit way. The reply pointer is managed by the Wrapper class as long as the method take() is not invoked. This method follows the semantics of QScopedPointer::take(). That is the pointer is set to null and the responsibility to free the pointer is passed to the callee. By this change we do not have the overhead of creating a QSharedPointer. In addition the Wrapper provides a copy ctor and assignment operator also using the semantics of take(). --- effects.cpp | 12 +- tests/CMakeLists.txt | 16 +++ tests/test_client_machine.cpp | 20 +--- tests/test_xcb_wrapper.cpp | 214 ++++++++++++++++++++++++++++++++++ tests/testutils.h | 61 ++++++++++ xcbutils.h | 87 ++++++++++++-- 6 files changed, 376 insertions(+), 34 deletions(-) create mode 100644 tests/test_xcb_wrapper.cpp create mode 100644 tests/testutils.h diff --git a/effects.cpp b/effects.cpp index 56fddcd96a..6845b748e4 100644 --- a/effects.cpp +++ b/effects.cpp @@ -1145,12 +1145,14 @@ void EffectsHandlerImpl::checkInputWindowStacking() if (input_windows.count() == 0) return; xcb_window_t* wins = new xcb_window_t[input_windows.count()]; - QList attributes; - foreach (const InputWindowPair &it, input_windows) { - attributes << Xcb::WindowAttributes(it.second); - } + QVector attributes(input_windows.count()); int pos = 0; - for (QList::iterator it = attributes.begin(); it != attributes.end(); ++it) { + foreach (const InputWindowPair &it, input_windows) { + attributes[pos] = Xcb::WindowAttributes(it.second); + pos++; + } + pos = 0; + for (QVector::iterator it = attributes.begin(); it != attributes.end(); ++it) { if (*it && (*it)->map_state != XCB_MAP_STATE_UNMAPPED) { wins[pos++] = (*it).window(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 739483d112..2785413995 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -50,3 +50,19 @@ target_link_libraries( testClientMachine ${XCB_XCB_LIBRARIES} ${X11_XCB_LIBRARIES} ) + +######################################################## +# Test XcbWrapper +######################################################## +set( testXcbWrapper_SRCS + test_xcb_wrapper.cpp +) +kde4_add_unit_test( testXcbWrapper TESTNAME kwin-TestXcbWrapper ${testXcbWrapper_SRCS} ) + +target_link_libraries( testXcbWrapper + ${QT_QTTEST_LIBRARY} + ${QT_QTCORE_LIBRARY} + ${QT_QTGUI_LIBRARY} + ${XCB_XCB_LIBRARIES} + ${X11_XCB_LIBRARIES} +) diff --git a/tests/test_client_machine.cpp b/tests/test_client_machine.cpp index 67cad0a797..1129c91b39 100644 --- a/tests/test_client_machine.cpp +++ b/tests/test_client_machine.cpp @@ -17,6 +17,7 @@ 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 . *********************************************************************/ +#include "testutils.h" // KWin #include "../client_machine.h" #include "../utils.h" @@ -66,24 +67,12 @@ private slots: void emptyHostName(); private: - xcb_window_t createWindow(); void setClientMachineProperty(xcb_window_t window, const QByteArray &hostname); xcb_window_t m_testWindow; QByteArray m_hostName; QByteArray m_fqdn; }; -xcb_window_t TestClientMachine::createWindow() -{ - xcb_window_t w = xcb_generate_id(connection()); - const uint32_t values[] = { true }; - xcb_create_window(connection(), 0, w, rootWindow(), - 0, 0, 10, 10, - 0, XCB_WINDOW_CLASS_INPUT_ONLY, XCB_COPY_FROM_PARENT, - XCB_CW_OVERRIDE_REDIRECT, values); - return w; -} - void TestClientMachine::setClientMachineProperty(xcb_window_t window, const QByteArray &hostname) { xcb_change_property(connection(), XCB_PROP_MODE_REPLACE, window, @@ -188,10 +177,5 @@ void TestClientMachine::emptyHostName() QCOMPARE(spy.isEmpty(), false); } -// need to implement main manually as we need a QApplication for X11 interaction -int main(int argc, char *argv[]) { - QApplication app(argc, argv); - TestClientMachine tc; - return QTest::qExec(&tc, argc, argv); -} +KWIN_TEST_MAIN(TestClientMachine) #include "test_client_machine.moc" diff --git a/tests/test_xcb_wrapper.cpp b/tests/test_xcb_wrapper.cpp new file mode 100644 index 0000000000..7369cc82ae --- /dev/null +++ b/tests/test_xcb_wrapper.cpp @@ -0,0 +1,214 @@ +/******************************************************************** +KWin - the KDE window manager +This file is part of the KDE project. + +Copyright (C) 2013 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) any later version. + +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 . +*********************************************************************/ +#include "testutils.h" +// KWin +#include "../xcbutils.h" +// Qt +#include +#include +// xcb +#include + +using namespace KWin; +using namespace KWin::Xcb; + +class TestXcbWrapper : public QObject +{ + Q_OBJECT +private slots: + void init(); + void cleanup(); + void defaultCtor(); + void normalCtor(); + void copyCtorEmpty(); + void copyCtorBeforeRetrieve(); + void copyCtorAfterRetrieve(); + void assignementEmpty(); + void assignmentBeforeRetrieve(); + void assignmentAfterRetrieve(); + void discard(); +private: + void testEmpty(WindowGeometry &geometry); + void testGeometry(WindowGeometry &geometry, const QRect &rect); + xcb_window_t m_testWindow; +}; + +void TestXcbWrapper::init() +{ + m_testWindow = XCB_WINDOW_NONE; +} + +void TestXcbWrapper::cleanup() +{ + if (m_testWindow != XCB_WINDOW_NONE) { + xcb_destroy_window(connection(), m_testWindow); + } +} + +void TestXcbWrapper::testEmpty(WindowGeometry &geometry) +{ + QCOMPARE(geometry.window(), noneWindow()); + QVERIFY(!geometry.data()); + QCOMPARE(geometry.isNull(), true); + QCOMPARE(geometry.rect(), QRect()); + QVERIFY(!geometry); +} + +void TestXcbWrapper::testGeometry(WindowGeometry &geometry, const QRect &rect) +{ + QCOMPARE(geometry.window(), m_testWindow); + // now lets retrieve some data + QCOMPARE(geometry.rect(), rect); + QVERIFY(geometry.isRetrieved()); + QCOMPARE(geometry.isNull(), false); + QVERIFY(geometry); + QVERIFY(geometry.data()); + QCOMPARE(geometry.data()->x, int16_t(rect.x())); + QCOMPARE(geometry.data()->y, int16_t(rect.y())); + QCOMPARE(geometry.data()->width, uint16_t(rect.width())); + QCOMPARE(geometry.data()->height, uint16_t(rect.height())); +} + +void TestXcbWrapper::defaultCtor() +{ + WindowGeometry geometry; + testEmpty(geometry); + QVERIFY(!geometry.isRetrieved()); +} + +void TestXcbWrapper::normalCtor() +{ + m_testWindow = createWindow(); + QVERIFY(m_testWindow != noneWindow()); + + WindowGeometry geometry(m_testWindow); + QVERIFY(!geometry.isRetrieved()); + testGeometry(geometry, QRect(0, 0, 10, 10)); +} + +void TestXcbWrapper::copyCtorEmpty() +{ + WindowGeometry geometry; + WindowGeometry other(geometry); + testEmpty(geometry); + QVERIFY(geometry.isRetrieved()); + testEmpty(other); + QVERIFY(!other.isRetrieved()); +} + +void TestXcbWrapper::copyCtorBeforeRetrieve() +{ + m_testWindow = createWindow(); + QVERIFY(m_testWindow != noneWindow()); + WindowGeometry geometry(m_testWindow); + QVERIFY(!geometry.isRetrieved()); + WindowGeometry other(geometry); + testEmpty(geometry); + QVERIFY(geometry.isRetrieved()); + + QVERIFY(!other.isRetrieved()); + testGeometry(other, QRect(0, 0, 10, 10)); +} + +void TestXcbWrapper::copyCtorAfterRetrieve() +{ + m_testWindow = createWindow(); + QVERIFY(m_testWindow != noneWindow()); + WindowGeometry geometry(m_testWindow); + QVERIFY(geometry); + QVERIFY(geometry.isRetrieved()); + QCOMPARE(geometry.rect(), QRect(0, 0, 10, 10)); + WindowGeometry other(geometry); + testEmpty(geometry); + QVERIFY(geometry.isRetrieved()); + + QVERIFY(other.isRetrieved()); + testGeometry(other, QRect(0, 0, 10, 10)); +} + +void TestXcbWrapper::assignementEmpty() +{ + WindowGeometry geometry; + WindowGeometry other; + testEmpty(geometry); + testEmpty(other); + + other = geometry; + QVERIFY(geometry.isRetrieved()); + testEmpty(geometry); + testEmpty(other); + QVERIFY(!other.isRetrieved()); +} + +void TestXcbWrapper::assignmentBeforeRetrieve() +{ + m_testWindow = createWindow(); + QVERIFY(m_testWindow != noneWindow()); + WindowGeometry geometry(m_testWindow); + WindowGeometry other = geometry; + QVERIFY(geometry.isRetrieved()); + testEmpty(geometry); + + QVERIFY(!other.isRetrieved()); + testGeometry(other, QRect(0, 0, 10, 10)); + + other = WindowGeometry(m_testWindow); + QVERIFY(!other.isRetrieved()); + QCOMPARE(other.window(), m_testWindow); + other = WindowGeometry(); + testEmpty(geometry); +} + +void TestXcbWrapper::assignmentAfterRetrieve() +{ + m_testWindow = createWindow(); + QVERIFY(m_testWindow != noneWindow()); + WindowGeometry geometry(m_testWindow); + QVERIFY(geometry); + QVERIFY(geometry.isRetrieved()); + WindowGeometry other = geometry; + testEmpty(geometry); + + QVERIFY(other.isRetrieved()); + testGeometry(other, QRect(0, 0, 10, 10)); + + other = WindowGeometry(); + testEmpty(other); +} + +void TestXcbWrapper::discard() +{ + // discard of reply cannot be tested properly as we cannot check whether the reply has been discarded + // therefore it's more or less just a test to ensure that it doesn't crash and the code paths + // are taken. + WindowGeometry *geometry = new WindowGeometry(); + delete geometry; + + m_testWindow = createWindow(); + geometry = new WindowGeometry(m_testWindow); + delete geometry; + + geometry = new WindowGeometry(m_testWindow); + QVERIFY(geometry->data()); + delete geometry; +} + +KWIN_TEST_MAIN(TestXcbWrapper) +#include "test_xcb_wrapper.moc" diff --git a/tests/testutils.h b/tests/testutils.h new file mode 100644 index 0000000000..d0154104d9 --- /dev/null +++ b/tests/testutils.h @@ -0,0 +1,61 @@ +/******************************************************************** +KWin - the KDE window manager +This file is part of the KDE project. + +Copyright (C) 2013 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) any later version. + +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 . +*********************************************************************/ +#ifndef TESTUTILS_H +#define TESTUTILS_H +// KWin +#include +// XCB +#include + +// like QTEST_MAIN, just using a QApplication for X11 interaction +#define KWIN_TEST_MAIN(TestObject) \ +int main(int argc, char *argv[]) { \ + QApplication app(argc, argv); \ + TestObject tc; \ + return QTest::qExec(&tc, argc, argv); \ +} + +namespace KWin { + +/** + * Wrapper to create an 0,0x10,10 input only window for testing purposes + **/ +static xcb_window_t createWindow() +{ + xcb_window_t w = xcb_generate_id(connection()); + const uint32_t values[] = { true }; + xcb_create_window(connection(), 0, w, rootWindow(), + 0, 0, 10, 10, + 0, XCB_WINDOW_CLASS_INPUT_ONLY, XCB_COPY_FROM_PARENT, + XCB_CW_OVERRIDE_REDIRECT, values); + return w; +} + +/** + * casts XCB_WINDOW_NONE to uint32_t. Needed to make QCOMPARE working. + **/ +static uint32_t noneWindow() +{ + return XCB_WINDOW_NONE; +} + +} // namespace + +#endif diff --git a/xcbutils.h b/xcbutils.h index ac034e19b1..5e7417825a 100644 --- a/xcbutils.h +++ b/xcbutils.h @@ -40,48 +40,112 @@ template (other)); + } virtual ~Wrapper() { - if (!m_retrieved) { - xcb_discard_reply(connection(), m_cookie.sequence); + cleanup(); + } + inline Wrapper &operator=(const Wrapper &other) { + if (this != &other) { + // if we had managed a reply, free it + cleanup(); + // copy members + m_retrieved = other.m_retrieved; + m_cookie = other.m_cookie; + m_window = other.m_window; + m_reply = other.m_reply; + // take over the responsibility for the reply pointer + takeFromOther(const_cast(other)); } + return *this; } inline const Reply *operator->() { getReply(); - return m_reply.data(); + return m_reply; + } + inline bool isNull() { + getReply(); + return m_reply == NULL; } inline operator bool() { - getReply(); - return !m_reply.isNull(); + return !isNull(); } - inline const QSharedPointer &data() { + inline const Reply *data() { getReply(); return m_reply; } inline WindowId window() const { return m_window; } + inline bool isRetrieved() const { + return m_retrieved; + } + /** + * Returns the value of the reply pointer referenced by this object. The reply pointer of + * this object will be reset to null. Calling any method which requires the reply to be valid + * will crash. + * + * Callers of this function take ownership of the pointer. + **/ + inline Reply *take() { + getReply(); + Reply *ret = m_reply; + m_reply = NULL; + m_window = XCB_WINDOW_NONE; + return ret; + } protected: void getReply() { - if (m_retrieved) { + if (m_retrieved || !m_cookie.sequence) { return; } - m_reply = QSharedPointer(replyFunc(connection(), m_cookie, NULL), &free); + m_reply = replyFunc(connection(), m_cookie, NULL); m_retrieved = true; } private: + inline void cleanup() { + if (!m_retrieved && m_cookie.sequence) { + xcb_discard_reply(connection(), m_cookie.sequence); + } else if (m_reply) { + free(m_reply); + } + } + inline void takeFromOther(Wrapper &other) { + if (m_retrieved) { + m_reply = other.take(); + } else { + //ensure that other object doesn't try to get the reply or discards it in the dtor + other.m_retrieved = true; + other.m_window = XCB_WINDOW_NONE; + } + } bool m_retrieved; Cookie m_cookie; WindowId m_window; - QSharedPointer m_reply; + Reply *m_reply; }; typedef Wrapper WindowAttributes; @@ -90,11 +154,12 @@ typedef Wrapper { public: + WindowGeometry() : Wrapper() {} explicit WindowGeometry(xcb_window_t window) : Wrapper(window) {} inline QRect rect() { - const QSharedPointer &geometry = data(); - if (geometry.isNull()) { + const xcb_get_geometry_reply_t *geometry = data(); + if (!geometry) { return QRect(); } return QRect(geometry->x, geometry->y, geometry->width, geometry->height);