<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://reviewboard.kde.org/r/1381/">http://reviewboard.kde.org/r/1381/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 21st, 2010, 4:18 p.m., <b>Lubos Lunak</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>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.</pre>
</blockquote>
<p>On May 22nd, 2010, 5:40 a.m., <b>Michael Leupold</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Thanks for taking a close look!
I agree on QAbstractEventDispatcher::EventFilter. I think when I first wrote this patch I just figured out message's type based on the Qt sources. I filed a request on Qt's bugtracker to see if they could give a guarantee about the type. If not I probably should just trash this patch as-is.</pre>
</blockquote>
</blockquote>
<pre>Good news. EventFilter is now fully documented along with a line saying "You can do a static cast to these types." (http://bugreports.qt.nokia.com/browse/QTBUG-10904, commit 594900e68f8e264facbe8c75eaf2b857240bc072) and afaict the fix will be in 4.7.
Thus I'll make the required modifications to get this patch ready for another review.</pre>
<br />
<p>- Michael</p>
<br />
<p>On May 21st, 2010, 5:09 a.m., Michael Leupold wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Michael Leupold.</div>
<p style="color: grey;"><i>Updated 2010-05-21 05:09:24</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;">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).</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;">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.</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>/trunk/KDE/kdelibs/kdeui/kernel/kwindowsystemeventfilter.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdeui/kernel/kwindowsystemeventfilter.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdeui/kernel/kapplication.cpp <span style="color: grey">(1129021)</span></li>
<li>/trunk/KDE/kdelibs/kdeui/kernel/kapplication.h <span style="color: grey">(1129021)</span></li>
<li>/trunk/KDE/kdelibs/kdeui/CMakeLists.txt <span style="color: grey">(1129021)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/1381/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>