[QuickAccent] Fix UI glitches, DPI-related issues, selection bugs, and add hardware shift key state fallback#46593
[QuickAccent] Fix UI glitches, DPI-related issues, selection bugs, and add hardware shift key state fallback#46593daverayment wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets the Quick Accent (PowerAccent) popup UI reliability by addressing DPI scaling/positioning issues, selection-state glitches, and improving keyboard handling for navigation.
Changes:
- Fix DPI-related sizing/positioning by using monitor DPI and switching popup placement to Win32
SetWindowPoswith raw coordinates. - Reduce selection flashing/glitching via selection reset,
ItemsSourceclearing on hide, and UI layout adjustments (min widths, trimming/wrapping). - Add/adjust native interop usage (CsWin32 update, new
SetWindowPosprojection,GetDpiForMonitor,GetAsyncKeyState) and tweak navigation logic.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/poweraccent/PowerAccent.UI/Selector.xaml.cs | Switches positioning to SetWindowPos, adds DPI/size-change repositioning, and improves selection reset/scroll behavior. |
| src/modules/poweraccent/PowerAccent.UI/Selector.xaml | Adjusts ListBox focus/scroll behavior, sizing (MinWidth), panel type, and description TextBlock trimming/wrapping. |
| src/modules/poweraccent/PowerAccent.UI/PowerAccent.UI.csproj | Adds CsWin32 package reference for UI project Win32 interop. |
| src/modules/poweraccent/PowerAccent.UI/NativeMethods.txt | Declares SetWindowPos for CsWin32 generation in UI project. |
| src/modules/poweraccent/PowerAccent.Core/Tools/WindowsFunctions.cs | Updates text insertion to send in a single SendInput call; uses GetDpiForMonitor and GetAsyncKeyState. |
| src/modules/poweraccent/PowerAccent.Core/PowerAccent.cs | Adjusts selection initialization/navigation (including shift behavior) and updates coordinate/max-width calculations for DPI correctness. |
| src/modules/poweraccent/PowerAccent.Core/NativeMethods.txt | Updates CsWin32 native method list to match new P/Invoke usage. |
| Directory.Packages.props | Bumps Microsoft.Windows.CsWin32 package version repo-wide and applies minor formatting cleanup. |
| shiftPressed = shiftPressed || WindowsFunctions.IsShiftState(); | ||
|
|
||
| if (_visible && _selectedIndex == -1) | ||
| { | ||
| if (triggerKey == TriggerKey.Left) | ||
| if (triggerKey == TriggerKey.Space) | ||
| { | ||
| _selectedIndex = (_characters.Length / 2) - 1; | ||
| _selectedIndex = shiftPressed ? (_characters.Length - 1) : 0; | ||
| } | ||
|
|
||
| if (triggerKey == TriggerKey.Right) | ||
| else if (_settingService.StartSelectionFromTheLeft) | ||
| { | ||
| _selectedIndex = _characters.Length / 2; | ||
| _selectedIndex = 0; |
There was a problem hiding this comment.
shiftPressed = shiftPressed || WindowsFunctions.IsShiftState() will treat any currently-held Shift as “navigate backwards”, including Shift that was already down to type an uppercase letter. This appears to contradict the keyboard hook’s existing behavior of only tracking Shift after the toolbar is visible (to avoid uppercase conflicts), and can cause Space navigation to unexpectedly go backwards when the user triggers Quick Accent while holding Shift for capitalization. Consider tracking the Shift state at toolbar-show time (and only applying the async-key fallback if Shift transitioned after the toolbar became visible), or moving the fallback into the hook where you can distinguish “Shift used for uppercase” vs “Shift used for navigation” reliably.
There was a problem hiding this comment.
This was an excellent catch. I've implemented the suggested logic in commit 59261e0. The new code tracks the initial state of the shift key when the popup is summoned, i.e. the user is typing a capital letter, and checks against this when determining the subsequent shift state in ProcessNextChar, disregarding the current hardware shift state if the initial state was true, i.e. only relying on the keyboard hook in that case. This means we still get the better async shift state handling for lowercase letters and we don't accidentally move backwards when the user is typing a capital letter.
| var hwnd = new System.Windows.Interop.WindowInteropHelper(this).Handle; | ||
| if (hwnd != IntPtr.Zero) | ||
| { | ||
| // Move off-screen to avoid flicker on previous monitor before Show() and | ||
| // UpdateLayout(). | ||
| PInvoke.SetWindowPos((HWND)hwnd, (HWND)IntPtr.Zero, -10000, -10000, 0, 0, WindowPosFlags); | ||
| } | ||
| else | ||
| { | ||
| this.Left = -10000; | ||
| this.Top = -10000; | ||
| } |
There was a problem hiding this comment.
The hard-coded off-screen move to (-10000, -10000) may still land on-screen for large multi-monitor layouts (e.g., several 4K monitors positioned to the left/top can extend beyond -10000 in virtual screen coordinates), which would reintroduce the flicker this is trying to avoid. Consider computing a guaranteed-offscreen position using the virtual screen bounds (e.g., via GetSystemMetrics(SM_XVIRTUALSCREEN/SM_CXVIRTUALSCREEN/SM_YVIRTUALSCREEN/SM_CYVIRTUALSCREEN)) and placing the window just outside that rectangle.
There was a problem hiding this comment.
Good call. Updated the code to render offscreen 1000 pixels top-left of 0,0 in virtual screen space, in commit 59261e0
…ed. Ensure offscreen rendering is actually offscreen by using virtual display metrics.
|
@daverayment Ah, looks like we are partying in the same app :) #46604 replaces the |
|
@niels9001 Ooh, exciting! It looks like the controls that are changing back to pure WPF in #46604 are ones that I've also changed in this PR, so there's likely going to be a conflict whichever way around we do it :) I think that the properties I've added or updated are consistent with regular WPF. It may be better to deal with this PR first before doing the WpfUI->WPF migration. Otherwise, the test document I wrote won't be valid. After the migration to pure WPF, we probably need to ensure the control properties I've changed/added are still present and then run through the tests again. |
Sounds good! Let's get this one in first! |
|
@niels9001 Just in case you were waiting on me, I've made the Copilot-requested changes. |
Summary of the Pull Request
This PR fixes several issues around the popup selection window's size and position, selection-related issues which result in flashing or glitching, and includes more reliable detection of the Shift key.
PR Checklist
Detailed Description of the Pull Request / Additional comments
This PR includes fixes for the Quick Accent's selection window position, its width measurement, and letter selection-related issues. In addition, glitches such as the window flashing the selection colour and the window appearing blank should be reduced or eliminated entirely.
Popup width bug
When opening Quick Accent from a letter with many mappings, it would appear too wide for the display. Even though letters could be selected, they may be entirely off-screen:
This was because of this flaw in
GetDisplayMaxWidth, which is used directly by the popup to set the maximum width of the characters area:GetActiveDisplayuses theGetMonitorInfoAPI, which exposes the working area of the display. It returns its values in raw unscaled pixel values:However, the
MaxWidthproperty must be a pre-scaled value, i.e. in logical WPF units not physical pixels. The fix is straightforward:Popup positioning bug
This is related to a subtle DPI issue in
GetActiveDisplay():Here, the application window's DPI is returned. Unfortunately, the window may report a value which is different from the monitor's own DPI value. This will consistently happen if the application is not Per-Monitor DPI-Aware, and the monitor is not at 100% Scale. The effects are that the Quick Accent popup can appear misaligned or even off-screen entirely. Quick Accent can still be used, but the user may not be able to see what they are selecting.
As Quick Accent is using monitor coordinates for setting its location, the solution is to use the monitor's own DPI value. The fix is to add this in place of the
GetDpiForWindowline:Selection bugs
After dismissing the Quick Accent window, the
_selectedIndexstate was not properly reset. The next time the window opened, it could attempt to scroll to or highlight an index that was out-of-bounds for the new character set. This could result in glitching, such as the window flashing the selection colour or the initial selection being incorrect. In this fix, I:_selectedIndexto-1when the UI hides.SelectedIndexinside Selector.xaml.cs before updating theItemsSource.Shift key activation
In certain cases, a quick press of Shift could fail to move back through the character list. In this fix, I:
ProcessNextChar()to evaluateshiftPressed || WindowsFunctions.IsShiftState().ifs inProcessNextCharto be an if/else structure, to prevent bugs when more than one trigger key is pressed.Support added for multi-codepoint graphemes
The current code loops through each
charof a mapping, callingSendInputmultiple times for multi-char sequences. This will fail for multi-codepoint graphemes, i.e. where the mapping 'letter' is more than one UTF-16 codepoint. Those characters may appear as[]. The amendedInsert()inWindowsFunctionsappends all characters before callingSendInput.Miscelleneous
OnDpiChangedhandler for the Selector control, so changing the DPI of the screen should be picked up automatically. (It's questionable whether this is essential, as the DPI would have to change while the control was displayed, but it's worth having for robustness.)SetWindowPosinstead of setting theLeftandTopof the popup control. Also now initialising the popup offscreen to attempt to reduce flicker and the occurrence of blank window flashes.Focusableproperty of the charactersListBoxtoFalse, to attempt to reduce flicker and the window flashing the selector colour.Widthand addedMinWidthto the letter control in Selector.xaml. This allows for wider letters or longer multi-letter mappings.VirtualizingStackPanelto a regularStackPanel. We do not have mappings with enough entries for a virtual control to be necessary, and using StackPanel seemed to have a positive effect on the appearance of blank window glitches.TextTrimming,TextWrappingandMaxHeightto the unicode descriptionTextBlock. This helps support extremely long unicode descriptions. Again, this will enable us to support longer multi-character mappings in the future.SetWindowPoscall.Validation Steps Performed
See separate doc for full test details:
https://docs.google.com/document/d/19uClcUiv7RUDRlbFhazG-Cmu46oNmrAVoJf9bHSjSJU/edit?usp=sharing