Support VK_PACKET in vncviewer on Windows - #1852
Conversation
c6fb5a3 to
04597c9
Compare
Fixes TigerVNC#1847 Co-authored-by: Pierre Ossman <ossman@cendio.se>
04597c9 to
dc2da2d
Compare
|
|
||
| // VK_PACKET handling: Translate to WM_*CHAR message, handled below. | ||
| if (vKey == VK_PACKET) { | ||
| TranslateMessage(msg); |
There was a problem hiding this comment.
I'd rather avoid the risk of intercepting another message type. Was there not enough information in the WM_KEYDOWN event? TranslateMessage() is able to make sense of it somehow, so there should be something there.
There was a problem hiding this comment.
I analyzed a hex dump of the entire WM_KEYDOWN message, and there is nothing there. TranslateMessage() is a Windows API call and need not be limited to the information contained in the parameter. Perhaps there is something in adjacent memory, but even if so it'd be riskier to access that than to use the provided API.
There was a problem hiding this comment.
I played around with it here and I could not find anything in the event either.
But I was getting proper symbols back from win32_vkey_to_keysym(). The complexity from TranslateMessage() might not be needed.
Did you test that path?
There was a problem hiding this comment.
Correct, TranslateMessage() was not needed. Rewritten as you suggested.
| // Plane, BMP), are encoded as a pair of UTF-16 code units that will need to | ||
| // be synthesized into one Unicode code point. | ||
| uint32_t ucsCode = msg->wParam; | ||
| if ((ucsCode & 0xfc00) == 0xd800) { |
There was a problem hiding this comment.
Could the surrogate handling be a separate commit? Makes it easier to see which parts are connected.
There was a problem hiding this comment.
I could, but only by making a first commit that exhibits wrong behavior w.r.t. surrogate pairs. Would you like that?
| codePoint = ucsCode; | ||
| } | ||
| uint32_t keySym = ucs2keysym(codePoint); | ||
| uint32_t keyCode = 0x100 + keySym; // Fake key code |
There was a problem hiding this comment.
Keysym is 32-bit, so this is not going to work. Perhaps something similar to the universal keysyms? I.e. 0x01000000 | codePoint?
There was a problem hiding this comment.
Good point, will change to what you suggested. Another option is actually a constant, e.g. keyCode = 0x123, since we expect only one concurrent VK_PACKET key down.
| /* us the directly encoded 24-bit UCS character */ | ||
| if ((ucs & 0xff000000) == 0) | ||
| /* ucs is a directly encoded 21-bit Unicode character */ | ||
| if (ucs <= 0x10ffff && ((ucs & 0xfff800) != 0x00d800)) |
There was a problem hiding this comment.
I guess this is a safety net? Good idea, but I think it is a bit hidden here. Perhaps instead:
diff --git a/vncviewer/keysym2ucs.c b/vncviewer/keysym2ucs.c
index 6607e3065..ad9b7ebf3 100644
--- a/vncviewer/keysym2ucs.c
+++ b/vncviewer/keysym2ucs.c
@@ -167,6 +167,14 @@ unsigned ucs2keysym(unsigned ucs)
if (keysym != NoSymbol)
return keysym;
+ /* surrogates? */
+ if (ucs >= 0xd800 && ucs <= 0xdfff)
+ return NoSymbol;
+
+ /* private use? */
+ if (ucs >= 0xe000 && ucs <= 0xf8ff)
+ return NoSymbol;
+
/* us the directly encoded 24-bit UCS character */
if ((ucs & 0xff000000) == 0)
return ucs | 0x01000000;
There was a problem hiding this comment.
Will make more explicit as you suggest, but I will leave the 0x10ffff limit since code points above that values are not valid.
|
@CendioOssman Addressed your review comments and pushed a fix. Once you decide what you want me to do with commit splitting, I'll clean up the commit history. |
|
No response. Closing. |
|
@CendioOssman @Neustradamus Rebased and rewrote as suggested without |
|
Pushed |
|
GitHub can't reopen a PR on a branch that was force-pushed. You will have to create a new PR. |
samhed
added
apple remote desktop
|
See #2021 |
3 participants
Fixes #1847