Review Request 115459: Introduce runtime platform support in KWindowSystem

[Aurélien Gâteau]

> On Feb. 4, 2014, 5:19 p.m., Aurélien Gâteau wrote:
> > Any particular reason for not using an abstract class? The D macro makes the code a bit surprising to read.
> 
> Martin Gräßlin wrote:
>     see discussion of https://git.reviewboard.kde.org/r/115225/ (comment thread started by Aaron) - I wanted to keep the same pattern.
> 
> Aurélien Gâteau wrote:
>     Sounds like premature optimization to me, but then, you are the maintainer, so it's your call. Naming the macro DELEGATE like in the other patch would make it easier to read, and a good idea if you want to follow the same pattern.
> 
> Martin Gräßlin wrote:
>     Yes it kind of sounds like premature optimization, but I like another aspect of it: each backend needs to provide an implementation for all methods and cannot rely on the dummy implementation. Thus the maintainer of the backend has to think about the proper default value. Also a change like introducing a new method will compile fail on other platforms.
>     
>     Renaming the macro is a good suggestion, though.

You would get the compilation failures with a pure abstract class as well.


- Aurélien


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115459/#review48944
-----------------------------------------------------------


On Feb. 5, 2014, 5:55 p.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115459/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2014, 5:55 p.m.)
> 
> 
> Review request for KDE Frameworks, kdewin and Alexander Richardson.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> -------
> 
> Introduce runtime platform support in KWindowSystem
> 
> This is a change similar to the one in KWindowInfo, but with variation
> to the pattern due to the static container.
> 
> There is now a generic implementation of KWindowSystem which is
> completely windowing system platform independent. This implementation
> delegates all methods into a KWindowSystemPrivate class.
> 
> Each windowing system platform implementation needs to provide a
> subclass (e.g. KWindowSystemPrivateX11) and provide all the methods
> which are delegated. Note that there are no virtual methods defined,
> instead the d-pointer gets casted into the proper type. Thus if a
> method is not provided it will end in a compile error.
> 
> To make use of a platform implementation it needs to be included in
> the ctor of KWindowSystemStaticContainer and the PlatformImplementation
> enum needs to be extended by a value for the platform. This is used in
> the D macro to cast and delegate correctly.
> 
> There is a dummy implementation for all not supported windowing system
> platforms.
> 
> This change also includes some API changes:
> * KWindowSystem::windows() returns a copy instead of const-ref
> * All methods are provided, there is no longer X11 specific methods
> * private methods and enums are removed
> 
> NOTE: This change breaks the implementation for Windows and Mac OS!
> They are currently excluded from build.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 23133d581944a8373b9b753b300d97054b7d6f18 
>   src/kwindowinfo.cpp c706b29b306b65c992a178d490819b76e1aeca84 
>   src/kwindowsystem.h d288e1ab7c49f68482c94285c2aab695f08f3524 
>   src/kwindowsystem.cpp PRE-CREATION 
>   src/kwindowsystem_p.h PRE-CREATION 
>   src/kwindowsystem_p_x11.h PRE-CREATION 
>   src/kwindowsystem_x11.cpp 8d841cbcec41dc2d4df381338803902badf3f35e 
> 
> Diff: https://git.reviewboard.kde.org/r/115459/diff/
> 
> 
> Testing
> -------
> 
> Unit tests still succeed for X11, but they are not complete, though the most important aspects are tested.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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