Review Request: KWindowSystemEventFilter: methods for installing global event filters

[Lubos Lunak]

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


I think the patch is generally ok, but, as far as I can say, Qt's "bool myEventFilter(void *message);" is brain-damaged undocumented API, and the code in KWindowSystemEventFilterPrivate::filterEvent() seems somewhat confused as a result. Is it actually documented somewhere that the void* the filter receives is in fact XEvent* on X11, and is it guaranteed to always be XEvent*? I cannot find anything. If it is guaranteed, you can remove all the "// only process XEvents.", "// check if void *message is a NSEvent*" etc. parts, since they are not necessary, if it's always the expected code. If it is not guaranteed, this whole patch cannot work, since those "checks" if the void* is something are broken and do not work.

I'm also not overly happy with the name KWindowSystemEventFilter, as it suggests it has something to do with the KWindowSystem class, I'd suggest dropping the 'Window' from the name.

- Lubos


On 2010-05-21 05:09:24, Michael Leupold wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1381/
> -----------------------------------------------------------
> 
> (Updated 2010-05-21 05:09:24)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch moves the installX11EventFilter functionality from inside KApplication to an external KWindowSystemEventFilter namespace. Like this, non-KApplications can use global event filters (and KApplication gets smaller).
> 
> The new methods use QAbstractEventDispatcher and are named KWindowSystemEventFilter::installEventFilter and removeEventFilter. They are not meant to be specific to X11 any longer but the current implementation is only concerned with X11 as I don't have another window system to test it with (I provided stubs that could serve as base for an implementation).
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdeui/kernel/kwindowsystemeventfilter.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/kernel/kwindowsystemeventfilter.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/kernel/kapplication.cpp 1129021 
>   /trunk/KDE/kdelibs/kdeui/kernel/kapplication.h 1129021 
>   /trunk/KDE/kdelibs/kdeui/CMakeLists.txt 1129021 
> 
> Diff: http://reviewboard.kde.org/r/1381/diff
> 
> 
> Testing
> -------
> 
> Too little I fear. I tested it with KColorDialog which uses KApplication::installX11EventFilter for its color picker and it worked fine.
> 
> There should be a unit-test for it but I haven't thought of any.
> 
> 
> Thanks,
> 
> Michael
> 
>