[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell

[mart (Marco Martin)]

mart added a comment.


  In https://phabricator.kde.org/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)

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D2218

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160721/24ea0d17/attachment-0001.html>