Fix lid open/closed configurations, OSD actions, more than 2 screens
Needs ReviewPublic

Authored by hoffmannrobert on May 8 2019, 2:32 PM.

Details

Reviewers
romangg
Group Reviewers
Plasma
Maniphest Tasks
T11095: Further KScreen / multi-display improvements
Summary

Screen configurations on notebooks couldn't distinguish
properly between open lid and closed lid configurations. The
open lid config was only created if the lid was closed and
deleted if the lid was opened again. Known configurations
were ignored.
This is fixed by keeping open and closed lid configurations
permanently in different config files.

Fixed: the OSD was displayed sometimes on a closed laptop panel.

With more than two screens, using the OSD always generated an "extend to right" configuration, even if "extend to left", "clone", or "disable external/internal" was selected. Now these options do what they say with more than two screens, too.

Test Plan

Test 1:
Use a notebook with one external screen connected, no kscreen configuration
(rm -fr ~/.local/share/kscreen).

Have lid open, login, OSD is displayed, configure as you like,
logout, close lid, login

Without patch: screen without panel is shown, both screens are activated

With patch: screen with panel is shown, only external screen is activated

Test 2:
Same HW config as in Test 1, no kscreen configuration.

Have lid closed, login

Without patch: screen without panel is shown, both screens are activated,
OSD is shown on closed notebook panel

With patch: screen with panel is shown, only external screen is activated,
OSD is not shown

Test 3:
Use a notebook with two external screens connected, no kscreen configuration

Have lid open, login, OSD is displayed, click any configuration option

Without patch: no matter which option you choose, always "extend to the right"
is done

With patch: OSD options do what they say

Test 4:
continuing from Test 3: logout, close lid, login

Without patch: closed notebook panel is still activated

With patch: only external screens are actived, OSD is shown

Test 5:
Same HW config as in Test 3, 4, no kscreen configuration.

Have lid closed, login

Without patch: external screens without panel are shown, all three screens are activated,
OSD is shown on closed notebook screen

With patch: on one screen a panel is shown, only external screens are activated,
OSD is shown

Test 6:
Same HW config as in Test 3, 4, 5, no kscreen configuration.

Have lid open, login, OSD is displayed

Configure the screens with the lid open.

Then close the lid and configure the screens differently.
Particularly for closed lid configure the screen connected to
the higher output number to the left (e.g. DP-1-2), the lower to
the right (e.g. DP-1-1). Open and close the lid.

With this patch applied screen configurations remain as previously
configured. Without, the higher output number is shown on the
right and the lower on the left.

Test 7:
Same HW config as in Test 3, 4, 5, 6, no kscreen configuration.

Have lid open, login, OSD is displayed

Configure the screens with the lid open.

Then close the lid and configure the screens differently.
Particularly for closed lid configure the screen connected to
the higher output number to the left (e.g. DP-1-2), the lower to
the right (e.g. DP-1-1). Open the lid, logout, close the lid, login

With this patch applied screen configurations remain as previously
configured. Without, the higher output number is shown on the
right and the lower on the left.

Diff Detail

Repository
R104 KScreen
Branch
fix_lid_open_closed_configs
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11666
Build 11684: arc lint + arc unit
hoffmannrobert created this revision.May 8 2019, 2:32 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 8 2019, 2:32 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hoffmannrobert requested review of this revision.May 8 2019, 2:32 PM
romangg added a subscriber: romangg.EditedMay 10 2019, 3:47 PM

So your goal is to support the following use case: User wants to set different screens properties (for example positions) on the same screen arrangement for when the lid is closed and when it is opened.

Correct? Had the lidOpenend mechanic this goal from the beginning or is this a new feature? To my understanding the lidOpenend mechanic does the following at the moment:

  1. Disable the internal display on closing the lid and reenable it when the lid is opened again.
  2. When the lid is opened again, restore to displays values, that were active before the lid was closed.

Function 2. is questionable. What if I move my external screens around while the lid is closed? Why should this become different for when the lid is opened again? With this logic the same holds the other way around. Why should the display positions of external screens become different just when I close the lid? It would make more sense to keep these display positions consistent independently of lid closed/opened and just reinsert the internal screen at a sensible / the old position. Can you explain why, as described in your test plan, it is sensible to have different display positions of external monitors for when having the lid opened and closed?

romangg edited reviewers, added: romangg; removed: broulik.May 10 2019, 3:48 PM

My goal is to make the screen configurations work reliably with notebooks, not to support a new use case. The use case is: work with a notebook docked and undocked, with and without external screen(s), lid open or closed.

The configurations work fine, if no notebook with a lid that can be opened or closed is involved.

But several users report, that they switch on their notebook (in docking station) in the morning, lid is closed, log in, and the external screens are swapped left and right again. Or only the empty background of the second screen is shown on the external monitor while the primary screen is on the closed integrated notebook screen.

So they configure it again as needed, while the lid is closed. But there is already a lidOpened-configuration lurking around, waiting for the lid to be opened and to destroy the configuration previously made.

Where does the wrong lidOpened-configuration come from? I guess it's created automatically when the lid is open and a button on the OSD ("Extend to right" or so) is clicked. After closing the lid, this configuration is copied to "lidOpened".

The situation described in the test plan is just a quick way to show what can go wrong and if that is fixed.

My goal is to make the screen configurations work reliably with notebooks, not to support a new use case. The use case is: work with a notebook docked and undocked, with and without external screen(s), lid open or closed.

The configurations work fine, if no notebook with a lid that can be opened or closed is involved.

But several users report, that they switch on their notebook (in docking station) in the morning, lid is closed, log in, and the external screens are swapped left and right again. Or only the empty background of the second screen is shown on the external monitor while the primary screen is on the closed integrated notebook screen.

This is the real issue. Why are the screen positions scrambled around although there should be a config from before shutdown/suspend (not the lidOpened one), that has the current positions stored. Apparently this one has the wrong positions saved as well. You could check this in your config files.

So they configure it again as needed, while the lid is closed. But there is already a lidOpened-configuration lurking around, waiting for the lid to be opened and to destroy the configuration previously made.

Where does the wrong lidOpened-configuration come from? I guess it's created automatically when the lid is open and a button on the OSD ("Extend to right" or so) is clicked. After closing the lid, this configuration is copied to "lidOpened".

If between yesterday and in the morning they never opened the lid, there should still be a lidOpened-config from the last time yesterday they closed the lid.

The situation described in the test plan is just a quick way to show what can go wrong and if that is fixed.

  • Fix OSD displayed on closed laptop panel.
  • Fix generated configurations if there are more than two screens.
hoffmannrobert edited the test plan for this revision. (Show Details)May 22 2019, 2:02 PM
hoffmannrobert edited the test plan for this revision. (Show Details)
hoffmannrobert edited the test plan for this revision. (Show Details)
hoffmannrobert edited the test plan for this revision. (Show Details)May 22 2019, 2:29 PM

This is the real issue. Why are the screen positions scrambled around although there should be a config from before shutdown/suspend (not the lidOpened one), that has the current positions stored. Apparently this one has the wrong positions saved as well. You could check this in your config files.

The problem is, there is no existing config anymore. If you configure screens with the lid closed, as soon as you open it again, the previous config is gone.

I added this scenario as Test 7 in the test plan.

If between yesterday and in the morning they never opened the lid, there should still be a lidOpened-config from the last time yesterday they closed the lid.

I guess they opened it some time after configuring and before logging out.

I added some more fixes:

  • The OSD was displayed even on a closed laptop panel.
  • With more than two screens, using the OSD always generated an "extend to right" configuration, even if "extend to left", "clone", or "disable external/internal" was selected. Now these options do what they say with more than two screens, too.
hoffmannrobert retitled this revision from Fix lid open/closed configurations to Fix lid open/closed configurations, OSD actions, more than 2 screens.May 22 2019, 2:40 PM
hoffmannrobert edited the summary of this revision. (Show Details)

I added your diff to a task I created for tracking some improvements to KScreen.

Please test out how lid open/closed configs behave with the new code on master branch. Also rebase on current master.

Put the additional two fixes you did in separate diffs if they are independent of open/closed configs question. It sounds like that.

Thank you!

hoffmannrobert edited the summary of this revision. (Show Details)
hoffmannrobert edited the test plan for this revision. (Show Details)

Rebase and adapt to new upstream code

I tested the new code:
Tests 1-5 of the test plan behave like previously "without patch", in tests 6 and 7 the integrated laptop screen isn't reacitivated after opening.

After adapting my changes to the new code, all tests work as described "with patch".

The additional fixes are not independent of the first fix, so they are not separated.