<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>
</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;">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>
<br />
<p>- Stefanos</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>