Fix setting primary connector if primary output changed
ClosedPublic

Authored by hoffmannrobert on Jul 2 2018, 3:40 PM.

Details

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

  1. 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.
  1. 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

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 562
Build 574: arc lint + arc unit
hoffmannrobert created this revision.Jul 2 2018, 3:40 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 2 2018, 3:40 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hoffmannrobert requested review of this revision.Jul 2 2018, 3:40 PM
hoffmannrobert edited the test plan for this revision. (Show Details)Jul 2 2018, 3:42 PM
ngraham added a subscriber: ngraham.Jul 3 2018, 5:05 PM
davidedmundson requested changes to this revision.Jul 3 2018, 5:17 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
shell/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 revision now requires changes to proceed.Jul 3 2018, 5:17 PM
hoffmannrobert marked an inline comment as done.Jul 4 2018, 2:15 PM
hoffmannrobert added inline comments.
shell/screenpool.cpp
108

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.

hoffmannrobert marked an inline comment as done.Jul 4 2018, 2:23 PM

Addition to last comment: In the other case, booting with HDMI-2 and hotplugging HDMI-3 the problem with primary switching doesn't exist.

  • Remove wrong Q_ASSERT
davidedmundson accepted this revision.Aug 28 2018, 10:42 AM
This revision is now accepted and ready to land.Aug 28 2018, 10:42 AM

Thanks for reviewing. Can you please land it for me, I don't have commit access.

ngraham closed this revision.Aug 30 2018, 11:22 PM