D13853: Fix setting primary connector if primary output changed
ngraham closed this revision. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D13853 To: hoffmannrobert, #plasma, mart, davidedmundson Cc: davidedmundson, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13853: Fix setting primary connector if primary output changed
ngraham added a comment. Done! REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D13853 To: hoffmannrobert, #plasma, mart, davidedmundson Cc: davidedmundson, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13853: Fix setting primary connector if primary output changed
hoffmannrobert added a comment. Thanks for reviewing. Can you please land it for me, I don't have commit access. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D13853 To: hoffmannrobert, #plasma, mart, davidedmundson Cc: davidedmundson, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13853: Fix setting primary connector if primary output changed
hoffmannrobert updated this revision to Diff 37174. hoffmannrobert added a comment. - Remove wrong Q_ASSERT REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13853?vs=37072=37174 BRANCH master REVISION DETAIL https://phabricator.kde.org/D13853 AFFECTED FILES shell/screenpool.cpp To: hoffmannrobert, #plasma, mart, davidedmundson Cc: davidedmundson, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13853: Fix setting primary connector if primary output changed
hoffmannrobert marked an inline comment as done. hoffmannrobert added a comment. Addition to last comment: In the other case, booting with HDMI-2 and hotplugging HDMI-3 the problem with primary switching doesn't exist. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D13853 To: hoffmannrobert, #plasma, mart, davidedmundson Cc: davidedmundson, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13853: Fix setting primary connector if primary output changed
hoffmannrobert marked an inline comment as done. hoffmannrobert added inline comments. INLINE COMMENTS > davidedmundson wrote in screenpool.cpp:108 > Either this is a valid case to be in, and this assert doesn't make sense. > > Or we should never be in this case, and the patch doesn't make sense. > > I don't fully understand the background of how we end up in this situation to > say which it is. This is a valid case, and the assert doesn't make sense, it needs to be removed. In the case described in the test plan, the old primary display ist HDMI-3, the new primary is HDMI-2. If HDMI-2 is plugged in and "extend to right" is selected in the OSD, the QGuiApplication::screenAdded signal starts ShellCorona::addOutput(). This checks, if the new screen is redundant (ShellCorona::isOutputRedundant). At this time, both have the same geometry (two equal screens, QRect(0,0 1920x1080)), so yes, the new screen is considered redundant and not added to the screen pool. There is already some remedy against this wrong assumption: On the signal QGuiApplication::primaryScreenChanged ShellCorona::primaryOutputChanged is called. Here, with m_reconsiderOutputsTimer.start() ShellCorona::reconsiderOutputs() is run at a later time, which will correct this. But that's too late for the primary change: before reconsiderOutputs() is run, ScreenPool::setPrimaryConnector( newPrimary->name() ) is called. Here, m_idForConnector does not contain this new primary, it only knows "HDMI-3", which is still mapped to 0. If the Q_ASSERT is disabled, m_idForConnector.value(primary (The new primary HDMI-2!) ) returns 0, so both HDMI-2 and HDMI-3 are mapped to 0 in m_idForConnector, and m_connectorForId[0] is set to "HDMI-2", so the mapping to HDMI-3 is lost. The other case, booting with HDMI-2 and hotplugging HDMI-3 works, because in ShellCorona::isOutputRedundant, geometries are different: HDMI-2 has: QRect(0,0 1920x1080) and HDMI-3: QRect(1920,0 1920x1080), so one doesn't contain the other. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D13853 To: hoffmannrobert, #plasma, mart, davidedmundson Cc: davidedmundson, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13853: Fix setting primary connector if primary output changed
davidedmundson requested changes to this revision. davidedmundson added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > screenpool.cpp:108 > } > Q_ASSERT(m_idForConnector.contains(primary)); > Either this is a valid case to be in, and this assert doesn't make sense. Or we should never be in this case, and the patch doesn't make sense. I don't fully understand the background of how we end up in this situation to say which it is. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D13853 To: hoffmannrobert, #plasma, mart, davidedmundson Cc: davidedmundson, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13853: Fix setting primary connector if primary output changed
hoffmannrobert created this revision. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. hoffmannrobert requested review of this revision. REVISION SUMMARY If a user logged in with one screen connected plugs in a second screen, which becomes the new primary screen, this screen would stay black or behave weird. Unplugging the screen again would mess up plasmashell. Added to ScreenPool::setPrimaryConnector(): In the case primary output changed m_idForConnector doesn't contain the new primary, so a screen mapping is created for it. TEST PLAN Testing on virtualbox or vmware player seems impossible, because these don't allow disabling the first display (VGA-1) and booting with the second (VGA-2) only. 1. Boot machine with one screen connected to HDMI-3 (primary output). 2. Log in 3. Plug in second screen to HDMI-2: > primary output changes from HDMI-3 to HDMI-2 == 4. OSD appears: extend to right > Without this patch, the new screen (HDMI-2) would stay blank. === > With this patch applied, the screen content moves to the new == second screen. 5. Unplug second screen (HDMI-2) > Without this patch, the background would get black, control panel === would disappear, could only be restored by restart of plasmashell > With this patch applied, screen content moves to the right and works REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D13853 AFFECTED FILES shell/screenpool.cpp To: hoffmannrobert Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
