Review Request 112896: Rework NETWM classes

Mon Sep 23 13:11:34 UTC 2013

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112896/
-----------------------------------------------------------

Review request for KDE Frameworks.

Description
-------

This is a patch series, if needed I can push the branch.

The patches address the following topics:
* add unit tests for NETRootInfo and NETWinInfo which do not require a running window manager. Test coverage of netwm.h is:
  ** line coverage: 75 %
  ** functions coverage: 84 %
  ** branch coverage: 62 %
  The tests start their own Xvfb to have a clean state and not mess up with the Window Manager of a user or cause followint tests to fail on the CI system
* areas which are covered by unit tests are changed from XLib to XCB. This mostly affects changing and reading window properties
* API is changed from XLib types to XCB types. E.g. xcb_window_t instead of Window. Note: this is an API break, which I consider as necessary, given that especially the type "Window" is problematic.
* several bugs which were discovered through the added tests are fixed
* NETWinInfo2 is merged into NETWinInfo (marked as TODO KDE5)
* Small wrapper class for intern atom added to KXUtils. Similar code already in usage in KWin.

=====
Commits:

commit 2e50845a5d0df436106aeb776e3936691c32a753
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 23 14:31:42 2013 +0200

    Use XCB for reading properties in NETRootInfo::update

    Viewport handling was incorrect and is adjusted to be read correctly.

commit 23887726c03c49b4e0021c01f319653d3b9f36c5
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 23 11:41:26 2013 +0200

    Use XCB for reading properties in NETWinInfo::update

    Those areas which are covered by unit tests are migrated from
    XGetWindowProperty to the xcb variant.

    BlocksCompositing had an incorrect read - it expected a string atom,
    but actually uses a cardinal. This bug is fixed.

commit 2731ebbc85eddb885700232f98e0e429cb0e066b
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 23 09:41:28 2013 +0200

    Use XCB to change the Client dependent properties of NETWinInfo

    Those properties for which we have unit tests are changed to XCB to
    either change or delete the property.

commit 1bb85e440ec0004ef6b18b6fa1855c08c8f6697a
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Fri Sep 20 09:58:09 2013 +0200

    Unit test for Client aspect of NETWinInfo

    Similar to the NETWinInfo test with WindowManager aspect, just
    verifying the properties which can only be set by a Client.

    The test found highlights a few possible problems:
     * for some window types a fallback type is specified, but the lenght
       is set to 1, thus the fallback type is not set at all
     * Fullscreen monitors property is not handled in the event function

commit 2731ebbc85eddb885700232f98e0e429cb0e066b
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 23 09:41:28 2013 +0200

    Use XCB to change the Client dependent properties of NETWinInfo

    Those properties for which we have unit tests are changed to XCB to
    either change or delete the property.

commit 1bb85e440ec0004ef6b18b6fa1855c08c8f6697a
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Fri Sep 20 09:58:09 2013 +0200

    Unit test for Client aspect of NETWinInfo

    Similar to the NETWinInfo test with WindowManager aspect, just
    verifying the properties which can only be set by a Client.

    The test found highlights a few possible problems:
     * for some window types a fallback type is specified, but the lenght
       is set to 1, thus the fallback type is not set at all
     * Fullscreen monitors property is not handled in the event function

commit 448f200ecdd642d1a4c85522eaa7d28072cda873
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Thu Sep 19 13:57:03 2013 +0200

    Use XCB to change the WM dependent properties of NETWinInfo

    Those properties for which we have unit tests are changed to XCB to
    either change or delete the property.

commit 736024c6c6233a9c66a03972b9dbfbd2e6d383f4
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Thu Sep 19 13:16:03 2013 +0200

    Add unit test for window manager aspect of NETWinInfo

    Similar to the test for NETRootInfo it tests setting the properties
    which can only be set by the window manager. Also running in its own
    Xvfb.

commit 480842406c2c070280a8be8ae3e4af93674369f3
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Thu Sep 19 07:20:57 2013 +0200

    Merge NETWinInfo2 into NETWinInfo

    Class only existed for BIC changes and had a TODO remove KDE5 marker.

commit 4e71654d95715f5b44efef80fb95fa9bee295b6c
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Thu Sep 19 07:16:00 2013 +0200

    Drop desktop argument from NETRootInfo::(set)DesktopGeometry

    The desktop argument was only there because of a different semantic in
    an early draft of NETwm and completely unused. Doesn't make sense to
    carry it into the next version.

commit 78a818af6937a01d0464b27c8256b05e9889f692
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Wed Sep 18 14:35:52 2013 +0200

    Use xcb to change properties in NETRootInfo

    Change is done in all the methods which are covered by the unit test.

commit a34612f1841cf063a7e5b3e433b45e87cb16ef36
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Wed Sep 18 13:47:16 2013 +0200

    NETRootInfo opperates on WindowTypeMask not on WindowType

    ::isSupported and ::setSupported for WindowType operated on the wrong
    type. This needs to be the mask.

commit 43ebfba6ee0f309eddb9517af992e8490eef3e24
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Wed Sep 18 12:41:06 2013 +0200

    Add unit test for NETRootInfo with role WindowManager

    Verifies that the properties are set correctly. The unit test found
    problems in the following areas:
    * setSupported for window types is taking the types instead of the
      mask and by that completely messing up the supported windows.
      Test is set to expected fail for those elements

    The unit test starts an Xvfb in the initTestCase and closes it again
    when the tests end. The display number is chosen by random in [1,98].
    0 is skipped as that is the normal X server and 99 is skipped as that's
    what KDE's CI infrastructure already uses.

    An explicit Xvfb is started instead of using an already running
    X Server to not interfere with already running window managers or
    left-over states from other tests.

    It would be even better to have one Xvfb instance per test method,
    but this is not possible as the atoms are only resolved once and
    thus NETRootInfo is indirectly bound to a Display although the API
    kind of implies that it would be able to use different NETRootInfo
    for different Displays.

commit 99dbcd1d352c5d1fb69595899bb653ec195939fa
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 14:20:24 2013 +0200

    Use xcb datatypes for helper functions in netwm.cpp

    Some more Window -> xcb_window_t

commit 8a8e0edcb4c697ae8539e8a91885a0e422f30b3a
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 14:16:37 2013 +0200

    Remove Xlib includes from netwm.h

    We don't need them in the header any more. Only need to forward
    declare the Display variable.

    Unfortunately we still need the includes in other parts of
    kwindowsystems where Xlib is still used and netwm.h was included.

commit e4a78dbc3fda5c20a11b6ede708f5390ac399915
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 14:06:49 2013 +0200

    Use bool/true/false instead of Bool/True/False in netwm

    For our internal API we can just use the proper datatypes instead of
    the XLib macros.

commit 2c86f1898b8c0898a6c9c73cf90dda87b1af5095
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 13:59:48 2013 +0200

    Use xcb_atom_t instead of Atom in NET(Root|Win)Info

    Not all are replaced as XGetWindowProperty starts to complain if
    we pass in a pointer to xcb_atom_t instead of pointer to Atom.

commit 79be0cbce1719c3fcbc0d3e89a8374192cfd6af7
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 13:50:52 2013 +0200

    Use xcb_timestamp_t instead of Time in NET(Root|Win)Info

    Same as with Window - using xcb type.

commit 28ae8e12894ab6cb948772bceee4ac4798417233
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 13:44:39 2013 +0200

    Use xcb_window_t instead of Window in NETWinInfo

    Just like for NETRootInfo.

commit c2a8a37926b16b39c0d468de0aec4cfaee0abb5d
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 13:25:09 2013 +0200

    Switch NETRootInfo from Window to xcb_window_t

    Window is the old XLib type whose name is rather conflicting as
    it's not in a namespace. So let's use the XCB type.

    For the properties which take a list of Windows, the list is
    still passed as Window and not as xcb_window_t. Reason for this
    is that apparently the wrong value gets written if it's the xcb
    datatype. This will be removed once it is using xcb to change
    the property.

commit 9ddf0634f4973a0ba4effeddca2b7edda1921e73
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 12:55:12 2013 +0200

    Use KXUtils::Atom wrapper for all the items in netwm

    By using the Atom wrapper the code is ported to XCB and the reply is
    only retrieved once it is needed.

commit c63db861ea4224967bb0330e92ef4f914339d7bd
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 12:47:43 2013 +0200

    Add xcb_connection_t* to NET(Root|Win)InfoPrivate

    For future use: have an xcb_connection_t* created from the Display*
    in the private classes. Once we get rid of XLib it should be passed
    in through the ctor instead of the Display pointer.

commit 8b73a064a5773c3cc8b6b2207b7c3f656dbf5afb
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 12:39:18 2013 +0200

    Add wrapper class for intern atom to KXUtils

    Small wrapper class to simplify request and reply for an intern
    atom.

commit a89950dd09a1aaf15ba196009e50aa137a490a8e
Author: Martin Gräßlin <mgraesslin at kde.org>
Date:   Mon Sep 16 11:24:12 2013 +0200

    Add dependency for X11_XCB to KWindowSystems framework

    X11_XCB allows us to transit to XCB gradually. That is without
    changing the external API.

Diffs
-----

  tier1/kwindowsystem/autotests/CMakeLists.txt a1a7066 
  tier1/kwindowsystem/autotests/netrootinfotestwm.cpp PRE-CREATION 
  tier1/kwindowsystem/autotests/netwininfotestclient.cpp PRE-CREATION 
  tier1/kwindowsystem/autotests/netwininfotestwm.cpp PRE-CREATION 
  tier1/kwindowsystem/src/CMakeLists.txt 31fb53e 
  tier1/kwindowsystem/src/kstartupinfo.cpp 971a23f 
  tier1/kwindowsystem/src/kwindowinfo_x11.cpp f382e9c 
  tier1/kwindowsystem/src/kwindowsystem_x11.cpp 9a1f05b 
  tier1/kwindowsystem/src/kxutils_p.h 84d639b 
  tier1/kwindowsystem/src/netwm.h 71877c0 
  tier1/kwindowsystem/src/netwm.cpp c4f9989 
  tier1/kwindowsystem/src/netwm_p.h d7f4599 

Diff: http://git.reviewboard.kde.org/r/112896/diff/

Testing
-------

see unit tests, for the window manager dependent code areas I want to write some test application and include it in /test

Thanks,

Martin Gräßlin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130923/1c5b4a03/attachment-0001.html>