Skip to content

fix(windows): recover display layout after undocking - #298

Merged
ReenigneArcher merged 12 commits into
LizardByte:masterfrom
jdvmi00:fix/laptop-undock
Sep 29, 2026
Merged

ReenigneArcher merged 12 commits into
LizardByte:masterfrom
jdvmi00:fix/laptop-undock

Conversation

@jdvmi00

@jdvmi00 jdvmi00 commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Description

I ran into this while using Sunshine with a docked Windows laptop, lid closed, and a virtual display selected with ensure_only_display. Unplugging the dock during a stream and then disconnecting leaves the original monitor unavailable. Opening the lid turns the panel on, but the virtual display stays enabled and the old recovery state keeps being retried.

This lets restoration fall back to surviving original displays, or a built-in panel if none remain. It checks that the replacement is active before removing displays enabled by the session, and only clears the recovery state after verifying the final layout. With the lid closed and no physical display available, recovery stays pending.

There was a second part to the same repro: the streaming layout became Windows' saved docked layout, so redocking brought back the virtual display instead of the external monitor. WinDisplayDevice now has an optional save_to_database argument so callers can make temporary CCD changes. The default stays unchanged. The companion Sunshine change is LizardByte/Sunshine#5625.

Testing:

  • 103 common tests pass on the current master base.
  • 431 tests pass on Windows with the current PR sources, including all 11 undock recovery tests and the saved/temporary display tests. The runner skips 10 hardware-only tests; two existing tests are disabled: build and report.
  • The release-based trial passed the Windows library and Sunshine suites, including the new regression tests. Hardware-only tests were skipped on the runner: build and reports.
  • The original recovery code fails the undock regression in that trial; the patched code passes.
  • Tested the full docked stream → unplug with lid closed → disconnect → open lid → redock sequence on the laptop. The panel recovered, the virtual display turned off, recovery state cleared, and redocking restored the external monitor. QDC_DATABASE_CURRENT also confirmed the saved docked layout stayed unchanged while streaming.

The hardware trial used Sunshine v2026.516.143833. This PR ports those changes to current master. The Windows run above is in my fork; upstream Actions still need maintainer approval to run.

Screenshot

Issues Fixed or Closed

Roadmap Issues

Type of Change

  • feat: New feature (non-breaking change which adds functionality)
  • fix: Bug fix (non-breaking change which fixes an issue)
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semicolons, etc.)
  • refactor: Code change that neither fixes a bug nor adds a feature
  • perf: Code change that improves performance
  • test: Adding missing tests or correcting existing tests
  • build: Changes that affect the build system or external dependencies
  • ci: Changes to CI configuration files and scripts
  • chore: Other changes that don't modify src or test files
  • revert: Reverts a previous commit
  • BREAKING CHANGE: Introduces a breaking change (can be combined with any type above)

Checklist

  • Code follows the style guidelines of this project
  • Code has been self-reviewed
  • Code has been commented, particularly in hard-to-understand areas
  • Code docstring/documentation-blocks for new or existing methods/components have been added or updated
  • Unit tests have been added or updated for any new or modified functionality

AI Usage

See our AI usage policy.

  • None: No AI tools were used in creating this PR
  • Light: AI provided minor assistance (formatting, simple suggestions)
  • Moderate: AI helped with code generation or debugging specific parts
  • Heavy: AI generated most or all of the code changes

@CLAassistant

CLAassistant commented Sep 6, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@ReenigneArcher

ReenigneArcher commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

Hello, thanks for the PR!

I review this with GPT 6 Sol, extra high.


I found two issues:

  1. [P1] Undock recovery can be skipped when the modified topology includes the unplugged display. The new fallback runs only after revertModifiedSettings succeeds (revert path). With EnsureActive or EnsurePrimary plus a mode, HDR, or primary change, that earlier step tries to restore a topology containing the missing dock display and returns failure (early return). Recovery never reaches the surviving display or reopened panel. The new tests use a modified state with no such changes, so they do not cover this case.

  2. [P2] Staging breaks surviving clone groups into separate displays. The recovery target preserves groups, but staging appends each display as its own group (staging loop). Path selection then requires a separate source for each group. An adapter may support a cloned pair alongside the stream but lack enough sources to extend both members separately; staging fails even though the grouped replacement is viable.

@jdvmi00

jdvmi00 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Both fixed in f819cd6:

  1. If switching back to the modified topology fails because some of its devices are no longer available, revert now restores the HDR states, modes, and primary only for the devices that remain. Then it continues to restore the initial topology and fall back to recovery. The persisted record is kept intact so unplugged displays can still be fully reverted if they return. Added a regression test with a modified topology of dock + stream display, plus mode/HDR/primary changes.
  2. Staging now appends the target's groups intact rather than one group per device. Added a test where splitting the clone group fails.

Windows run: https://github.com/jdvmi00/libdisplaydevice/actions/runs/36352876700

@ReenigneArcher ReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding regression coverage for the earlier feedback. The updated code handles those tested cases, but two recovery paths still need attention before merge. I left details inline.

Comment thread src/windows/settings_manager_revert.cpp Outdated
Comment thread src/windows/settings_manager_revert.cpp Outdated
@jdvmi00

jdvmi00 commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Both fixed in db3ea02, 727005c and 1be1907:

  1. Pending restores are kept. After a successful recovery, the record now keeps the mode, HDR and primary restores of original displays that are still unplugged, instead of clearing everything. The kept record's initial topology is the original groups plus the recovered ones. Its modified topology holds only the returning devices. Two things changed so the kept record doesn't cause new problems:

    • An apply while the dock is still away carries those entries through, instead of trying to restore a missing device.
    • A revert activates any carried device that is back and postpones the rest.

    Once the dock returns, the next revert restores its settings and clears the record. The record keeps both the dock and the recovered display in its initial topology, so a redock ends with both active. Tests: the redock case in UnpluggedDisplayInModifiedTopologyDoesNotBlockRecovery now reconnects the dock and checks its mode, HDR and primary are restored. PendingSettingsSurviveApplyWhileStillUndocked applies while the dock is absent, then redocks and reverts.

  2. Clone groups with an active member. Staging now extends the active member's group in place. With current [[left],[stream]] and target [[left,right]], it stages [[left,right],[stream]]. A merged group never exceeds two displays, since Windows rejects larger clone groups. Test: StagingKeepsCloneGroupWithActiveMember.

Windows run: https://github.com/jdvmi00/libdisplaydevice/actions/runs/36584993163

@ReenigneArcher ReenigneArcher left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two issues from my previous review are addressed in the updated code and tests, and I resolved those threads. I found one remaining primary-display recovery case inline. Please also fix the Sonar issues reported for this PR.

Comment thread src/windows/settings_manager_apply.cpp Outdated
ReenigneArcher

This comment was marked as resolved.

ReenigneArcher

This comment was marked as resolved.

… unplugged

Keep surviving clone groups intact while staging recovery.
…ups during recovery

Retain the original mode, HDR and primary settings of displays that are still
unplugged after a topology recovery, so redocking can restore them.

Extend an active clone member's group in place while staging recovery instead of
adding the other members as separate sources.
…s while a display is away

Base the retained record on the recovered layout so applying settings still works,
carry the pending entries through an apply, and stage clone groups within the
two-display limit.
… away

Track the display to restore while the original primary is unavailable, so an
EnsurePrimary session can be reverted without leaving its display primary.
@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Sep 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.17687% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.86%. Comparing base (8511c06) to head (f72c62a).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/windows/settings_manager_revert.cpp 90.40% 5 Missing and 14 partials ⚠️
src/windows/win_display_device_general.cpp 57.14% 0 Missing and 3 partials ⚠️
src/windows/settings_manager_apply.cpp 97.05% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
+ Coverage   79.98%   80.86%   +0.88%     
==========================================
  Files          63       63              
  Lines        3412     3680     +268     
  Branches     1550     1694     +144     
==========================================
+ Hits         2729     2976     +247     
+ Misses        333      331       -2     
- Partials      350      373      +23     
Flag Coverage Δ
Linux 93.44% <100.00%> (+0.01%) ⬆️
Windows-AMD64 91.02% <91.63%> (-0.04%) ⬇️
Windows-ARM64 66.27% <58.41%> (-0.82%) ⬇️
macOS 57.97% <64.70%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/common/include/display_device/types.h 100.00% <100.00%> (ø)
src/common/json_serializer.cpp 100.00% <100.00%> (ø)
src/windows/include/display_device/windows/types.h 91.42% <100.00%> (+0.25%) ⬆️
src/windows/json_serializer.cpp 100.00% <100.00%> (ø)
src/windows/settings_utils.cpp 94.64% <100.00%> (+0.14%) ⬆️
src/windows/win_display_device_modes.cpp 99.13% <100.00%> (ø)
src/windows/win_display_device_primary.cpp 100.00% <100.00%> (ø)
src/windows/win_display_device_topology.cpp 98.07% <100.00%> (+0.11%) ⬆️
src/windows/settings_manager_apply.cpp 98.46% <97.05%> (-0.24%) ⬇️
src/windows/win_display_device_general.cpp 95.31% <57.14%> (-1.41%) ⬇️
... and 1 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8511c06...f72c62a. Read the comment docs.

@ReenigneArcher
ReenigneArcher merged commit 5feea6f into LizardByte:master Sep 29, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants