[PowerDisplay] Add linked brightness control - #48207
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
|
@microsoft-github-policy-service agree |
|
@gilnatab Would you mind posting a video/screenshots in the PR description? |
This comment has been minimized.
This comment has been minimized.
@niels9001 Screenshots have been added to the PR description. I also implemented the primary-display read: when linked mode is enabled, the master slider now seeds from the Windows primary display, with a fallback if that cannot be resolved. @moooyo I think there should be no more planned feature work to add from my side for this PR. Does the current scope look ready for review, or is there anything else you would like me to address first? |
|
@gilnatab Awesome! Thank you! I'm wondering, would it make sense to get rid of the info banner and move the message to the tooltip of the link icon? 
My thinking is that we can then ensure that the UI stays clean as possible, especially with multiple monitors? |
@niels9001 Thanks for the suggestion. I understand the goal of keeping the panel compact, but I would prefer to keep the inline brightness row rather than move the explanation entirely into the link icon tooltip. In linked mode, the individual displays section is already collapsed by default, so the primary UI remains compact even with multiple monitors. When a user manually expands that section, I think it is reasonable to assume that they want more information and more granular control. In that context, keeping the layout consistent with the unlinked mode seems preferable: it preserves visual consistency and reduces the need for users to relearn where information has moved. Discoverability is also a concern. Some displays, such as built-in laptop panels, may only support brightness control. If the brightness row is removed in linked mode, the display card may become almost empty. Users may wonder where the brightness control went, and the current state would no longer be immediately visible in a quick-control flyout. A tooltip requires users to already know that the link icon is relevant and hover over it. There is also a possible extensibility concern. Linked mode currently applies only to brightness, so the tooltip would remain short. If we later support linking contrast, volume, or other controls, the tooltip could become increasingly complicated. Keeping the rows visible also leaves room for a future design where individual settings can be excluded from linking independently, rather than excluding an entire display at once. I am not sure users will actually need that level of control, but removing the rows now narrows that option. That said, these are only my thoughts, and I am not a UI designer. If you still prefer the tooltip-only approach after considering the trade-offs, I am happy to follow your direction and update the PR. |
|
code side looks good overall. about UI part, I agree niels idea. I don't think we need a separate info banner. Add some hints into the hover message is good enough. yeah, we currently only support brighness, and future will support more feature. But I don't think we need to support a very detailed control (e.g., a monitor needs to be linked, but its contrast do not). If many user asked this control behaviour, we can consider in the future, but now I think we can just keep it simple. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
And please call this script to fix xaml style issue. C:\a_work\1\s.pipelines\applyXamlStyling.ps1 : XAML Styling is incorrect, please run |
@moooyo Thanks for the feedback. I pushed updates to remove the separate info banner and move the linked-brightness hint into the link icon tooltip. I also fixed the XAML styling/layout issue, simplified the seed logic, and updated the linked brightness debounce handling. Could you please take another look? |
| /// </summary> | ||
| private void SetLinkedBrightnessSilently(int value) | ||
| { | ||
| _suppressLinkedBrightnessBroadcast = true; |
There was a problem hiding this comment.
Why we need _suppressLinkedBrightnessBroadcast ?
There was a problem hiding this comment.
We still need this is to avoid changing hardware brightness during initialization. When linked mode is restored on startup, or when the toggle is turned on, we programmatically set LinkedBrightness only to position the master slider. Without this flag, that setter would trigger OnLinkedBrightnessChanged and schedule a brightness broadcast. The intended behavior is that the first hardware write happens only after the user moves the master slider.
There was a problem hiding this comment.
in which initialization? I mean, if we expect the UI will hang for a period of time, we need to lock the UI rather than add inner logic.
There was a problem hiding this comment.
@moooyo The purpose of _suppressLinkedBrightnessBroadcast is just to avoid treating the initial slider value as a user brightness change.
For example, suppose the laptop panel is at 90 because the room is bright, and an external monitor is at 20 because it is much brighter by default. When linked mode is restored or turned on, we currently seed the master slider from one existing monitor brightness so it has a real starting value. That assignment is only meant to position the slider.
Without the suppress flag, this seed would be handled the same as the user moving the master slider, so enabling linked mode could immediately push both monitors to 90 or both monitors to 20. If the monitors have very different brightness ranges or perceived brightness, that can make one display suddenly too bright or too dark even though the user has not adjusted anything yet.
So the behavior I was trying to preserve is: turning linked mode on should not change monitor brightness by itself; the first broadcast should happen only after the user moves the master slider.
If you think it is OK for enabling/restoring linked mode to immediately apply the seeded value to all linked monitors, I can remove this part.
niels9001
added
Area-Localization
|
@moooyo You're right, I simplified this. The debounce logic was duplicated before. I moved the actual timer/restart code into I also simplified the planner side.
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
4 participants
Summary of the Pull Request
Adds linked brightness control to PowerDisplay so multiple brightness-capable monitors can be controlled from a single "All Displays" slider.
This PR:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Screenshots
The first version is intentionally scoped to brightness-only linked control. Contrast, volume, color temperature, input source, and LightSwitch-specific behavior remain independent.
Linked brightness is stored as global PowerDisplay settings:
linked_levels_activeexcluded_from_sync_monitor_idsNewly connected brightness-capable monitors are included by default, because the exclusion list is the explicit exception. Hotplugging a monitor does not immediately write brightness; linked hardware writes happen only after the user changes the master slider.
Profiles remain per-monitor snapshots. This PR does not add profile-level linked brightness configuration. If linked brightness is active when a profile is applied, linked mode is turned off first, then the saved per-monitor profile values are applied. That avoids leaving the master linked slider active while hardware brightness has been changed independently per monitor.
When linked mode is turned on, the master slider is seeded from the linked brightness-capable display with the lowest Windows DISPLAY number, falling back to monitor ID for determinism. Excluded displays and displays without brightness support are ignored; if no linked target remains, the master slider stays disabled. The seed only positions the slider; it is never written to hardware, so the first user gesture is the first broadcast.
Validation Steps Performed
PowerDisplay.Lib.UnitTestsDebug x64:PowerDisplay.Lib.UnitTestswithvstest.console.exe.\.pipelines\applyXamlStyling.ps1 -Mainsrc/modules/powerdisplay/PowerDisplay/PowerDisplayXAML/MainWindow.xaml.