<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
    <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="http://git.reviewboard.kde.org/r/112896/">http://git.reviewboard.kde.org/r/112896/</a>
     </td>
    </tr>
   </table>
   <br />





<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
 <p style="margin-top: 0;">On September 26th, 2013, 4:27 a.m. CEST, <b>Fredrik Höglund</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm just going to point out something I know you already know since we've discussed it many times;
that an xcb port of the NETWM classes already exists in a branch.</pre>
 </blockquote>







</blockquote>

<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">my aim was to write the unit test and that was the big junk of this work compared to the xcb changes (which I thought to be easier than trying to rebase the branch :-( )</pre>
<br />










<p>- Martin</p>


<br />
<p>On September 23rd, 2013, 3:11 p.m. CEST, Martin Gräßlin wrote:</p>








<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
 <tr>
  <td>

<div>Review request for KDE Frameworks.</div>
<div>By Martin Gräßlin.</div>


<p style="color: grey;"><i>Updated Sept. 23, 2013, 3:11 p.m.</i></p>






<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
 <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
 <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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.</pre>
  </td>
 </tr>
</table>


<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
 <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">see unit tests, for the window manager dependent code areas I want to write some test application and include it in /test</pre>
  </td>
 </tr>
</table>




<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>tier1/kwindowsystem/autotests/CMakeLists.txt <span style="color: grey">(a1a7066)</span></li>

 <li>tier1/kwindowsystem/autotests/netrootinfotestwm.cpp <span style="color: grey">(PRE-CREATION)</span></li>

 <li>tier1/kwindowsystem/autotests/netwininfotestclient.cpp <span style="color: grey">(PRE-CREATION)</span></li>

 <li>tier1/kwindowsystem/autotests/netwininfotestwm.cpp <span style="color: grey">(PRE-CREATION)</span></li>

 <li>tier1/kwindowsystem/src/CMakeLists.txt <span style="color: grey">(31fb53e)</span></li>

 <li>tier1/kwindowsystem/src/kstartupinfo.cpp <span style="color: grey">(971a23f)</span></li>

 <li>tier1/kwindowsystem/src/kwindowinfo_x11.cpp <span style="color: grey">(f382e9c)</span></li>

 <li>tier1/kwindowsystem/src/kwindowsystem_x11.cpp <span style="color: grey">(9a1f05b)</span></li>

 <li>tier1/kwindowsystem/src/kxutils_p.h <span style="color: grey">(84d639b)</span></li>

 <li>tier1/kwindowsystem/src/netwm.h <span style="color: grey">(71877c0)</span></li>

 <li>tier1/kwindowsystem/src/netwm.cpp <span style="color: grey">(c4f9989)</span></li>

 <li>tier1/kwindowsystem/src/netwm_p.h <span style="color: grey">(d7f4599)</span></li>

</ul>

<p><a href="http://git.reviewboard.kde.org/r/112896/diff/" style="margin-left: 3em;">View Diff</a></p>







  </td>
 </tr>
</table>








  </div>
 </body>
</html>