I have been using this patch for a couple of days now and I haven’t experienced issues anymore with the screens. Before the patch, I had the issue that despite the correct kscreen configuration, the panel would start on the wrong screen, etc. (This was then corrected by restarting plasmashell). Also from time to time I had the issue that one of the two screens would be completely black and no background or panel would be displayed.

Both issues have no longer occurred, despite using three different setups (Single laptop screen, Home with external monitor as primary and laptop screen on the right, Work with external monitor as primary and laptop screen on the left). Everything works now as it should.

When adding a new design can you describe the design in words. i.e what roles a class has
It'd make things a lot easier.

ScreenPool:
It seems to have two jobs:

• you've got the converting screen "names" to IDs.
• "Tracking" DesktopViews

Why do we have m_idsForConnector and m_idsForDesktopView

m_idsForDesktopView is bound to be the same as
m_idsForConnector(view->screenToFollow()->name()) right?

at which point we can kill the second one, and that simplifies a lot of the code and avoid something that can go out of sync.

Panels are now a bit of a mix:

In createWaitingPanels we base the panel screen based on where the DesktopView is
in primaryChanged - we don't and use screens API directly (BUG: and setting screenToFollow)
in removeView we're using the View as the indicator of what screen is which.
screenForContainment is also going via View.

Is it meant to be going via a View or not?

Personally I think it's a bit weird as we can generate the index directly from the screen, but whatever it should be it should be consistent

bugs:
I think you also have a potential crash if you switch primary on a panel and then disconnect the first screen

This code will have a bug if you drag a panelview to another screen - then resize the first screen.
Same for if you have a panel on a primary screen then switch primary then alter the screen.

You have a crash if:

• you have previously inserted a containment on screen A
• you reboot with screen B, C
• number of screens ==2, firstAvailableIndex=1
• the first containment will restore
• the second one will be a new containment with an ID of 3
• createContainment checks number of ScreensAvailable and will return a null pointer.

You have a crash if:

edit, that final crash doesn't exist anymore. Didn't see that you'd removed the numScreens check in p-f

In D2218#41550, @davidedmundson wrote:

When adding a new design can you describe the design in words. i.e what roles a class has
It'd make things a lot easier.

ScreenPool:
It seems to have two jobs:

• you've got the converting screen "names" to IDs.
• "Tracking" DesktopViews
  
  Why do we have m_idsForConnector and m_idsForDesktopView
  
  m_idsForDesktopView is bound to be the same as m_idsForConnector(view->screenToFollow()->name()) right?
  
  at which point we can kill the second one, and that simplifies a lot of the code and avoid something that can go out of sync.

I don't think I can drop idsForConnector, because (as connectorsForId) it contains associations also of screens that it remembers but may be disabled right now (it saves and loads to/from configuration file)

i may be able to kill idsfordesktopview tough, will update the rr

Panels are now a bit of a mix:

In createWaitingPanels we base the panel screen based on where the DesktopView is
in primaryChanged - we don't and use screens API directly (BUG: and setting screenToFollow)

views gets switched in screens only in the case the primary screen changes, that's the one moment you want to see panels and desktops moving to a different screen

do you think if panels would be completely managed in screenpool as well logic would be more readable?

in removeView we're using the View as the indicator of what screen is which.
screenForContainment is also going via View.

Is it meant to be going via a View or not?

Personally I think it's a bit weird as we can generate the index directly from the screen, but whatever it should be it should be consistent

what we know there is that a desktopview must be removed due to a screen death, so the panels with that screen should be killed as well.
perhaps would be more clear if the logic for that is completely moved in screenpool?

bugs:
I think you also have a potential crash if you switch primary on a panel and then disconnect the first screen

hmm, i tried to brutally disconnect the primary screen, but doesn't seem to crash?

This code will have a bug if you drag a panelview to another screen - then resize the first screen.

why? when dragging to another screen screentofollow of the panel would be changed as well.

Same for if you have a panel on a primary screen then switch primary then alter the screen.

You have a crash if:

• you have previously inserted a containment on screen A
• you reboot with screen B, C
• number of screens ==2, firstAvailableIndex=1
• the first containment will restore
• the second one will be a new containment with an ID of 3
• createContainment checks number of ScreensAvailable and will return a null pointer.

yes, there should be a new containment as the one of screen a should stay for when/if screen a is connected again (should be fine with latest libplasma)

Something has got messed up with the patch?

i may be able to kill idsfordesktopview tough, will update the rr

Yep, that's exactly what I meant. ++
Also means panels can use the API directly. (see comment #3)

perhaps would be more clear if the logic for that is completely moved in screenpool?

There should definitely be some consistency. Either screenpool is in charge of managing views or shellcorona is.

Either woudl be valid but currently this patch current inserts and removals of DesktopView to the right screen are all handled by ShellCorona setting the screen, but primary changes of DesktopView are done by ScreenPool; and Panels are mixed up too.

Personally, I think the part that's out of place is ScreenPool touching the desktop views; if you move m_desktopViewforId to shellCorona the design all fits:
insert/remove in ScreenPool become reflective
and all DesktopView stuff is within one class, screen to ID mapping is in one class.

In D2218#41871, @davidedmundson wrote:

Personally, I think the part that's out of place is ScreenPool touching the desktop views; if you move m_desktopViewforId to shellCorona the design all fits:
insert/remove in ScreenPool become reflective
and all DesktopView stuff is within one class, screen to ID mapping is in one class.

I tried to move the panels logic in screenpool but i didn't really like it, screenpool kindof became almost a small clone of corona..
I'll try instead moving m_desktopViewforId in corona

most comments should be adressed now, will need again a period of test with several multiscreen users, as it has changed a bit

In D2218#41975, @mart wrote:

most comments should be adressed now, will need again a period of test with several multiscreen users, as it has changed a bit

I've been running this for a while, in two setups (one with 3 identical screens, another with two screens of which one is rotated 90 degrees) with no issues. Coupled with the already-merged patch by sebas in kscreen, I get no containment dance anymore at startup.