Skip to content

[PowerDisplay] Confirm before enabling module; log EdidId in Phase 0 #48111

Merged
moooyo merged 11 commits into
mainfrom
yuleng/pd/dialog/1
May 25, 2026
Merged

[PowerDisplay] Confirm before enabling module; log EdidId in Phase 0 #48111
moooyo merged 11 commits into
mainfrom
yuleng/pd/dialog/1

Conversation

@moooyo
Copy link
Copy Markdown
Contributor

@moooyo moooyo commented May 25, 2026

Summary of the Pull Request

Two crash-correlation aids for the kernel-side DDC/CI BSOD mitigated by #47734:

  1. Log EDID hardware ID (manufacturer + product code, e.g. DELD1A8) during Phase 0 monitor classification, before any DDC/CI capability fetch enters the BSOD risk window.
  2. Show a confirmation dialog before turning the Power Display module on from the Settings page, so the user understands the BSOD risk before the first capability fetch runs.

PR Checklist

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end-user-facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

1. Phase 0 EdidId logging

MonitorIdentity.EdidIdFromDevicePath parses the EDID hardware ID segment from a DevicePath of the form \?\DISPLAY#DELD1A8#5&abc&0&UID12345#{guid} and returns DELD1A8. The 3-letter PNP manufacturer code + 4-hex product code is identical for every physical unit of the same model, so it identifies the model without leaking per-unit identifiers.

MonitorManager logs the EdidId on the existing Phase 0 classification line. Phase 0 uses QueryDisplayConfig, which reads OS-cached EDID and cannot BSOD, so this line is guaranteed on disk before the crash-prone Phase 2 capability fetch starts. If a machine crashes during enumeration, the recovered log identifies every attached model (including same-model duplicates), which makes it possible to correlate crash reports to specific monitor models even when the user can't tell us which monitor caused the crash.

2. Enable-module confirmation dialog

PowerDisplayViewModel.IsEnabled setter is refactored to follow the same two-phase pattern already used by MaxCompatibilityMode:

  • false → true does not commit immediately; it kicks off ConfirmAndEnableModuleAsync, which awaits the existing DangerousFeatureWarningDialog (resource prefix PowerDisplay_EnableModule) and either commits or reverts the ToggleSwitch via OnPropertyChanged.
  • true → false commits unconditionally — we never block a user who wants to turn the module off.
  • App-startup loads via InitializeEnabledValue() / RefreshEnabledState() assign the _isEnabled field directly, bypassing the setter, so the dialog never fires on settings restore or GPO refresh.
  • GPO-configured state still short-circuits before any dialog logic.

The dialog reuses the existing DangerousFeatureWarningDialog injected by PowerDisplayPage.xaml.cs. The 5 new PowerDisplay_EnableModule_* strings explain that the BSOD is in Windows (not Power Display), that Power Display will auto-disable itself after a detected crash, and that the user has to re-enable + dismiss the warning each time.

Validation Steps Performed

  • Built src/settings-ui and src/modules/powerdisplay locally.
  • Unit tests: added EdidIdFromDevicePath_* cases to MonitorIdentityTests, all green.
  • Settings UI manual: toggling Power Display ON now shows the warning dialog. Pressing Cancel reverts the ToggleSwitch visually; pressing Enable commits and the module starts. Toggling OFF does not prompt. Restarting Settings UI with PowerDisplay enabled does not prompt. GPO-disabled state still locks the toggle.
  • Log inspection: MonitorManager Phase 0 log now shows EdidId=... for each path before any capability fetch.

🤖 Generated with Claude Code

Yu Leng (from Dev Box) and others added 3 commits May 20, 2026 20:04
Adds two crash-correlation aids for the kernel-side DDC/CI BSOD mitigated by
#47734:

- MonitorIdentity.EdidIdFromDevicePath extracts the EDID hardware ID (e.g.
  DELD1A8) — identical for every monitor of the same model. MonitorManager
  logs it during Phase 0 classification, before any capability fetch enters
  the BSOD risk window, so a recovered log identifies attached models
  (including same-model duplicates).

- The IsEnabled toggle on the PowerDisplay settings page now confirms with
  the user before turning the module on, reusing the existing
  DangerousFeatureWarningDialog under the PowerDisplay_EnableModule resource
  prefix. Disabling stays unconditional. App-startup loads bypass the setter
  via field assignment, so the dialog never fires on restore.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Guard ConfirmAndEnable{Module,MaxCompat}Async with try/catch/finally so a
  ContentDialog.ShowAsync exception logs and still raises OnPropertyChanged,
  preventing the ToggleSwitch from getting stuck in the wrong visual state.
- Delete EdidIdFromDevicePath; route MonitorManager through the existing
  EdidIdFromMonitorId helper (now documents that it accepts either a
  Monitor.Id or a raw DevicePath with trailing "#{guid}").
- Substitute "?" when the EdidId is empty so the Phase 0 log line keeps a
  stable column shape for log scraping.
- Split PowerDisplay_EnableModule_WarningList and the existing
  PowerDisplay_MaxCompatibility_WarningList into three per-bullet resw
  entries; DangerousFeatureWarningDialog renders them as three TextBlocks
  with a code-side "• " prefix so translators only see body text and
  Narrator can navigate between bullets.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@moooyo moooyo changed the title [PowerDisplay] Log EdidId in Phase 0 and warn before enabling module [PowerDisplay] Confirm before enabling module; log EdidId in Phase 0 May 25, 2026
Yu Leng (from Dev Box) and others added 8 commits May 25, 2026 14:58
Replace the three hardcoded WarningListItem1/2/3 TextBlocks with a single
ItemsControl bound to a string list built by iterating _WarningList_Item{N}
until ResourceLoader.GetString returns empty. Adding a 4th bullet now
requires only a new resw entry — no XAML or code change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defensive upper bound on the _WarningList_Item{N} loop in case a future
ResourceLoader change ever returns a non-empty value for a missing key.
A real dialog has 3 bullets, so the cap is never hit in practice.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…et keys

Fix regression introduced when DangerousFeatureWarningDialog switched to
reading _WarningList_Item{N} via the ItemsControl loop: the three
pre-existing dialog callers (color temperature, power state, input source)
still used a single _WarningList resw value, so their bullet list was
rendering empty.

Split each into _WarningList_Item1/2/3 with translator comments, matching
the EnableModule and MaxCompatibility blocks.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop bullets that restate adjacent ones:

- ColorTemperature: merge "Incorrect display colors" + "Display malfunction"
  into "Incorrect display colors or other display malfunction" (the first
  was a subset of the second).
- PowerState: merge the standby symptom and the physical-button recovery
  into one bullet (they describe the same incident).
- EnableModule: merge the auto-disable + manual-re-enable flow into one
  bullet (same recovery path, no value in splitting).

Item3 entries are removed; the dialog's _Item{N} loop now stops at Item3
returning empty (no XAML or code change needed).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull the "⚠️" prefix off the five WarningHeader resw values and prepend it
in DangerousFeatureWarningDialog instead, matching the existing pattern
used for the "•" bullet glyph. Translators now only see body text, and
the glyph lives in one place if it ever needs to change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… comments

Translator-facing <comment> fields should describe *where* a string is
shown, not how the dialog decorates it. The trailing "The dialog prepends
a glyph" / "Rendered with a leading bullet" sentences were leaking
implementation detail into the localization file with no value to the
translator.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Translator-facing <comment> should only add context that the key name and
value can't supply. For the five confirmation dialogs (color temperature,
power state, input source, max compatibility, enable module), every
"<role> of the <feature> warning dialog." comment was restating the
self-documenting key suffix (_WarningTitle / _Header / _Description /
_List_Item{N} / _Confirm). Remove them so the resw stays signal-only.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fix dialog never showing: MRT++ ResourceLoader.GetString throws
"NamedResource Not Found" when the key is absent — not return empty
string like the legacy WinRT loader. The Item{N} probe loop hit that
the moment it walked past the last real bullet, the exception bubbled
through ConfirmAndEnableModuleAsync's try/catch, the dialog never
showed, and the ToggleSwitch snapped back to OFF.

Replace the loop's GetString with ResourceMap.TryGetValue on a direct
handle to the Settings PRI's "Resources" subtree — returns null for
missing keys, no exception, no exception-driven control flow. Other
GetString calls in the constructor stay (those keys must exist; a
miss is a real bug worth surfacing).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@moooyo moooyo enabled auto-merge (squash) May 25, 2026 08:21
@moooyo moooyo added the 0.100 label May 25, 2026
@moooyo moooyo merged commit f175a9c into main May 25, 2026
16 checks passed
@moooyo moooyo deleted the yuleng/pd/dialog/1 branch May 25, 2026 09:08
pull Bot pushed a commit to ScorpiusDraconis83/PowerToys that referenced this pull request May 25, 2026
…icrosoft#48111)

## Summary of the Pull Request

Two crash-correlation aids for the kernel-side DDC/CI BSOD mitigated by
microsoft#47734:

1. Log EDID hardware ID (manufacturer + product code, e.g. `DELD1A8`)
during Phase 0 monitor classification, before any DDC/CI capability
fetch enters the BSOD risk window.
2. Show a confirmation dialog before turning the Power Display module on
from the Settings page, so the user understands the BSOD risk before the
first capability fetch runs.

## PR Checklist

- [ ] Closes: #xxx
- [x] **Communication:** I've discussed this with core contributors
already. If the work hasn't been agreed, this work might be rejected
- [x] **Tests:** Added/updated and all pass
- [x] **Localization:** All end-user-facing strings can be localized
- [ ] **Dev docs:** Added/updated
- [ ] **New binaries:** Added on the required places
- [ ] [JSON for
signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json)
for new binaries
- [ ] [WXS for
installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs)
for new binaries and localization folder
- [ ] [YML for CI
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml)
for new test projects
- [ ] [YML for signed
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml)
- [ ] **Documentation updated:** If checked, please file a pull request
on [our docs
repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys)
and link it here: #xxx

## Detailed Description of the Pull Request / Additional comments

### 1. Phase 0 EdidId logging

`MonitorIdentity.EdidIdFromDevicePath` parses the EDID hardware ID
segment from a DevicePath of the form
``\?\DISPLAY#DELD1A8#5&abc&0&UID12345#{guid}`` and returns ``DELD1A8``.
The 3-letter PNP manufacturer code + 4-hex product code is identical for
every physical unit of the same model, so it identifies the *model*
without leaking per-unit identifiers.

`MonitorManager` logs the EdidId on the existing Phase 0 classification
line. Phase 0 uses `QueryDisplayConfig`, which reads OS-cached EDID and
cannot BSOD, so this line is guaranteed on disk before the crash-prone
Phase 2 capability fetch starts. If a machine crashes during
enumeration, the recovered log identifies every attached model
(including same-model duplicates), which makes it possible to correlate
crash reports to specific monitor models even when the user can't tell
us which monitor caused the crash.

### 2. Enable-module confirmation dialog

`PowerDisplayViewModel.IsEnabled` setter is refactored to follow the
same two-phase pattern already used by `MaxCompatibilityMode`:

- `false → true` does not commit immediately; it kicks off
`ConfirmAndEnableModuleAsync`, which awaits the existing
`DangerousFeatureWarningDialog` (resource prefix
`PowerDisplay_EnableModule`) and either commits or reverts the
ToggleSwitch via `OnPropertyChanged`.
- `true → false` commits unconditionally — we never block a user who
wants to turn the module off.
- App-startup loads via `InitializeEnabledValue()` /
`RefreshEnabledState()` assign the `_isEnabled` field directly,
bypassing the setter, so the dialog never fires on settings restore or
GPO refresh.
- GPO-configured state still short-circuits before any dialog logic.

The dialog reuses the existing `DangerousFeatureWarningDialog` injected
by `PowerDisplayPage.xaml.cs`. The 5 new `PowerDisplay_EnableModule_*`
strings explain that the BSOD is in Windows (not Power Display), that
Power Display will auto-disable itself after a detected crash, and that
the user has to re-enable + dismiss the warning each time.

## Validation Steps Performed

- Built `src/settings-ui` and `src/modules/powerdisplay` locally.
- Unit tests: added `EdidIdFromDevicePath_*` cases to
`MonitorIdentityTests`, all green.
- Settings UI manual: toggling Power Display ON now shows the warning
dialog. Pressing Cancel reverts the ToggleSwitch visually; pressing
Enable commits and the module starts. Toggling OFF does not prompt.
Restarting Settings UI with PowerDisplay enabled does not prompt.
GPO-disabled state still locks the toggle.
- Log inspection: `MonitorManager` Phase 0 log now shows `EdidId=...`
for each path before any capability fetch.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Yu Leng (from Dev Box) <yuleng@microsoft.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants