[kwin] [Bug 363804] Struts are broken with multiple screens

[Martin Gräßlin via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

Martin Gräßlin  changed:

   What|Removed |Added

  Latest Commit||http://commits.kde.org/kwin
   ||/e5fe3137b8a1b82360aa1507b6
   ||bcee65089d93b1
 Resolution|--- |FIXED
 Status|CONFIRMED   |RESOLVED

--- Comment #12 from Martin Gräßlin  ---
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-4autotests/wayland/struts_test.cpp
M  +19   -13   geometry.cpp

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

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Martin Gräßlin via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #10 from Martin Gräßlin  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Thomas Lübking via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #9 from Thomas Lübking  ---
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()

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Martin Gräßlin via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

Martin Gräßlin  changed:

   What|Removed |Added

URL||https://phabricator.kde.org
   ||/D1744
  Flags||ReviewRequest+

--- Comment #8 from Martin Gräßlin  ---
patch for the regression: https://phabricator.kde.org/D1744

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Luca Beltrame via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

Luca Beltrame  changed:

   What|Removed |Added

 CC||lbeltr...@kde.org

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Martin Gräßlin via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #7 from Martin Gräßlin  ---
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   -7autotests/wayland/struts_test.cpp

http://commits.kde.org/kwin/1a23ab7cf05ecf44cb105db43ebbd5db515d31ee

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Thomas Lübking via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

Thomas Lübking  changed:

   What|Removed |Added

 Ever confirmed|0   |1
 Status|UNCONFIRMED |CONFIRMED

--- Comment #6 from Thomas Lübking  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Martin Gräßlin via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #5 from Martin Gräßlin  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Aleix Pol via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #4 from Aleix Pol  ---
That does fix the problem.
Note that this used to work up until few weeks ago...

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Thomas Lübking via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #3 from Thomas Lübking  ---
Wild guess, what if you left-align the lower screen?

   xrandr --output LVDS1 --pos 0x1080

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Aleix Pol via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #2 from Aleix Pol  ---
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.3256.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.8759.98 
   1600x900  60.00  
   1280x1024 75.0260.02  
   1440x900  74.9859.89  
   1280x960  60.00  
   1366x768  59.79  
   1360x768  60.02  
   1280x800  74.9359.81  
   1152x864  75.00  
   1280x768  74.8959.87  
   1280x720  60.00  
   1024x768  75.0870.0760.00  
   1024x576  59.97  
   800x600   72.1975.0060.3256.25  
   848x480   60.00  
   640x480   75.0072.8160.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)

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Thomas Lübking via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

--- Comment #1 from Thomas Lübking  ---
please run "xprop" on the supposingly strutting panel and "xrandr -q" and
attach both outputs.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kwin] [Bug 363804] Struts are broken with multiple screens

[Aleix Pol via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=363804

Aleix Pol  changed:

   What|Removed |Added

 CC||se...@kde.org

-- 
You are receiving this mail because:
You are watching all bug changes.