Review Request 110824: Make desktop grid only consider windows from current activity

[Thomas Lübking]

> On June 4, 2013, 3:22 p.m., Martin Gräßlin wrote:
> > 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.
> 
> Stefanos Harhalakis wrote:
>     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));
>
> 
> Stefanos Harhalakis wrote:
>     Correction:
>     
>     foreach (Toplevel *w, list)
>       if (w->isOnCurrentActivity())
>         ret.append(effectWindow(w));
> 
> Martin Gräßlin wrote:
>     no, way to complicated. Have a look at DesktopGridEffect::setup() around line 1100
> 
> Stefanos Harhalakis wrote:
>     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?
>

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...


- Thomas


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


On June 4, 2013, 3:02 p.m., Stefanos Harhalakis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110824/
> -----------------------------------------------------------
> 
> (Updated June 4, 2013, 3:02 p.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Description
> -------
> 
> Fix windowAt function to only return windows from current activity.
> 
> 
> This addresses bug 301447.
>     http://bugs.kde.org/show_bug.cgi?id=301447
> 
> 
> Diffs
> -----
> 
>   kwin/effects/desktopgrid/desktopgrid.cpp dc3d82b 
> 
> Diff: http://git.reviewboard.kde.org/r/110824/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Stefanos Harhalakis
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130604/ec32b2b1/attachment.htm>