<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://git.reviewboard.kde.org/r/110824/">http://git.reviewboard.kde.org/r/110824/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 4th, 2013, 3:22 p.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think the fix is applied at the wrong place. The actual problem is that window not on the current activity are included in the layouting and so on which results in the empty spots where a window is. So the fix should be to not include such windows in the first place, then it doesn't matter where the user clicks.</pre>
</blockquote>
<p>On June 4th, 2013, 4:16 p.m. UTC, <b>Stefanos Harhalakis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This would mean changing one of the following:
* Workspace::xStackingOrder()
* EffectsHandlerImpl::stackingOrder()
I assume you don't mean Workspace::xStackingOrder() so that would be EffectsHandlerImpl::stackingOrder(). Right?
In any case, if isOnCurrentActivity() is expensive as Thomas said, would it be ok to proactively run this against all available windows instead of doing it at the very last moment?
Changing EffectsHandlerImpl for example, would mean this:
foreach (Toplevel *w, list)
if (!w->isOnCurrentActivity())
ret.append(effectWindow(w));
</pre>
</blockquote>
<p>On June 4th, 2013, 4:17 p.m. UTC, <b>Stefanos Harhalakis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Correction:
foreach (Toplevel *w, list)
if (w->isOnCurrentActivity())
ret.append(effectWindow(w));</pre>
</blockquote>
<p>On June 4th, 2013, 4:21 p.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">no, way to complicated. Have a look at DesktopGridEffect::setup() around line 1100</pre>
</blockquote>
<p>On June 4th, 2013, 5:29 p.m. UTC, <b>Stefanos Harhalakis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Sorry, I don't get it. Apart from not completely understanding that piece of code, it seems to be relevant only when presentwindows is used, which is not the case that this patch tries to solve.
The only interesting part I could find is this one:
if (w->isOnDesktop(i) && w->screen() == j &&isRelevantWithPresentWindows(w)) {
manager.manage(w);
}
but that's wrapped in an "if (isUsingPresentWindows())"
and even in that case, the windowAt() function uses the following to get the list of available windows and AFAICT the effect does not control the result of the stackingOrder() function:
EffectWindowList windows = effects->stackingOrder();
Any hints?
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The PW supplied case was *mostly* fixed "btw" with commit 3e5fdde239056760b2c0b46bf176a8ea6317697a
Your change is nevertheless required in :890 AND :895
Btw. this
while (begin < end)
qSwap(*begin++, *end--);
is silly - the iterators can just be traversed reverse. (unrelated, just mentioning because i just looked at it)
Also it's dumb to get the stacking order while we likely don't need it...</pre>
<br />
<p>- Thomas</p>
<br />
<p>On June 4th, 2013, 3:02 p.m. UTC, Stefanos Harhalakis wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kde-workspace.</div>
<div>By Stefanos Harhalakis.</div>
<p style="color: grey;"><i>Updated June 4, 2013, 3:02 p.m.</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; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Fix windowAt function to only return windows from current activity.
</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=301447">301447</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kwin/effects/desktopgrid/desktopgrid.cpp <span style="color: grey">(dc3d82b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110824/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>