Review Request 112896: Rework NETWM classes

Kevin Ottens ervin at kde.org
Mon Nov 4 15:40:39 UTC 2013



> On Sept. 26, 2013, 2:27 a.m., Fredrik Höglund wrote:
> > 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.
> 
> Martin Gräßlin wrote:
>     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 :-( )
> 
> Fredrik Höglund wrote:
>     The problem with rebasing the branch was solved a long time ago.
>     There shouldn't be any conflicts except maybe in CMakeLists.txt files.
>
> 
> Martin Gräßlin wrote:
>     ah, I didn't know that. I will have a look and see whether I can integrate the unit tests and the bug fixes on top of your branch.
> 
> Kevin Ottens wrote:
>     Any news about that patch?
> 
> Kevin Ottens wrote:
>     Should I close this one too next monday?

Last warning: I'll close it next time I do my review round and there's no activity. ;-)


- Kevin


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


On Sept. 23, 2013, 1:11 p.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112896/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2013, 1:11 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> 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/20131104/0c4c09d6/attachment-0001.html>


More information about the Kde-frameworks-devel mailing list