Changeset View
Standalone View
ghost/intern/GHOST_SystemWin32.cpp
| Context not available. | |||||
| typedef BOOL(API *GHOST_WIN32_EnableNonClientDpiScaling)(HWND); | typedef BOOL(API *GHOST_WIN32_EnableNonClientDpiScaling)(HWND); | ||||
| GHOST_SystemWin32::GHOST_SystemWin32() | GHOST_SystemWin32::GHOST_SystemWin32() | ||||
| : m_hasPerformanceCounter(false), m_freq(0), m_start(0), m_lfstart(0) | : m_keycode_last_repeat_key(0), m_lowSurrogate(0), | ||||
| m_hasPerformanceCounter(false), m_freq(0), m_start(0), m_lfstart(0) | |||||
| { | { | ||||
| m_displayManager = new GHOST_DisplayManagerWin32(); | m_displayManager = new GHOST_DisplayManagerWin32(); | ||||
| GHOST_ASSERT(m_displayManager, "GHOST_SystemWin32::GHOST_SystemWin32(): m_displayManager==0\n"); | GHOST_ASSERT(m_displayManager, "GHOST_SystemWin32::GHOST_SystemWin32(): m_displayManager==0\n"); | ||||
nicholas_rishel: No longer ignored, needs explanation. | |||||
Done Inline Actions
nicholas_rishel: > No longer ignored, needs explanation.
| |||||
Done Inline ActionsShould these be unsigned shorts? nicholas_rishel: Should these be unsigned shorts? | |||||
Done Inline ActionsTo clarify this question: I'd appreciate if you could review when types are being converted to types of different length or signed/unsigned and either explain why it was intentional (in inline comments in this issue), or rewrite them to be consistent and explain what was changed and for what reasons. Right now there are a few implicit conversions between different sized types which may be perfectly fine, but reviewing each takes time when I suspect the type conversion are unnecessary and would be made consistent after you go through them. nicholas_rishel: To clarify this question: I'd appreciate if you could review when types are being converted to… | |||||
Done Inline ActionsYou are right, going from unsigned short to int and back to unsigned short is unnecessary. I overlooked those. samhocevar: You are right, going from `unsigned short` to `int` and back to `unsigned short` is unnecessary. | |||||
Not Done Inline ActionsFor me that description needs a little more clarification.
(I'm not sure who maintains this area, so whoever is going to revise this patch may have to study it first). mano-wii: For me that description needs a little more clarification.
- Where are these messages ignored? | |||||
Done Inline ActionsI removed mention of the “hardware keys”. Do you think the new wording in the comment clarifies what happens, or does it need more explanations? I could add more details about where the messages are ignored (in the large switch/case around line 1600) but that information was already missing before my patch. I believe a seasoned Windows programmer will immediately know where to look for when reading about WM_-something being ignored. samhocevar: I removed mention of the “hardware keys”. Do you think the new wording in the comment clarifies… | |||||
Not Done Inline ActionsJust to be sure, MapVirtualKeyW does not return the expected value if vk is VK_PACKET? mano-wii: Just to be sure, `MapVirtualKeyW` does not return the expected value if `vk` is `VK_PACKET`? | |||||
Done Inline ActionsIndeed, it will return 0. That function relies on the current keyboard layout (this is not documented but can be confirmed experimentally and is a well known fact), not the current input state (contrary to ToUnicodeEx which is used later in the function), so it cannot map VK_PACKET to multiple different Unicode characters. samhocevar: Indeed, it will return `0`. That function relies on the current keyboard layout (this is not… | |||||
Not Done Inline Actions@Germano Cavalcante (mano-wii) VK_PACKET indicates the message contains a Unicode character but doesn't embed what that Unicode character is, whereas most (all?) other virtual keys meaningfully map to a single character for a given keyboard layout. @Sam Hocevar (samhocevar) I'd suggest reordering this to MapVirtualKeyW(vk, MAPVK_VK_TO_CHAR) != 0 || vk == VK_PACKET as the former is the common case. nicholas_rishel: @mano-wii VK_PACKET indicates the message contains a Unicode character but doesn't embed what… | |||||
| Context not available. | |||||
| case WM_KEYUP: | case WM_KEYUP: | ||||
| case WM_SYSKEYUP: | case WM_SYSKEYUP: | ||||
| /* These functions were replaced by WM_INPUT*/ | /* These functions were replaced by WM_INPUT*/ | ||||
| case WM_CHAR: | |||||
| /* The WM_CHAR message is posted to the window with the keyboard focus when | |||||
| * a WM_KEYDOWN message is translated by the TranslateMessage function. WM_CHAR | |||||
| * contains the character code of the key that was pressed. | |||||
| */ | |||||
| case WM_DEADCHAR: | case WM_DEADCHAR: | ||||
| /* The WM_DEADCHAR message is posted to the window with the keyboard focus when a | /* The WM_DEADCHAR message is posted to the window with the keyboard focus when a | ||||
| * WM_KEYUP message is translated by the TranslateMessage function. WM_DEADCHAR | * WM_KEYUP message is translated by the TranslateMessage function. WM_DEADCHAR | ||||
| Context not available. | |||||
| * then typing the O key. | * then typing the O key. | ||||
| */ | */ | ||||
| break; | break; | ||||
| case WM_CHAR: | |||||
Done Inline ActionsCould you link me to the MSDN which explains this shift? From https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-keydown it looks like only bits 16-23 are valid for the scan code. nicholas_rishel: Could you link me to the MSDN which explains this shift? From https://docs.microsoft.com/en… | |||||
Done Inline ActionsYou’re right, this isn’t good. I got confused by GTK+ which does the same, I guess they’re looking for trouble, too. Fixed. samhocevar: You’re right, this isn’t good. I got confused by GTK+ which does the same, I guess they’re… | |||||
| /* The WM_CHAR message is posted to the window with the keyboard focus when | |||||
| * a WM_KEYDOWN message is translated by the TranslateMessage function. wParam | |||||
Not Done Inline Actionshttps://docs.microsoft.com/en-us/windows/win32/inputdev/virtual-key-codes
Do you have an intuition for what this means? I'm guessing it just means the virtual key is the low word of wParam in keydown/up, which is true for all virtual keys? nicholas_rishel: https://docs.microsoft.com/en-us/windows/win32/inputdev/virtual-key-codes
> The VK_PACKET key… | |||||
| * contains the character code of the key that was pressed. | |||||
Done Inline ActionsThese should be moved out of the Keyboard events, ignored commented section. nicholas_rishel: These should be moved out of the `Keyboard events, ignored` commented section. | |||||
| */ | |||||
| if ((lParam & 0xff0000) == 0) { | |||||
| /* If the scancode associated with the character is 0, it means the character | |||||
| * was not emitted by the keyboard layer. It most likely comes from some 3rd-party | |||||
| * input software (AutoHotKey, WinCompose) and will not be received by the WM_INPUT | |||||
| * handler, so it is best to handle it here. | |||||
Done Inline ActionsFind and replace error? nicholas_rishel: Find and replace error? | |||||
Done Inline ActionsI just thought the sentence wasn’t very clear here. MSDN says both “The WM_CHAR message contains the character code of the key” and “wParam — The character code of the key” and I thought stating the first part without clarifying with the second part was omitting valuable information. samhocevar: I just thought the sentence wasn’t very clear here. [MSDN](https://docs.microsoft.com/en… | |||||
| */ | |||||
| if ((wParam & 0xfc00) == 0xd800) { | |||||
| // Low surrogate character; store it and do nothing until the other half arrives | |||||
| system->m_lowSurrogate == wParam; | |||||
| } else { | |||||
| wchar_t utf16[3] = {0}; | |||||
| char utf8_char[6] = {0}; | |||||
| char ascii = 0; | |||||
| if ((wParam & 0xfc00) == 0xdc00) { | |||||
| // High surrogate character; combine it with the stored low surrogate | |||||
| utf16[0] = system->m_lowSurrogate; | |||||
| utf16[1] = wParam; | |||||
| } else { | |||||
| utf16[0] = wParam; | |||||
| } | |||||
| system->m_lowSurrogate = 0; | |||||
| conv_utf_16_to_8(utf16, utf8_char, 6); | |||||
| ascii = utf8_char[0] & 0x80 ? '?' : utf8_char[0]; | |||||
| event = new GHOST_EventKey(system->getMilliSeconds(), | |||||
| GHOST_kEventKeyDown, | |||||
| window, | |||||
| GHOST_kKeyUnknown, | |||||
| ascii, | |||||
| utf8_char, | |||||
| false); | |||||
| } | |||||
| } | |||||
| break; | |||||
| case WM_SYSDEADCHAR: | case WM_SYSDEADCHAR: | ||||
| /* The WM_SYSDEADCHAR message is sent to the window with the keyboard focus when | /* The WM_SYSDEADCHAR message is sent to the window with the keyboard focus when | ||||
| * a WM_SYSKEYDOWN message is translated by the TranslateMessage function. | * a WM_SYSKEYDOWN message is translated by the TranslateMessage function. | ||||
| Context not available. | |||||
No longer ignored, needs explanation.