Bug 363804 - Struts are broken with multiple screens
Summary: Struts are broken with multiple screens
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: platform-x11-nested (show other bugs)
Version: git master
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: KWin default assignee
URL: https://phabricator.kde.org/D1744
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-01 16:56 UTC by Aleix Pol
Modified: 2016-06-15 12:48 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:
mgraesslin: ReviewRequest+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Aleix Pol 2016-06-01 16:56:33 UTC
I have 2 screens, one panel at the bottom of one of the screens. Maximizing windows get under the panel.

This doesn't happen if I "openbox --replace"
This doesn't happen with just 1 screen.

Reproducible: Always

Steps to Reproduce:
1. Start plasma with one panel at the bottom of one of the screen
2.
3.
Comment 1 Thomas Lübking 2016-06-01 17:06:11 UTC
please run "xprop" on the supposingly strutting panel and "xrandr -q" and attach both outputs.
Comment 2 Aleix Pol 2016-06-01 23:17:56 UTC
Sure! :)

That's xprop: https://paste.kde.org/pk9kiemwu

$ xrandr -q                                                                                                                                                                                                                                                         
Screen 0: minimum 8 x 8, current 1920 x 1848, maximum 32767 x 32767                                                                                                                                                                                                              
LVDS1 connected primary 1366x768+554+1080 (normal left inverted right x axis y axis) 277mm x 156mm                                                                                                                                                                               
   1366x768      60.02*+                                                                                                                                                                                                                                                         
   1280x720      60.00                                                                                                                                                                                                                                                           
   1024x768      60.00                                                                                                                                                                                                                                                           
   1024x576      60.00                                                                                                                                                                                                                                                           
   960x540       60.00                                                                                                                                                                                                                                                           
   800x600       60.32    56.25                                                                                                                                                                                                                                                  
   864x486       60.00                                                                                                                                                                                                                                                           
   640x480       59.94                                                                                                                                                                                                                                                           
   720x405       60.00                                                                                                                                                                                                                                                           
   680x384       60.00                                                                                                                                                                                                                                                           
   640x360       60.00                                                                                                                                                                                                                                                           
DP1 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 510mm x 287mm                                                                                                                                                                                             
   1920x1080     60.00*+                                                                                                                                                                                                                                                         
   1680x1050     59.95                                                                                                                                                                                                                                                           
   1680x945      60.02                                                                                                                                                                                                                                                           
   1400x1050     74.87    59.98                                                                                                                                                                                                                                                  
   1600x900      60.00                                                                                                                                                                                                                                                           
   1280x1024     75.02    60.02  
   1440x900      74.98    59.89  
   1280x960      60.00  
   1366x768      59.79  
   1360x768      60.02  
   1280x800      74.93    59.81  
   1152x864      75.00  
   1280x768      74.89    59.87  
   1280x720      60.00  
   1024x768      75.08    70.07    60.00  
   1024x576      59.97  
   800x600       72.19    75.00    60.32    56.25  
   848x480       60.00  
   640x480       75.00    72.81    60.00  
   720x400       70.08  
DP2 disconnected (normal left inverted right x axis y axis)
DP3 disconnected (normal left inverted right x axis y axis)
HDMI1 disconnected (normal left inverted right x axis y axis)
HDMI2 disconnected (normal left inverted right x axis y axis)
HDMI3 disconnected (normal left inverted right x axis y axis)
VGA1 disconnected (normal left inverted right x axis y axis)
VIRTUAL1 disconnected (normal left inverted right x axis y axis)
Comment 3 Thomas Lübking 2016-06-01 23:39:24 UTC
Wild guess, what if you left-align the lower screen?

   xrandr --output LVDS1 --pos 0x1080
Comment 4 Aleix Pol 2016-06-02 00:29:39 UTC
That does fix the problem.
Note that this used to work up until few weeks ago...
Comment 5 Martin Flöser 2016-06-02 06:05:16 UTC
(In reply to Aleix Pol from comment #4)
> That does fix the problem.
> Note that this used to work up until few weeks ago...

we did some changes in bc25677caf745e9080771852b2dc3e01781cb79f on April 11. If the strut on the window is incorrect it gets ignored now.

I don't want to exclude the possibility that your setup gets now excluded. It's a more complex case due to the screens kind of overlapping.

Anyway, feel free to add your setup as a test case to the added auto test.
Comment 6 Thomas Lübking 2016-06-02 06:33:32 UTC
The commit is completely wrong since adjustedClientArea is called per screen area and not (only) in total, see hasOffscreenXineramaStrut check.

Such check would have to run after the calls in updateClientArea and conditionally reset the previous area to ignore the strut. Alternatively, a flag to skip the check on the offscreen tests would likely do, but semantically the function constrains a generic area, not the entire root geometry.

cc. "tests test the tests". always.
Comment 7 Martin Flöser 2016-06-02 07:31:29 UTC
Git commit 1a23ab7cf05ecf44cb105db43ebbd5db515d31ee by Martin Gräßlin.
Committed on 02/06/2016 at 07:29.
Pushed by graesslin into branch 'master'.

[autotest] Add test case exposing problem of bug 363804

The test case sets up apol's screen setup and the panel as described
in the attachements in bug 363804. As the test shows the panel's strut
is incorrectly ignored.

M  +88   -7    autotests/wayland/struts_test.cpp

http://commits.kde.org/kwin/1a23ab7cf05ecf44cb105db43ebbd5db515d31ee
Comment 8 Martin Flöser 2016-06-02 09:00:18 UTC
patch for the regression: https://phabricator.kde.org/D1744
Comment 9 Thomas Lübking 2016-06-02 09:18:08 UTC
I'd also frankly object the entire idea of the patch.
The typical bug would be that an inner client struts ie. +1920+432+32x768 sets a left strut of 1952, isn't?

In that case you'd likely want to perform a spatial sanitation, ie. ignore the strut for that area because it spans across the entire screen in strut direction, not because it leaves "some" intersection (for a "proper" partial strut broken this particular way usually will)

Also I'd remove the check from the ::adjustedClientArea() function and move it to ::updateClientArea() where it belongs as the actual strutting is done there.
Yes, that defeats the present tests, but they've proven to be worthless and the new magic condition in ::adjustedClientArea() is just a magic condition - the approach damages code to support tests.
In doubt one could add a test for ::updateClientArea()
Comment 10 Martin Flöser 2016-06-02 14:34:29 UTC
(In reply to Thomas Lübking from comment #9)
> I'd also frankly object the entire idea of the patch.
> The typical bug would be that an inner client struts ie. +1920+432+32x768
> sets a left strut of 1952, isn't?

It's also about ensuring that during screen reconfiguring we are not in a state where a client hasn't updated or already updated. Screen changes are racing and the suggested change ensures that it doesn't break.

The main reason I worked on it was bug 361551 which shows that it can happen in a multi screen setup that a screen gets excluded.

So yes, I still think that we should make sure this cannot happen.

> Also I'd remove the check from the ::adjustedClientArea() function and move it to ::updateClientArea() where it belongs as the actual strutting is done there.

Sure can be done there.
Comment 11 Thomas Lübking 2016-06-02 15:01:06 UTC
The spatial check would actually be more aggressive in defeating any strut that spans a screen left to right OR top to botton - not "AND".
In the formerly described scenario, the present approach would leave a 432px high strip on the top of the left screen, but on center or non-aligned screens or with more struts, that strip could be 1px (and still be permitted)

Removing *only* the screen crossing part of a inner strut (and leaving the tail on the desired screen) would be another way to allow inner struts (ie. the panel on the inner left edge sets a strut from the outer left edge and kwin removes the part on the left screen, ending up with an unstrutted left and a strutting inner panel on the right screen - not that we'd be somehow adhering the spec by this ;-)
Comment 12 Martin Flöser 2016-06-15 12:48:31 UTC
Git commit e5fe3137b8a1b82360aa1507b6bcee65089d93b1 by Martin Gräßlin.
Committed on 15/06/2016 at 12:48.
Pushed by graesslin into branch 'master'.

Fix the ignore struts multi-screen handling

Summary:
The checks in Client::adjustedClientArea were a little bit too
agressive, excluding also valid setups.

This change addresses the regression by keeping the actual intended
improvements in place.

The check in Client::adjustedClientArea is now only done for the
special case of desktopArea == area. This ensures that a strut excluding
a complete screen won't affect the overall workarea.

In addition new checks are introduced in Workspace::updateClientArea.
When calculating the new sareas a check is performed whether the
intersection with the adjustedClientArea would result in the sarea
becoming empty (thus a screen being completely removed). If that's the
case the geometry is ignored to not exclude a complete screen.

Interestingly I should have noticed that something with the logic is
odd. As the test case had two commented geometries which we now get.

Reviewers: #plasma, apol, lbeltrame

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D1744

M  +2    -4    autotests/wayland/struts_test.cpp
M  +19   -13   geometry.cpp

http://commits.kde.org/kwin/e5fe3137b8a1b82360aa1507b6bcee65089d93b1