Support VK_PACKET in vncviewer on Windows - #2021
Conversation
3f0f7bb to
82b03fb
Compare
|
Squashed. |
|
Sorry, I haven't had time to look at this yet. It's on my to-do list, so it's not forgotten. |
CendioOssman
left a comment
CendioOssman
left a comment
There was a problem hiding this comment.
Looks really good! There are some minor tweaks and we can get this merged.
|
@CendioOssman ping |
|
I'm unfortunately days away from my summer holiday. I'll see if I can find some time to test and merge this, but there is a risk it will have to wait until I'm back. |
CendioOssman
left a comment
CendioOssman
left a comment
There was a problem hiding this comment.
Looks very nice. Some small stylistic fixes and then we're ready.
What good ways are there to test this?
| return true; | ||
| } | ||
| else { | ||
| assert((ucsCode & 0xfc00) == 0xdc00); |
There was a problem hiding this comment.
I assume this comes from Windows? We should be prepared for bugs there and not just crash. A check and return is more appropriate.
There was a problem hiding this comment.
No, this is a provable invariant. Added comments.
| vlog.error(_("Unmatched UTF-16 surrogate pair through VK_PACKET, " | ||
| "code: 0x%04x"), |
There was a problem hiding this comment.
It's best if we can keep the strings identical to avoid burdening the translators.
| HKL currentLayout; | ||
|
|
||
| bool altGrArmed; | ||
| uint32_t vkPacketHighSurrogate; |
There was a problem hiding this comment.
This is a confusing placement as it is not related to the AltGr variables above and below it.
I suggest giving it its own section below those two.
| /* surrogates? */ | ||
| if (0xd800 <= ucs && ucs <= 0xdfff) | ||
| return NoSymbol; | ||
|
|
||
| /* private use? */ | ||
| if ((0xe000 <= ucs && ucs <= 0xf8ff) || | ||
| (0x0f0000 <= ucs && ucs <= 0x0ffffd) || | ||
| (0x100000 <= ucs && ucs <= 0x10fffd)) | ||
| return NoSymbol; |
There was a problem hiding this comment.
This is a nice addition, but not strictly related to VK_PACKET? Could you put it as a separate commit?
There was a problem hiding this comment.
Oh wait, separate commit, not PR. Fixed and force pushed.
I don't know how "good" it is, but here's the way I test it manually:
Expected / with this fix:
Actual / without this fix:
|
Fixes TigerVNC#1847 Co-authored-by: Pierre Ossman <ossman@cendio.se>
2 participants
No description provided.