Bug 349400

Created attachment 93254 [details]
possible fix

PanelView is watching for kscreen changing to call updateStruts, but updateStruts is using QScreen's internals knowledge, which has the /potential/ to be off.

I don't have access to another monitor for 10 days, would anyone be able to try this patch out ^

Hi,
I tested your patch (applied on version 5.3.1). It works for the majority of the times, but it sometimes doesn't work. 3 out of the 10 times I tested it, it didn't work.

OK, sorry about that. I guess I need to debug what's going on a bit more.

If you want to help, you can add some debug into PanelView.cpp line 894 adding

qDebug() << "Updating struts: " << thisScreen << wholeScreen;

or I'll do it when I return.

DRAFT:

Hi,
Sorry, I have been busy with exams for the past two weeks, so I didn't have time to respond.
I have debuged it a bit, and I found out a few things:

1. The first disconnect after (re)starting plasmashell never works. updateStruts() isn't even called in this case.
1435993121: ===Plasmashell starting up===
1435993125: Updating struts: QRect(0,0 1366x768) QRect(0,0 3286x1080)
# Screen disconnected, waited a few seconds, screen reconnected
1435993153: Updating struts: QRect(0,0 1366x768) QRect(0,0 1366x768)
1435993154: Updating struts: QRect(0,0 1366x768) QRect(0,0 3286x1080)
1435993154: Updating struts: QRect(0,0 1366x768) QRect(0,0 3286x1080)
1435993154: Updating struts: QRect(0,0 1366x768) QRect(0,0 3286x1080)

2. When it does work, updateStruts() is called twice, first with the old screen size, then again with the new screen size.
1435994121: Updating struts:  QRect(0,0 1366x768) QRect(0,0 3286x1080)
1435994122: Updating struts:  QRect(0,0 1366x768) QRect(0,0 1366x768)
# Screen disconnected, waited a few seconds, screen reconnected
1435994132: Updating struts:  QRect(0,0 1366x768) QRect(0,0 3286x1080)
1435994132: Updating struts:  QRect(0,0 1366x768) QRect(0,0 3286x1080)

3. When it does not work, updateStruts() is called once, but with the old screen size.
1435994301: Updating struts:  QRect(0,0 1366x768) QRect(0,0 3286x1080)
# Screen disconnected, waited a few seconds, screen reconnected
1435994307: Updating struts:  QRect(0,0 1366x768) QRect(0,0 1366x768)
1435994308: Updating struts:  QRect(0,0 1366x768) QRect(0,0 3286x1080)
1435994308: Updating struts:  QRect(0,0 1366x768) QRect(0,0 3286x1080)
1435994308: Updating struts:  QRect(0,0 1366x768) QRect(0,0 3286x1080)

Oops, that "DRAFT" is not supposed to be there.

Is that with my possible fix patch applied or without?

Oh, forgot to mention, all of the things described above are with your patch applied. I just tested and found out that without your patch, updateStruts() isn't called either on connecting or disconnecting the display.

I figured out what's going on:

1) In the original code, panelview is listing to KScreen's outputAdded and outputRemoved, however, the output isn't 'added'/'removed' when it's connected/disconnected, it is mearly marked as (dis)connected.

2) Your patch listens to QScreen's virtualGeometryChanged, but that isn't always notified when it changes (I don't know why, might be a QScreen bug).

3) KScreen::Screen has a currentSize, but that's also broken. It is not updated by Screen::apply when it should really be. I filled the bug as https://bugs.kde.org/show_bug.cgi?id=352001 The bright side is that with that bug fixed, currentSizeChanged is usable. Therefore, I propose a fix based on it:



diff --git a/shell/panelview.cpp b/shell/panelview.cpp
index f67ae7d..c9b1968 100644
--- a/shell/panelview.cpp
+++ b/shell/panelview.cpp
@@ -106,14 +106,9 @@ PanelView::PanelView(ShellCorona *corona, QScreen *targetScreen, QWindow *parent
     m_strutsTimer.setSingleShot(true);
     connect(&m_strutsTimer, &QTimer::timeout,
             this, &PanelView::updateStruts);
-    connect(m_corona->screensConfiguration().data(), &KScreen::Config::outputAdded,
-            this, [this] (const KScreen::OutputPtr &) {
-                updateStruts();
-            });
-    connect(m_corona->screensConfiguration().data(), &KScreen::Config::outputRemoved,
-            this, [this] (int) {
-                updateStruts();
-            });
+
+    connect(m_corona->screensConfiguration()->screen().data(), &KScreen::Screen::currentSizeChanged,
+            this, &PanelView::updateStruts);
 
     qmlRegisterType<QScreen>();
     engine()->rootContext()->setContextProperty("panel", this);



I have tested that it works on my computer. Just a reminder, it only works if libkscreen is patched with the patch in Bug 352001.

>2) Your patch listens to QScreen's virtualGeometryChanged, but that isn't always notified when it changes (I don't know why, might be a QScreen bug).

Seems so; Qt has a /lot/ of bugs with QScreen
Will be worth fixing that long term

Patch seems OK, can you put it on reviewboard like you have for your kscreen one?

Now you're an expert at panel struts; do you think these two bugs are the same as this:

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

Submitted: https://git.reviewboard.kde.org/r/124996/

Bug 349094 is the same as this bug, basically it is the reverse of the scenario in the description: the strut doesn't update and becomes thinner than it should be.
Bug 347195 looks related, but it might be caused by bugs in updateStruts. Disconnecting the primary output seems to cause a lot of problems. My simple testing (disconencting the primary output and then reconnecting) causes an integer overflow in the struts. Reconnecting it again causes even more problems (I've even seen windows disappearing).
I'll look into these problems.

*** Bug 349094 has been marked as a duplicate of this bug. ***

Git commit d66d6d57d34f3fc46a1cb7886f06dfe5b7a61763 by David Edmundson, on behalf of Andrew Chen.
Committed on 31/08/2015 at 08:09.
Pushed by davidedmundson into branch 'Plasma/5.4'.

Update struts on screen size change

Calls updateStruts on KScreen::Screen::currentSizeChanged instead of
KScreen::Config::outputAdded/outputRemoved.
When an output is (dis)conencted, it isn't added/removed, it is mearly
marked as (dis)connected. Therefore, updating struts on
outputAdded/outputRemoved doesn't work.
REVIEW: 124996

M  +3    -8    shell/panelview.cpp

http://commits.kde.org/plasma-workspace/d66d6d57d34f3fc46a1cb7886f06dfe5b7a61763

Hello

Using plasma-5.4.0:

Disconnected:
$ xrandr -q 
Screen 0: minimum 8 x 8, current 1920 x 1080, maximum 8192 x 8192
DVI-I-0 disconnected (normal left inverted right x axis y axis)
LVDS-0 connected primary 1920x1080+0+0 (normal left inverted right x axis y axis) 382mm x 215mm
   1920x1080     60.01*+
HDMI-0 disconnected (normal left inverted right x axis y axis)
DVI-I-1 disconnected (normal left inverted right x axis y axis)

Connected:
$ xrandr -q 
Screen 0: minimum 8 x 8, current 3840 x 1200, maximum 8192 x 8192
DVI-I-0 disconnected (normal left inverted right x axis y axis)
LVDS-0 connected primary 1920x1080+0+0 (normal left inverted right x axis y axis) 382mm x 215mm
   1920x1080     60.01*+
HDMI-0 disconnected (normal left inverted right x axis y axis)
DVI-I-1 connected 1920x1200+1920+0 (normal left inverted right x axis y axis) 519mm x 324mm
   1920x1200     59.95*+
   1920x1080     59.93  
   1680x1050     59.95  
   1600x1200     60.00  
   1280x1024     60.02  
   1280x960      60.00  
   1024x768      60.00  
   800x600       60.32  
   640x480       59.94  

Disconnected
$ xrandr -q 
Screen 0: minimum 8 x 8, current 1920 x 1080, maximum 8192 x 8192
DVI-I-0 disconnected (normal left inverted right x axis y axis)
LVDS-0 connected primary 1920x1080+0+0 (normal left inverted right x axis y axis) 382mm x 215mm
   1920x1080     60.01*+
HDMI-0 disconnected (normal left inverted right x axis y axis)
DVI-I-1 disconnected (normal left inverted right x axis y axis)