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