Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support VK_PACKET in vncviewer on Windows #1852

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support VK_PACKET in vncviewer on Windows #1852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc2da2d
Support VK_PACKET in vncviewer on Windows
svenssonaxel Oct 17, 2024
d7956ca
Review fixes
svenssonaxel Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions vncviewer/Viewport.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ Viewport::Viewport(int w, int h, const rfb::PixelFormat& /*serverPF*/, CConn* cc
lastPointerPos(0, 0), lastButtonMask(0),
#ifdef WIN32
altGrArmed(false),
vkPacketHighSurrogate(0),
#endif
firstLEDState(true), pendingClientClipboard(false),
menuCtrlKey(false), menuAltKey(false), cursor(nullptr)
Expand Down Expand Up @@ -971,6 +972,12 @@ int Viewport::handleSystemEvent(void *event, void *data)
keyCode = 0x38;
}

// VK_PACKET handling: Translate to WM_*CHAR message, handled below.
if (vKey == VK_PACKET) {
TranslateMessage(msg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return 1;
}

// Windows doesn't have a proper AltGr, but handles it using fake
// Ctrl+Alt. However the remote end might not be Windows, so we need
// to merge those in to a single AltGr event. We detect this case
Expand Down Expand Up @@ -1086,6 +1093,12 @@ int Viewport::handleSystemEvent(void *event, void *data)
keyCode = 0x38;
}

// VK_PACKET handling: Translate to WM_*CHAR message, handled below.
if (vKey == VK_PACKET) {
TranslateMessage(msg);
return 1;
}

// We can't get a release in the middle of an AltGr sequence, so
// abort that detection
if (self->altGrArmed)
Expand Down Expand Up @@ -1118,6 +1131,55 @@ int Viewport::handleSystemEvent(void *event, void *data)
self->handleKeyRelease(0x36);
}

return 1;
} else if ((msg->message == WM_CHAR) || (msg->message == WM_DEADCHAR) ||
(msg->message == WM_SYSCHAR) || (msg->message == WM_SYSDEADCHAR)) {
// Windows will send VK_PACKET key events if it doesn't have a physical key
// associated with the event (e.g. from a mobile device virtual keyboard).
// After translating a keydown/keyup event pair with VK_PACKET as vKey, a
// WM_*CHAR message will be issued containing a UTF-16 code unit. Unicode
// characters outside of the range U+0000 - U+ffff (Basic Multilingual
// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the surrogate handling be a separate commit? Makes it easier to see which parts are connected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, but only by making a first commit that exhibits wrong behavior w.r.t. surrogate pairs. Would you like that?

// We have received a high surrogate code unit. Remember it and wait for
// the low surrogate which should come immediately after.
if (self->vkPacketHighSurrogate) {
vlog.error(_("Unmatched UTF-16 surrogate pair through VK_PACKET, "
"codes: 0x%04x, 0x%04x"),
self->vkPacketHighSurrogate, ucsCode);
}
self->vkPacketHighSurrogate = ucsCode;
return 1;
}
uint32_t codePoint = 0;
if ((ucsCode & 0xfc00) == 0xdc00) {
// We have received a low surrogate code unit. We should have a high
// surrogate saved that we can use to calculate the code point.
if (!self->vkPacketHighSurrogate) {
vlog.error(_("Unmatched UTF-16 surrogate pair through VK_PACKET, "
"code: 0x%04x"),
ucsCode);
return 1;
}
codePoint = (((self->vkPacketHighSurrogate & 0x03ff) << 10) |
(ucsCode & 0x03ff)) + 0x010000;
} else {
// BMP character. Unicode characters in this range are encoded as a
// single UTF-16 code unit.
if (self->vkPacketHighSurrogate) {
vlog.error(_("Unmatched UTF-16 surrogate pair through VK_PACKET, "
"codes: 0x%04x, U+%04x"),
self->vkPacketHighSurrogate, ucsCode);
self->vkPacketHighSurrogate = 0;
}
codePoint = ucsCode;
}
uint32_t keySym = ucs2keysym(codePoint);
uint32_t keyCode = 0x100 + keySym; // Fake key code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keysym is 32-bit, so this is not going to work. Perhaps something similar to the universal keysyms? I.e. 0x01000000 | codePoint?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self->handleKeyPress(keyCode, keySym);
self->handleKeyRelease(keyCode);
return 1;
}
#elif defined(__APPLE__)
Expand Down
1 change: 1 addition & 0 deletions vncviewer/Viewport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class Viewport : public Fl_Widget, public EmulateMB {

#ifdef WIN32
bool altGrArmed;
uint32_t vkPacketHighSurrogate;
unsigned int altGrCtrlTime;
#endif

Expand Down
4 changes: 2 additions & 2 deletions vncviewer/keysym2ucs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ unsigned ucs2keysym(unsigned ucs)
if (keysym != NoSymbol)
return keysym;

/* 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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make more explicit as you suggest, but I will leave the 0x10ffff limit since code points above that values are not valid.

return ucs | 0x01000000;

/* no matching keysym value found */
Expand Down
Loading