chore: refactor and add localization to Accelerator Service #680

United600 · 2025-09-24T20:50:22Z

Improves the AcceleratorService to support localization. Additionally, it improves device information and globalization helpers to use static readonly properties for better performance and clarity.

Moves AcceleratorService from the Controls folder to the Extensions folder for better code organization
Fixes the Tooltip creation logic for KeyboardAccelerator when using multiple modifiers
Adds new strings for keys and modifiers in KeyboardResources.resw
Adds KeyboardAccelerator localization support
Improves Xbox support by adding detection for physical keyboards

P.S. I'm still unsure whether we should push the VirtualKey/VirtualKeyModifiers strings to Crowdin. We could create a separate file for them.

I've built a application to quickly verify, and export, their localized values.

Gravacao.2025-09-24.214732.mp4

…tiple modifiers add physical keyboards detection convert IsDesktop and IsXbox to readonly fields add localized strings for keys and modifiers

add touch present property

Copilot

Pull Request Overview

This PR refactors and adds localization support to the AcceleratorService to enable proper keyboard accelerator display across different languages and locales. It moves the service to a more appropriate namespace, improves device detection capabilities, and adds comprehensive keyboard and modifier key localization.

Moves AcceleratorService from Controls to Extensions folder for better organization
Adds comprehensive localization support for all VirtualKey and VirtualKeyModifiers values
Improves Xbox device detection and adds keyboard/touch presence detection
Converts helper properties to static readonly for better performance

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`Screenbox/Strings/en-US/KeyboardResources.resw`	Adds localized strings for all keyboard keys and modifiers
`Screenbox/Screenbox.csproj`	Moves AcceleratorService file location and adds new KeyboardAcceleratorHelper
`Screenbox/Pages/MainPage.xaml`	Updates namespace references from controls to extensions
`Screenbox/Helpers/KeyboardAcceleratorHelper.cs`	New helper class for converting keyboard accelerators to localized display names
`Screenbox/Helpers/GlobalizationHelper.cs`	Converts property to static readonly for performance
`Screenbox/Helpers/DeviceInfoHelper.cs`	Adds keyboard/touch detection and improves Xbox detection
`Screenbox/Extensions/AcceleratorService.cs`	Refactored service with proper localization and device detection
Multiple XAML files	Updates namespace references from controls to extensions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-24T20:50:51Z

Screenbox/Helpers/DeviceInfoHelper.cs

    /// </summary>
    /// <returns><see langword="true"/> if the device is an Xbox; otherwise, <see langword="false"/>.</returns>
-    public static bool IsXbox => DeviceFamily == "Windows.Xbox";
+    public static readonly bool IsXbox = (DeviceFamily == "Windows.Xbox") || (DeviceFamily == "Windows.XBoxSRA") || (DeviceFamily == "Windows.XBoxERA");


The device family names contain inconsistent capitalization. 'XBoxSRA' and 'XBoxERA' should likely be 'XboxSRA' and 'XboxERA' to match the standard Xbox naming convention used in 'Windows.Xbox'.

Suggested change

public static readonly bool IsXbox = (DeviceFamily == "Windows.Xbox") || (DeviceFamily == "Windows.XBoxSRA") || (DeviceFamily == "Windows.XBoxERA");

public static readonly bool IsXbox = (DeviceFamily == "Windows.Xbox") || (DeviceFamily == "Windows.XboxSRA") || (DeviceFamily == "Windows.XboxERA");

I took the values directly from the Microsoft UI Xaml repository:

https://github.com/microsoft/microsoft-ui-xaml/blob/232a16e5ddfc22c9a1b79a2c51abeb9a39a94494/dev/dll/SharedHelpers.cpp#L370

Screenbox/Extensions/AcceleratorService.cs

move keyboard accelerator display name logic to a new helper file

huynhsontung

This is impressive! Thank you!

huynhsontung · 2025-09-27T07:59:04Z

P.S. I'm still unsure whether we should push the VirtualKey/VirtualKeyModifiers strings to Crowdin. We could create a separate file for them.

What's the concern of pushing VirtualKey strings to Crowdin? Is it because they don't change across languages?

United600 · 2025-09-27T08:11:16Z

P.S. I'm still unsure whether we should push the VirtualKey/VirtualKeyModifiers strings to Crowdin. We could create a separate file for them.

What's the concern of pushing VirtualKey strings to Crowdin? Is it because they don't change across languages?

Some keys don't change, but my main concern is that since they're on Crowdin we can’t validate or lock them. This risks inconsistencies, and translators don’t have straightforward access to all the key translations (not unless I release the above app).

United600 · 2025-09-27T08:26:27Z

Also I probably shouldn't have moved the keyboard accelerator localization logic out of theGlobalizationHelper file

huynhsontung · 2025-09-27T20:02:15Z

Some keys don't change, but my main concern is that since they're on Crowdin we can’t validate or lock them. This risks inconsistencies, and translators don’t have straightforward access to all the key translations (not unless I release the above app).

I see. I agree with this comment. Let's move these strings on another file. Also release the above app! It looks useful!

United600 · 2025-09-27T20:09:58Z

I see. I agree with this comment. Let's move these strings on another file. Also release the above app! It looks useful!

At this stage, it might be best to remove KeyboardResources.resw from Crowdin to avoid potential complications. Even more so once we add localization support for KeyboardAccelerators.

I can upload the repository, though I don't believe packaging it is necessary.

huynhsontung · 2025-09-29T04:55:10Z

I just removed KeyboardResources.resw from Crowndin syncing list. We can safely remove all the translated KeyboardResources.resw versions in our repo. That should be in a separate PR, though.

United600 · 2025-09-29T12:35:25Z

I just removed KeyboardResources.resw from Crowndin syncing list. We can safely remove all the translated KeyboardResources.resw versions in our repo. That should be in a separate PR, though.

Should I translate all of them as part of this PR? Shouldn't take too long.

huynhsontung · 2025-09-29T18:10:56Z

I was thinking you could remove all the translated KeyboardResources.resw files except the English one.

United600 · 2025-09-29T20:26:25Z

I was thinking you could remove all the translated KeyboardResources.resw files except the English one.

Sure, but eventually I'd like to translate them, at least for the most widely spoken languages.

United600 · 2025-10-02T16:31:04Z

If we don't translate the resources, the tooltips generated by the AcceleratorService will differ from those created by the system.

Current/without translations	After/with translations

fix zh-Hant KeyboardResources file inclusion

United600 added 3 commits August 17, 2025 14:56

add localization support to accelerator service and minor fix for mul…

42b8802

…tiple modifiers add physical keyboards detection convert IsDesktop and IsXbox to readonly fields add localized strings for keys and modifiers

replace keyboard accelerator dynamic list with array

60a1798

refactor keyboard accelerator localization logic

f6553ca

add touch present property

United600 requested a review from Copilot September 24, 2025 20:50

United600 added the a11y Issues or improvements related to inclusive design or assistive technologies label Sep 24, 2025

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. l10n Localization xbox Issues or improvements related to Xbox platform labels Sep 24, 2025

Copilot AI reviewed Sep 24, 2025

View reviewed changes

move accelerator service to the extensions folder

44eb283

move keyboard accelerator display name logic to a new helper file

United600 force-pushed the United600/accelerator-service branch from 2e088eb to 44eb283 Compare September 24, 2025 20:58

huynhsontung approved these changes Sep 27, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 27, 2025

add keyboard accelerator translations for multiple locales

fa7294a

fix zh-Hant KeyboardResources file inclusion

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Oct 2, 2025

United600 added 3 commits October 3, 2025 15:07

remove localized string resources for numeric keys (0-9)

a3ca9a8

improve accelerator tooltip formatting for right-to-left languages

e026a81

improve accelerator service comment clarity

f4649cb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

chore: refactor and add localization to Accelerator Service #680

chore: refactor and add localization to Accelerator Service #680

United600 commented Sep 24, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 24, 2025

Uh oh!

United600 Sep 24, 2025

Uh oh!

Uh oh!

huynhsontung left a comment

Uh oh!

huynhsontung commented Sep 27, 2025

Uh oh!

United600 commented Sep 27, 2025 •

edited

Loading

Uh oh!

United600 commented Sep 27, 2025

Uh oh!

huynhsontung commented Sep 27, 2025

Uh oh!

United600 commented Sep 27, 2025

Uh oh!

huynhsontung commented Sep 29, 2025

Uh oh!

United600 commented Sep 29, 2025

Uh oh!

huynhsontung commented Sep 29, 2025

Uh oh!

United600 commented Sep 29, 2025

Uh oh!

United600 commented Oct 2, 2025

Uh oh!

Uh oh!

	public static readonly bool IsXbox = (DeviceFamily == "Windows.Xbox") \|\| (DeviceFamily == "Windows.XBoxSRA") \|\| (DeviceFamily == "Windows.XBoxERA");
	public static readonly bool IsXbox = (DeviceFamily == "Windows.Xbox") \|\| (DeviceFamily == "Windows.XboxSRA") \|\| (DeviceFamily == "Windows.XboxERA");

Uh oh!

chore: refactor and add localization to Accelerator Service #680

Are you sure you want to change the base?

chore: refactor and add localization to Accelerator Service #680

Conversation

United600 commented Sep 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

United600 Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huynhsontung left a comment

Choose a reason for hiding this comment

Uh oh!

huynhsontung commented Sep 27, 2025

Uh oh!

United600 commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

United600 commented Sep 27, 2025

Uh oh!

huynhsontung commented Sep 27, 2025

Uh oh!

United600 commented Sep 27, 2025

Uh oh!

huynhsontung commented Sep 29, 2025

Uh oh!

United600 commented Sep 29, 2025

Uh oh!

huynhsontung commented Sep 29, 2025

Uh oh!

United600 commented Sep 29, 2025

Uh oh!

United600 commented Oct 2, 2025

Uh oh!

Uh oh!

United600 commented Sep 27, 2025 •

edited

Loading