| Summary: | Struts are broken with multiple screens | ||
|---|---|---|---|
| Product: | [Plasma] kwin | Reporter: | Aleix Pol <aleixpol> |
| Component: | platform-x11-nested | Assignee: | KWin default assignee <kwin-bugs-null> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | lbeltrame, sebas |
| Priority: | NOR | Flags: | mgraesslin:
ReviewRequest+
|
| Version: | git master | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| URL: | https://phabricator.kde.org/D1744 | ||
| Latest Commit: | http://commits.kde.org/kwin/e5fe3137b8a1b82360aa1507b6bcee65089d93b1 | Version Fixed In: | |
| Sentry Crash Report: | |||
|
Description
Aleix Pol
2016-06-01 16:56:33 UTC
please run "xprop" on the supposingly strutting panel and "xrandr -q" and attach both outputs. 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) Wild guess, what if you left-align the lower screen? xrandr --output LVDS1 --pos 0x1080 That does fix the problem. Note that this used to work up until few weeks ago... (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. 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. 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 patch for the regression: https://phabricator.kde.org/D1744 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() (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. 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 ;-) 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 |