Review Request: Fixes for screen hotplugging

Discussion:

Anthony Bryant

2010-06-24 01:37:16 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4451/
-----------------------------------------------------------

Review request for Plasma.

Summary
-------

This patch fixes a few problems that occur when hotplugging screens:
1. On startup, existing containments are associated with a screen even if that screen does not exist.
This causes a bug when a screen is added: since the containment already thinks it is associated with the new screen,
it doesn't emit screenChanged() with the new screen, and doesn't get a view created for it.
To fix this, I've made Containment::setScreen() set the new screen to -1 if it doesn't exist.
2. When a screen is removed, a containment will remain associated with the screen that it is on, causing the same bug
as in 1 when the screen is added again.
To fix this, I've made sure a containment's screen is set to -1 when its view is removed by PlasmaApp.
3. PlasmaApp tries to connect to a nonexistent signal in Kephal: screenAdded(int), the real one is screenAdded(Kephal::Screen*)

I'm not completely sure that this is the best way of fixing these problems, please correct me if it isn't.

Diffs
-----

/trunk/KDE/kdebase/workspace/plasma/desktop/shell/plasmaapp.h 1141983
/trunk/KDE/kdebase/workspace/plasma/desktop/shell/plasmaapp.cpp 1141983
/trunk/KDE/kdelibs/plasma/containment.cpp 1141983

Diff: http://reviewboard.kde.org/r/4451/diff

Testing
-------

Started plasma with and without an external screen and tried adding and removing it a few times.

Thanks,

Anthony

Chani Armitage

2010-06-24 06:59:25 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4451/#review6253
-----------------------------------------------------------

thanks for tackling this :)

there are other situations where the screen # may already be set - when you import a containment, either by starting a stopped activity or some other way. there are a couple of functions for that in libplasma (Corona iirc -load/importLayout?) that would need patching. you'd also have to apply the same logic to anywhere a *desktop* number could remain set, because PDV has the same problem (turn it on, remove a VD, add a VD, no view).

however, I'm thinking that this isn't the best way to solve it - you'd always be looking over your shoulder wondering if you'd missed a way for the screen or desktop # to get set. It might be better to do it the way that (iirc) panel views do it: when desktopcorona checks the screens, it fakes a containmentAdded, and that leads to plasmaapp putting the containment in the view-creation queue.

...or hell, maybe it would make more sense for plasmaapp to just ensure that the views exist as soon as the screen/desktop does, and trust the view and containment to find each other? take a step back and think about what would be most reliable and simple. The codebase for multiscreen/desktop has grown rather organically, and needs a bit of review now :)

- Chani

Post by Anthony Bryant
-----------------------------------------------------------
http://reviewboard.kde.org/r/4451/
-----------------------------------------------------------
(Updated 2010-06-24 01:37:15)
Review request for Plasma.
Summary
-------
1. On startup, existing containments are associated with a screen even if that screen does not exist.
This causes a bug when a screen is added: since the containment already thinks it is associated with the new screen,
it doesn't emit screenChanged() with the new screen, and doesn't get a view created for it.
To fix this, I've made Containment::setScreen() set the new screen to -1 if it doesn't exist.
2. When a screen is removed, a containment will remain associated with the screen that it is on, causing the same bug
as in 1 when the screen is added again.
To fix this, I've made sure a containment's screen is set to -1 when its view is removed by PlasmaApp.
3. PlasmaApp tries to connect to a nonexistent signal in Kephal: screenAdded(int), the real one is screenAdded(Kephal::Screen*)
I'm not completely sure that this is the best way of fixing these problems, please correct me if it isn't.
Diffs
-----
/trunk/KDE/kdebase/workspace/plasma/desktop/shell/plasmaapp.h 1141983
/trunk/KDE/kdebase/workspace/plasma/desktop/shell/plasmaapp.cpp 1141983
/trunk/KDE/kdelibs/plasma/containment.cpp 1141983
Diff: http://reviewboard.kde.org/r/4451/diff
Testing
-------
Started plasma with and without an external screen and tried adding and removing it a few times.
Thanks,
Anthony

Aaron Seigo

2010-06-24 14:27:12 UTC

Permalink

Post by Anthony Bryant
This causes a bug when a screen is added: since the containment already thinks it is associated with the new screen,
it doesn't emit screenChanged() with the new screen, and doesn't get a view created for it.

this is the reason for the second parameter in DesktopCorona::checkScreen(int screen, bool signalWhenExists)

when a screen appears, it should emit the signal contaimentAdded(Plasma::Containment*) even if the containment previously existed, as that will trigger PlasmaApp to create a view for it. the question is why isn't this working anymore (it was in 4.4, e.g.)

Post by Anthony Bryant
To fix this, I've made Containment::setScreen() set the new screen to -1 if it doesn't exist.

and then we lose all screen<->containment affinity, and on a system with >2 screens the order of the containments will be completely random when the screens are re-attached. this part of the patch is incorrect.

Post by Anthony Bryant
3. PlasmaApp tries to connect to a nonexistent signal in Kephal: screenAdded(int), the real one is screenAdded(Kephal::Screen*)

good catch; Kephal::screenRemoved(int) isn't symmetrical to this, a real design wart in kephal imho. anyways, i've merged this part of it.

- Aaron

Aaron Seigo

2010-06-24 14:39:03 UTC

Permalink

Anthony Bryant

2010-06-24 15:39:08 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4451/
-----------------------------------------------------------

(Updated 2010-06-24 15:39:08.285730)

Review request for Plasma.

Summary (updated)
-------

Aaron's patch almost fixed the problem, but there's still the issue that the view is getting created on startup even if its screen does not exist.
I've added a few checks in createWaitingDesktops() to make sure a view is only created if it's for a containment with a valid screen and desktop.

Diffs (updated)
-----

/trunk/KDE/kdebase/workspace/plasma/desktop/shell/plasmaapp.cpp 1142286

Diff: http://reviewboard.kde.org/r/4451/diff

Testing (updated)
-------

Started plasma with and without an external screen and tried adding and removing it a few times, with and without per virtual desktop views.

Thanks,

Anthony

Aaron Seigo

2010-06-24 17:38:40 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4451/#review6266
-----------------------------------------------------------

Ship it!

looks good; i've committed this patch as it seems you don't have an svn account? (if you would like to continue working on these kinds of patches, we can hook you up with one)

- Aaron