Bug 222921 - Estimation of next screen when using keyboard shortcuts for quick tiling is wrong
Summary: Estimation of next screen when using keyboard shortcuts for quick tiling is w...
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR wishlist (vote)
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-16 02:04 UTC by Marcel Schaal
Modified: 2014-03-29 23:01 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
adds neighborhood semantics to kephal and uses this in kwin (6.64 KB, patch)
2010-01-16 02:12 UTC, Marcel Schaal
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel Schaal 2010-01-16 02:04:11 UTC
Version:            (using Devel)
OS:                Linux
Installed from:    Compiled sources

when quick tiling a window across two screens with more than three screens in use, kwin may select the wrong next screen. i tested this successfully on the following  screen setup (xorg.conf):
    Screen      0  "ScreenT" 0 0
    Screen      1  "ScreenL" Below "ScreenT"
    Screen      2  "ScreenR" RightOf "ScreenL"

screen overview:
---
|T|
|L|R|
-----

symptom:
when a window is quick tiled on screen R to the left it appears on screen T but should appear on L
Comment 1 Marcel Schaal 2010-01-16 02:12:20 UTC
Created attachment 39930 [details]
adds neighborhood semantics to kephal and uses this in kwin

1. adds above/left/... semantics to screens in kephal
2. replace buggy code in kwin

probably the neighborhood semantics is too strong. but i think user don't leave any gaps between their monitors. i've tested it with horizontal and vertical stacked and an L-shaped  setup. 

i'm not sure whether u can commit to kephal, but from a software engineer's point of view it's the right place
Comment 2 Martin Flöser 2011-01-22 13:58:58 UTC
Could you please open a review request against Kephal on svn.reviewboard.kde.org or git.reviewboard.kde.org (after 29th of January). It's quite a pity that nobody responded to the patch and it was kind of lost in this report :-(

Concerning KWin: the quick tile code should only respect the current screen and not other screens.
Comment 3 Martin Flöser 2012-04-08 18:58:34 UTC
Current version of KWin does no longer use Kephal, which means the patch does no longer apply.

Changing to feature request as it needs extension of functionality.
Comment 4 Thomas Lübking 2013-12-23 12:26:16 UTC
quick note: the attached patch is "wrong" anyway since it assumes the screens would be strictly adjacent.

I *may* have fixed this in a local patch - if you're still interested, please post the output of the problematic "xrandr -q" (rather than a nice but imprecise ascii art ;-)
Comment 5 Thomas Lübking 2014-01-14 21:51:15 UTC
Git commit d46ca4837da88f6baee16cd38b6be8f7802fac6f by Thomas Lübking.
Committed on 23/12/2013 at 01:28.
Pushed by luebking into branch 'KDE/4.11'.

fix shortcut driven quicktiling

1. swapping direction would rather toggle tiling
2. the next screen was calculated wrongly (found outmost)
3. the electrictborder geometry was not updated when swapping the mode on screen changes
Related: bug 329136
FIXED-IN: 4.11.6
REVIEW: 114648

M  +46   -44   kwin/geometry.cpp

http://commits.kde.org/kde-workspace/d46ca4837da88f6baee16cd38b6be8f7802fac6f
Comment 6 Thomas Lübking 2014-01-14 22:20:56 UTC
The last commit will show up in kde-workspace 4.11.6 (released in about two weeks) - it would be very kind if you could check whether that fixes your issue as well.
Comment 7 Marcel Schaal 2014-01-15 04:11:34 UTC
Thomas, thanks for your effort. Am currently not using KDE and therefore I have no development environment either. I'll try to get KDE up and running when the updated packages hit in Fedora.

I know this is not a review board, but i have one annotation and one question:
Line 3219 (and same for line 3221):
if (x >= screens[curScreen].center().x() || (curScreen != nextScreen && x <= screens[nextScreen].center().x()))
If the screens are not the same, then x-coordinates won't be the same, so the second condition can be shortened to:
if (x >= screens[curScreen].center().x() || ( x < screens[nextScreen].center().x()))

Line 3214:
Assuming you have a (highly unlikely) setup of four screens, where one screen has e.g. 4K resolution and three smaller monitors (A,B,C) are stacked to one side, like: 
|A||   |
|B||4k|
|C||   |
So the 4k screen's y-resolution so big, that it will span across A,B and C. Will the tilled window reliably end up on monitor B or does it depend on the placement in the screens vector? The same issue may apply to only two smaller screens next to 4k, but then the placement is non-trivial anyway.

Otherwise the patch looks promising. Keep up the awesome work!
Comment 8 Thomas Lübking 2014-01-15 15:31:53 UTC
(In reply to comment #7)
> If the screens are not the same, then x-coordinates won't be the same, so

|--------------|
| Screen 1 |
|--------------|
|--------------|
| Screen 2 |
|--------------|

You were saying? ;-)
(Actually the entire center could be the same if the screens are clones)

> Line 3214:
> Assuming you have a (highly unlikely) setup of four screens, where one
> screen has e.g. 4K resolution and three smaller monitors (A,B,C) are stacked
> to one side, like: 

You mean moving the tile from the 4k screen to the left?
All three screens on the left are "valid" candidates - the first one tested (likely "A") will be chosen - the code just checks that the new screen is not really above or really below the old screen.

If you want to "post-review", see https://git.reviewboard.kde.org/r/114648/
Comment 9 Marcel Schaal 2014-01-16 15:07:21 UTC
> You were saying? ;-)
> (Actually the entire center could be the same if the screens are clones)
But this case is already handled before in line 3214, isn't it? If it's not in the same "stripe", the screen is not considered further.
Comment 10 Thomas Lübking 2014-01-16 19:10:03 UTC
(In reply to comment #9)
> > You were saying? ;-)
> > (Actually the entire center could be the same if the screens are clones)
> But this case is already handled before in line 3214, isn't it? If it's not
> in the same "stripe", the screen is not considered further.

Notice that the check is *not* for "are x coordinates not equal" but to move the window to the closest left or right screen (depending on the tiling mode), thus has to ensure it moves
a) into the right direction and
b) not further than a position we already may have found.
Comment 11 Marcel Schaal 2014-03-29 23:01:10 UTC
Hi, I've tried 4.12 and it was working as expected. Thanks!