Render Snapshot to Disk accelerator from the active keyboard layout#5684
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates how menu shortcut (accelerator) key labels are rendered on Windows so that the displayed key matches the active keyboard layout (while keeping the underlying shortcut behavior unchanged), primarily to improve the “Snapshot to Disk” menu accelerator display.
Changes:
- Introduces a new
LLKeyboard::stringFromAcceleratorMenuKey()entry point with a platform override hook (stringFromAcceleratorMenuKeyImpl()). - Implements a Win32-specific menu-key string mapping using the current keyboard layout (
ToUnicodeExpath). - Switches
LLMenuItemGL::appendAcceleratorString()to use the new menu-key string function when building menu accelerator labels.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| indra/llwindow/llkeyboard.h | Adds a dedicated API + virtual hook for “menu accelerator key string” rendering. |
| indra/llwindow/llkeyboard.cpp | Implements the new static API and default virtual implementation. |
| indra/llwindow/llkeyboardwin32.h | Declares the Win32 override for layout-aware menu key labeling. |
| indra/llwindow/llkeyboardwin32.cpp | Implements Win32 layout-aware key-to-string via ToUnicodeEx. |
| indra/llui/llmenugl.cpp | Uses the new menu-key string path when composing accelerator labels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::string LLKeyboard::stringFromAcceleratorMenuKey(KEY key, bool translate) | ||
| { | ||
| if (gKeyboard != NULL) | ||
| { | ||
| return gKeyboard->stringFromAcceleratorMenuKeyImpl(key, translate); | ||
| } | ||
|
|
||
| return stringFromKey(key, translate); | ||
| } |
There was a problem hiding this comment.
LLKeyboard::stringFromAcceleratorMenuKey() checks gKeyboard != NULL, but in the gKeyboard == NULL path it calls stringFromKey(key, translate), which (when translate is true) dereferences gKeyboard->mStringTranslator and can still crash. Consider returning stringFromKey(key, /*translate=*/false) here (or otherwise avoiding any gKeyboard access) so the NULL-guard is actually effective.
| { | ||
| std::string key_string = ll_convert_wide_to_string(std::wstring(chars, 1)); | ||
| if (!key_string.empty()) | ||
| { |
There was a problem hiding this comment.
ToUnicodeEx() is being called here to derive the display label, but keyboard_state is all zeros; for letter keys this typically produces lowercase (e.g. "a") whereas the existing accelerator labels use uppercase ("A"). This will change the appearance of many menu shortcuts on Windows; consider normalizing the result (e.g., uppercasing ASCII a-z) or short-circuiting A–Z to LLKeyboard::stringFromKey() to keep menu labels consistent while still translating OEM keys.
| { | |
| { | |
| if (key_string.size() == 1 && key_string[0] >= 'a' && key_string[0] <= 'z') | |
| { | |
| key_string[0] = static_cast<char>(key_string[0] - 'a' + 'A'); | |
| } |
| int res = ToUnicodeEx( | ||
| os_key, | ||
| scan_code, | ||
| keyboard_state, | ||
| chars, | ||
| 8, | ||
| 1 << 2, | ||
| layout); | ||
|
|
||
| if ((res == 1 || res == -1) && chars[0] >= 0x20) | ||
| { | ||
| std::string key_string = ll_convert_wide_to_string(std::wstring(chars, 1)); | ||
| if (!key_string.empty()) | ||
| { | ||
| if (translate) | ||
| { | ||
| LLKeyStringTranslatorFunc* trans = mStringTranslator; | ||
| if (trans != NULL) | ||
| { | ||
| key_string = trans(key_string); | ||
| } | ||
| } | ||
|
|
||
| return key_string; | ||
| } | ||
| } |
There was a problem hiding this comment.
ToUnicodeEx() can have thread-global side effects related to dead-key and keyboard-buffer state on some Windows versions (see the existing cautionary comment around ToUnicodeEx usage in llviewerwindow.cpp). In particular, treating res == -1 (dead key) as success without clearing the dead-key state risks impacting subsequent character translation/WM_CHAR handling on this UI thread. Consider avoiding the res == -1 path (fall back to the base implementation for dead keys) or explicitly clearing the dead-key state after the call (or using an alternative like MapVirtualKeyEx(..., MAPVK_VK_TO_CHAR, ...) that doesn’t mutate dead-key state).
This keeps the existing
Snapshot to Diskshortcut as-is, but renders its menu accelerator from the active keyboard layout on Windows.Tested on
US,US-International,pt-BR, andGerman.