Make MPO tweak a three-state control - #4897
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe MPO registry toggle is replaced with a three-state combobox. New helpers detect, apply, verify, and roll back registry states. The WPF renderer synchronizes selections and displays state descriptions, warnings, and help links. ChangesMultiplane Overlay state management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WPFComboBox
participant InvokeWPFUIElements
participant GetWinUtilRegistryComboState
participant SetWinUtilRegistryComboState
participant Registry
WPFComboBox->>InvokeWPFUIElements: initialize MPO selection
InvokeWPFUIElements->>GetWinUtilRegistryComboState: resolve current state
GetWinUtilRegistryComboState->>Registry: read OverlayTestMode and DisableOverlays
Registry-->>GetWinUtilRegistryComboState: return registry values
WPFComboBox->>InvokeWPFUIElements: select MPO mode
InvokeWPFUIElements->>SetWinUtilRegistryComboState: apply selected mode
SetWinUtilRegistryComboState->>Registry: write and verify values
SetWinUtilRegistryComboState-->>InvokeWPFUIElements: return success or failure
InvokeWPFUIElements-->>WPFComboBox: update or restore selection
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
pester/multiplane-overlay.Tests.ps1 (2)
12-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTwo important cases have no coverage.
First, the absent-value case. The mock at Line 13 always returns
0for both properties. On a clean machine neither value exists, soGet-ItemPropertyreturns$nulland Lines 7-8 ofGet-WinUtilMultiplaneOverlayStatecoerce$nullto0. That coercion is the most common real path and no test exercises it.Second, the
Enabledstate.Set-WinUtilMultiplaneOverlayhas tests forDisabled (Compatibility)andFully Disabledbut not forEnabled, which is the state that reverses the tweak.💚 Proposed tests
It "reports Enabled when neither registry value exists" { Mock Get-ItemProperty { $null } Get-WinUtilMultiplaneOverlayState | Should -Be "Enabled" }It "writes the enabled values" { $script:registryValues = @{ OverlayTestMode = 5; DisableOverlays = 1 } Set-WinUtilMultiplaneOverlay -State "Enabled" Should -Invoke Set-WinUtilRegistry -Times 1 -Exactly -ParameterFilter { $Name -eq "OverlayTestMode" -and $Value -eq 0 } Should -Invoke Set-WinUtilRegistry -Times 1 -Exactly -ParameterFilter { $Name -eq "DisableOverlays" -and $Value -eq 0 } }Adjust the expected values if you adopt
<RemoveEntry>for theEnabledstate.Also applies to: 72-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pester/multiplane-overlay.Tests.ps1` around lines 12 - 18, Add coverage in Get-WinUtilMultiplaneOverlayState for Get-ItemProperty returning $null, asserting the state is "Enabled". Add a Set-WinUtilMultiplaneOverlay test for "Enabled" that verifies Set-WinUtilRegistry writes OverlayTestMode and DisableOverlays as 0, or verifies the adopted removal behavior.
135-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThese assertions test source text, not behavior.
Lines 141-148 and Lines 156-158 read
.ps1files as raw strings and match regular expressions against them. Three consequences follow:
- A behavior-preserving refactor breaks the tests. For example, renaming the local
$comboBoxvariable breaks Line 143.- The tests pass when the matched text appears only in a comment or a disabled code path.
- Line 148 asserts the absence of
StateFunction|ApplyFunction|IsApplyinganywhere in the renderer. A future unrelated identifier or comment that contains one of those words fails the test with no useful message.The configuration assertions at Lines 137-140 are sound, because the JSON contract is the thing under test. For the renderer wiring, prefer a behavior test: build a real
Windows.Controls.ComboBox, callSync-WPFMultiplaneOverlayStatewith a mockedGet-WinUtilMultiplaneOverlayState, then assertSelectedItem.Content,Items.Count, andToolTip. That test covers the unknown-state recovery path from the PR objectives directly, which the current string match at Line 156 does not.Do you want me to draft the behavior-level replacements for these two
Describeblocks?Also applies to: 152-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pester/multiplane-overlay.Tests.ps1` around lines 135 - 149, Replace the renderer source-text assertions in the affected Describe blocks with a behavior-level test of Sync-WPFMultiplaneOverlayState. Instantiate a real Windows.Controls.ComboBox, mock Get-WinUtilMultiplaneOverlayState, invoke the function, and assert SelectedItem.Content, Items.Count, and ToolTip, including unknown-state recovery; retain the valid configuration assertions and remove brittle regex checks for implementation names or text.functions/private/Set-WinUtilMultiplaneOverlay.ps1 (2)
9-12: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Enabledwrites zeros instead of restoring the Windows default absence of the values.The linked issue defines
Enabledas "Restore both registry values to their defaults". The Windows default is that neither value exists. This code writesOverlayTestMode=0andDisableOverlays=0, so it leaves two residual DWORDs on machines that never had them.Get-WinUtilMultiplaneOverlayStatemaps0,0toEnabled, so the UI stays correct, but the original state is not restored.
Set-WinUtilRegistryalready supports<RemoveEntry>, and the verification at Line 36 casts a missing value to0, so removal still verifies correctly.♻️ Proposed refactor
"Enabled" { - # Zero is Windows' default for both values, leaving MPO enabled. - @{ OverlayTestMode = 0; DisableOverlays = 0 } + # Windows' default is the absence of both values, leaving MPO enabled. + @{ OverlayTestMode = "<RemoveEntry>"; DisableOverlays = "<RemoveEntry>" } }This change also requires the verification at Line 36 to compare against the expected effective value, for example by mapping
<RemoveEntry>to0before the comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/private/Set-WinUtilMultiplaneOverlay.ps1` around lines 9 - 12, Update the Enabled branch of Set-WinUtilMultiplaneOverlay to return removal markers for both registry values instead of writing zeros, restoring the Windows default absence. Adjust the verification logic to map each RemoveEntry marker to effective value 0 before comparing, while preserving existing verification for explicitly written values.
39-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd log entries for the verification failure and the rollback outcome.
Set-WinUtilRegistrylogs each individual write, but this catch block records no context. When a user reports that MPO did not apply, the session log shows the writes and no indication that verification failed or that a rollback ran. This operation modifies HKLM, so the failure path should be traceable.♻️ Proposed refactor
} catch { $applyError = $_.Exception.Message + Write-WinUtilLog -Level "ERROR" -Component "MultiplaneOverlay" -Message "Failed to apply state '$State': $applyError Rolling back." $overlayRollbackValue = if ($overlayTestModeExisted) { [int]$previousOverlayTestMode } else { "<RemoveEntry>" }if (-not $overlayRestored -or -not $disableOverlaysRestored) { + Write-WinUtilLog -Level "ERROR" -Component "MultiplaneOverlay" -Message "Rollback of Multiplane Overlay values failed." throw "Unable to apply Multiplane Overlay state '$State': $applyError Rollback also failed." } + + Write-WinUtilLog -Component "MultiplaneOverlay" -Message "Rolled back Multiplane Overlay values after a failed apply."As per coding guidelines: "Preserve logging and user-feedback patterns for long-running or destructive operations."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/private/Set-WinUtilMultiplaneOverlay.ps1` around lines 39 - 57, Add logging in the catch block of Set-WinUtilMultiplaneOverlay for the verification failure and rollback result: record that verification failed and rollback was attempted, then log whether both registry values were successfully restored or rollback failed, including the relevant error context before throwing.Source: Coding guidelines
functions/public/Invoke-WPFUIElements.ps1 (2)
266-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the per-item description lookup.
$entryInfo.ComboDescriptions.PSObject.Properties[$comboitem].Valueworks, because thePSMemberInfoCollectionindexer returns$nullfor a missing name. The direct member access$entryInfo.ComboDescriptions.$comboitemis shorter and reads the same way for names that contain spaces.Note that
pester/multiplane-overlay.Tests.ps1Line 145 asserts this exact expression as a literal string. Update that assertion if you change this line.♻️ Proposed refactor
if ($entryInfo.ComboDescriptions) { - $comboDescription = $entryInfo.ComboDescriptions.PSObject.Properties[$comboitem].Value + $comboDescription = $entryInfo.ComboDescriptions.$comboitem if ($comboDescription) { $comboBoxItem.ToolTip = $comboDescription } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/public/Invoke-WPFUIElements.ps1` around lines 266 - 280, In the ComboBox item construction loop, replace the PSObject.Properties-based lookup in the ComboDescriptions handling with direct dynamic member access using $comboitem, while preserving the existing tooltip assignment and missing-description behavior. Update the literal-expression assertion in multiplane-overlay.Tests.ps1 to match the revised lookup.
335-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
(?)link block duplicates the block in thedefaultbranch.Lines 336-352 repeat the link text block that Lines 463-498 already build for check boxes: same
Namesuffix, sameText, sameToolTip, sameStyle, sameAdd_MouseUpbody, same$syncregistration. The only differences are the omitted margin script, which the horizontalStackPanelmakes unnecessary, and theTagtarget.Extract a small helper, for example
New-WPFLinkTextBlock -Owner $comboBox -Link $entryInfo.Link, and call it from both branches. Two further points to consider while doing so:
- The condition hard-codes
WPFMultiplaneOverlay. Any combo box entry with alinkwould benefit from the same affordance, soif ($entryInfo.Link)alone is sufficient.Start-Process $Sender.ToolTip -ErrorAction Stopat Line 348 throws inside an event handler with nocatch. The copy in thedefaultbranch has the same problem, so a shared helper fixes both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/public/Invoke-WPFUIElements.ps1` around lines 335 - 353, The duplicated link TextBlock construction should be centralized and reused. Add a small helper such as New-WPFLinkTextBlock accepting the owner control and link, move the shared properties, sync registration, and MouseUp handling into it, and add appropriate error handling for Start-Process failures; call it from both the WPFMultiplaneOverlay path and the default checkbox path. Replace the hard-coded entry-name check with a link-presence check while preserving the existing StackPanel layout and owner Tag behavior.functions/private/Get-WinUtilMultiplaneOverlayState.ps1 (1)
5-8: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding the
[int]casts against non-numeric registry values.If
OverlayTestModeorDisableOverlaysexists with a non-numeric type, for exampleREG_SZwith text orREG_BINARY, the[int]cast throws a conversion error. The caller then shows a cast error message instead of the intended "Unexpected Multiplane Overlay registry state" message. ATryParse-style guard keeps the message actionable.♻️ Proposed refactor
- $overlayTestMode = if ($null -eq $overlayTestMode) { 0 } else { [int]$overlayTestMode } - $disableOverlays = if ($null -eq $disableOverlays) { 0 } else { [int]$disableOverlays } + $parsedOverlayTestMode = 0 + $parsedDisableOverlays = 0 + if ($null -ne $overlayTestMode -and -not [int]::TryParse([string]$overlayTestMode, [ref]$parsedOverlayTestMode)) { + throw "Unexpected Multiplane Overlay registry state: OverlayTestMode is not a number." + } + if ($null -ne $disableOverlays -and -not [int]::TryParse([string]$disableOverlays, [ref]$parsedDisableOverlays)) { + throw "Unexpected Multiplane Overlay registry state: DisableOverlays is not a number." + } + $overlayTestMode = $parsedOverlayTestMode + $disableOverlays = $parsedDisableOverlays🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/private/Get-WinUtilMultiplaneOverlayState.ps1` around lines 5 - 8, Guard the conversions in Get-WinUtilMultiplaneOverlayState for both OverlayTestMode and DisableOverlays using a TryParse-style numeric validation, while retaining 0 for missing values. Ensure non-numeric registry values do not throw during casting and instead flow through to the existing “Unexpected Multiplane Overlay registry state” handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/tweaks.json`:
- Line 1465: The Description value for the Multiplane Overlay preference uses
incorrect subject-verb agreement; change “Multiplane Overlay compose” to
“Multiplane Overlay composes” while preserving the rest of the user-facing text.
- Around line 1463-1478: Regenerate Multiplane Overlay documentation in
MultiplaneOverlay.mdx to reference the WPFMultiplaneOverlay preference and its
current Combobox schema, replacing the obsolete WPFToggleMultiplaneOverlay and
Toggle references. Ensure the documented options and behavior match the
ComboItems and ComboDescriptions defined for WPFMultiplaneOverlay.
In `@pester/multiplane-overlay.Tests.ps1`:
- Around line 1-9: Declare the required Pester version at the top of the test
file by adding the Pester module requirement for version 5.8.0 before the
existing BeforeAll block, ensuring local runs do not select older Pester
versions.
---
Nitpick comments:
In `@functions/private/Get-WinUtilMultiplaneOverlayState.ps1`:
- Around line 5-8: Guard the conversions in Get-WinUtilMultiplaneOverlayState
for both OverlayTestMode and DisableOverlays using a TryParse-style numeric
validation, while retaining 0 for missing values. Ensure non-numeric registry
values do not throw during casting and instead flow through to the existing
“Unexpected Multiplane Overlay registry state” handling.
In `@functions/private/Set-WinUtilMultiplaneOverlay.ps1`:
- Around line 9-12: Update the Enabled branch of Set-WinUtilMultiplaneOverlay to
return removal markers for both registry values instead of writing zeros,
restoring the Windows default absence. Adjust the verification logic to map each
RemoveEntry marker to effective value 0 before comparing, while preserving
existing verification for explicitly written values.
- Around line 39-57: Add logging in the catch block of
Set-WinUtilMultiplaneOverlay for the verification failure and rollback result:
record that verification failed and rollback was attempted, then log whether
both registry values were successfully restored or rollback failed, including
the relevant error context before throwing.
In `@functions/public/Invoke-WPFUIElements.ps1`:
- Around line 266-280: In the ComboBox item construction loop, replace the
PSObject.Properties-based lookup in the ComboDescriptions handling with direct
dynamic member access using $comboitem, while preserving the existing tooltip
assignment and missing-description behavior. Update the literal-expression
assertion in multiplane-overlay.Tests.ps1 to match the revised lookup.
- Around line 335-353: The duplicated link TextBlock construction should be
centralized and reused. Add a small helper such as New-WPFLinkTextBlock
accepting the owner control and link, move the shared properties, sync
registration, and MouseUp handling into it, and add appropriate error handling
for Start-Process failures; call it from both the WPFMultiplaneOverlay path and
the default checkbox path. Replace the hard-coded entry-name check with a
link-presence check while preserving the existing StackPanel layout and owner
Tag behavior.
In `@pester/multiplane-overlay.Tests.ps1`:
- Around line 12-18: Add coverage in Get-WinUtilMultiplaneOverlayState for
Get-ItemProperty returning $null, asserting the state is "Enabled". Add a
Set-WinUtilMultiplaneOverlay test for "Enabled" that verifies
Set-WinUtilRegistry writes OverlayTestMode and DisableOverlays as 0, or verifies
the adopted removal behavior.
- Around line 135-149: Replace the renderer source-text assertions in the
affected Describe blocks with a behavior-level test of
Sync-WPFMultiplaneOverlayState. Instantiate a real Windows.Controls.ComboBox,
mock Get-WinUtilMultiplaneOverlayState, invoke the function, and assert
SelectedItem.Content, Items.Count, and ToolTip, including unknown-state
recovery; retain the valid configuration assertions and remove brittle regex
checks for implementation names or text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f8b5ea7-7661-45d1-a188-e8c0f0b68528
📒 Files selected for processing (6)
config/tweaks.jsonfunctions/private/Get-WinUtilMultiplaneOverlayState.ps1functions/private/Set-WinUtilMultiplaneOverlay.ps1functions/private/Sync-WPFMultiplaneOverlayState.ps1functions/public/Invoke-WPFUIElements.ps1pester/multiplane-overlay.Tests.ps1
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64fc4788f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Looking over this I don't like that we are moving the tweak from the json to a ps1 private function. I'm going to have to rework this. |
I agree, this is completely fair and viable. The registry mappings should remain in tweaks and then PowerShell should only provide a generic stateful combobox engine. I'm pushing the refactor. |
coderabbitai
Bot
added
new feature
|
@ChrisTitusTech I updated the MPO implementation to be config-driven, now it follows the same behavior as other tweaks and the documentation clearly displays the different states. 
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/src/content/docs/code-reference/architecture.mdx (1)
392-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the complete
Valuescontract.Lines 392-393 do not state that
Valuesmust map everyComboItemsstate. They also do not document that<RemoveEntry>removes the registry value. The configuration test enforces matching state names, andSet-WinUtilRegistryComboStatedepends on the removal marker. Add these rules to prevent invalid registry combobox configuration.As per coding guidelines, “For architecture, build, schema, or workflow changes, update hand-written developer documentation and keep sidebar slugs synchronized.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/src/content/docs/code-reference/architecture.mdx` around lines 392 - 393, Update the registry-backed combobox documentation around registry[].Values to state that it must define every ComboItems state with matching state names, and document that the <RemoveEntry> marker causes Set-WinUtilRegistryComboState to remove the registry value. Keep the existing DefaultValue description and synchronize any affected architecture sidebar slug if documentation navigation changes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/src/content/docs/code-reference/architecture.mdx`:
- Around line 392-393: Update the registry-backed combobox documentation around
registry[].Values to state that it must define every ComboItems state with
matching state names, and document that the <RemoveEntry> marker causes
Set-WinUtilRegistryComboState to remove the registry value. Keep the existing
DefaultValue description and synchronize any affected architecture sidebar slug
if documentation navigation changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2b924c9-7714-4632-9bdc-2ef1ceb7cd1e
📒 Files selected for processing (12)
config/tweaks.jsondocs/src/content/docs/code-reference/architecture.mdxfunctions/private/Get-WinUtilRegistryComboState.ps1functions/private/Get-WinUtilRegistryComboValue.ps1functions/private/Invoke-WinUtilCurrentSystem.ps1functions/private/Invoke-WinUtilTweaks.ps1functions/private/Set-WinUtilRegistryComboState.ps1functions/public/Invoke-WPFUIElements.ps1pester/configs.Tests.ps1pester/multiplane-overlay.Tests.ps1pester/system-helpers.Tests.ps1pester/tweaks.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (2)
- config/tweaks.json
- functions/public/Invoke-WPFUIElements.ps1
2 participants
Type of Change
Description
Updates the Multiplane Overlay tweak based on issue #4896.
The previous toggle is now a three-state dropdown:
The current state is detected from the registry, and changes are applied immediately when the selection changes (same behavior from the entire Customize Preferences section).
Unexpected registry combinations are shown as
Custom / Unknown - select a state, with an actionable tooltip. Selecting a supported state replaces the custom values. Failed updates are verified and rolled back when possible.Validation completed:
Issue related to PR