[OpenSmalltalk/opensmalltalk-vm] Linux Squeak VM X11 keyboard changes breaks 32b VM (#396)

Tim Johnson reported a problem with 32b Linux builds:
http://forum.world.st/Linux-Squeak-VM-X11-keyboard-changes-td5099584.html

Pressing ctrl-d now acts like debugIt instead of doIt like in the 64b. Ctrl-p also stopping working.

I did a bisection of all versions in https://bintray.com/opensmalltalk/vm/cog/ and narrowed down the problem to

good squeak.cog.spur_linux32x86_201811272342.tar.gz
bad squeak.cog.spur_linux32x86_201812301551.tar.gz

The error is consistent on both 18231 and the oldest 18221 released image for 32b VM but does not appear with 64b VMs.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Thanks for all the work you've done on this.
I (Tim J who reported the problem) have built a debug build and uploaded it to a personal web server. If you trust a strange binary built on a DigitalOcean server, it is here (tarball is named after the commit hash).

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Thanks Tim,

The following lines in sqUnixX11.c doesn't seem right. When Ctrl and p are pressed at the same time, sq2KeyPlain should return ctrl-p (16r10). This code returns p instead (16r70). I don't understand why this change was made.

2065 if (charCode >= 1 && charCode <= 26) {
2066 /* check for Ctrl-letter that gets translated into charCode 1-26 instead of letters a-z */
2067 KeySym keysym = *symbolic;
2068 if (keysym >= XK_a && keysym <= XK_z)
2069 return (int)'a' + (keysym - XK_a);
2070 if (keysym >= XK_A && keysym <= XK_Z)
2071 return (int)'A' + (keysym - XK_A);

Could you try a build with this section removed and see if it fixes your problem?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Relevant commit: 1b837f9

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Tim,

I used gdb to nop out above code in the text segment as this was quicker and easier than trying to rebuild a 32b binary. Then the image worked like charm.

The comment in recode() is confusing and I need time to understand its rationale. Shift is keyboard specific and decides charCode and should be handled in the VM while Ctrl is a modifier and should be passed to the image. So I don't understand why a key event should be generated when Shift alone is pressed or released.

I also found a thread from 2012 where Guillermo Polito fixed a similar problem in Windows code
http://forum.world.st/Windows-cog-vm-Keyboard-events-related-about-keycode-mapping-td4330359.html

HTH .. Subbu

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Hi Subbu,
I have rebuilt the 32-bit Linux x86/i386 Squeak-Cog-Spur VM with the above section #ifdef'ed out, and it works. I tested Ctrl-P, Ctrl-D, and Ctrl-Shift-D. Each of them do what is expected/desired.
Thank you!
Tim

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

So, if I understand the issue, the problem is that we always get keysym == XK_A rather than keysym == XK_a when CTRL is pressed, even if we did not press SHIFT.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

The keysym is produced by XLookupString.

https://linux.die.net/man/3/xlookupstring sounds clear to me: it should honour the shift for the Keysym output, and map to ASCII for the ASCII buffer output...

The XLookupString function translates a key event to a KeySym and a string. The KeySym is obtained by using the standard interpretation of the Shift, Lock, group, and numlock modifiers as defined in the X Protocol specification. If the KeySym has been rebound (see XRebindKeysym), the bound string will be stored in the buffer. Otherwise, the KeySym is mapped, if possible, to an ISO Latin-1 character or (if the Control modifier is on) to an ASCII control character, and that character is stored in the buffer. XLookupString returns the number of characters that are stored in the buffer.

Does it work like advertised? We'd better activate DEBUG_KEYBOARD_EVENTS, but currently we print address of the keysym rather than the keysym!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Actually, the problem is that charCode in sq2KeyPlain has already translated ctrl and XK_a as ctrl-a (16r1), but the errant code returns 'a' instead of ctrl-a.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Hi Subbu, you made very good job at tracking the changes causing the problem, but I'm not sure that reverting is the right solution, this change was made for some reason. If we want to disentangle the code and remove some cruft we need better understanding. So let's focus on symptoms: CTRL+d is performing debugIt: rather than doIt: action.

So clearly, the CTRL/CMD or whatever modifier is taken into account, and transformation of keyValue to Ctrl-d code (0x4) is not necessary. It is not only not necessary, it also create various problems of interpretation of keyboard event, because 0x4 is also the encoding of END key, see implementors/senders of specialShiftCmdKeys. I played with various combinations of CTRL,ALT,OPTION,CMD,SHIFT + d on MacOS and Windows VM, and HandMorph showEvents: true., and I can tell that it's messy all the way down!

The problem we observe in Squeak images is that the TextEditor performs the shiftCmdActions instead of the cmdActions. See initializeCmdKeyShortcuts and initializeShiftCmdKeyShortcuts in SmalltalkEditor. Why?

A good candidate for suspiscion is in TextEditor>>dispatchOnKeyboardEvent:

"the control key can be used to invoke shift-cmd shortcuts"
(honorCommandKeys and: [ aKeyboardEvent controlKeyPressed ])
    ifTrue: [^ self
        perform: (self class shiftCmdActions at: aKeyboardEvent keyValue + 1)
        with: aKeyboardEvent].

Ahah: I don't know if this is a MacOS-centric piece of code or what, but if we generate CTRL modifier instead of CMD modifier, maybe the image code ain't gonna perform the cmdActions, but rather the shiftCmdActions as we observe. Since you seem to identify a change in modifier state between old and new VM, this is maybe where we start inquiring instead.

The keyboard event is composed of several parts:

• keyValue code (eventBuffer third at image side) that tries to be an ASCII code + special codes for special keys (cursor, etc...) - as produced by sq2KeyPlain and friends.
• ucs4 (utf32) code for the character (eventBuffer sixth)
• keyboard modifiers (eventBuffer fifth) with a very complex dance in the VM.

But only one code is retained in the KeyboardEvent distributed for Morph consumption: either the keyValue, or the ucs4 code might be used, depending on Localized World currentHand keyboardInterpreter class. Most probably the ucs4 one nowadays if non null. By the way, what is your keyboardInterpreter class?

Note that Ctrl-d has some mapping in shiftCmdActions, "thanks" to this super hack in initializeShiftCmdKeyShortcuts

1 to: cmds size by: 2 do: [ :i |
    cmdMap at: ((cmds at: i) asciiValue + 1) put: (cmds at: i + 1). "plain keys"
    cmdMap at: ((cmds at: i) asciiValue - 32 + 1) put: (cmds at: i + 1). "shifted keys"
    cmdMap at: ((cmds at: i) asciiValue - 96 + 1) put: (cmds at: i + 1). "ctrl keys"
].

which implies that shift-command #debugIt: associated to $d (100 0x64) will also be associated to $D (68 0x44) and to Ctrl-d (4 0x04).

But I do not see any thing like that in cmdActions, SmalltalkEditor cmdActions occurrencesOf: #doIt: answers 1, it is associated to $d only.

You see that this would/could conflict with END key special actions (same for other special keys encoding), which is a bad thing. The Ctlr-d (0x4) value & co are a bad thing coming from the past (like console mode compatibility) that we do not need, neither at low level keyboard handling (KeyUp KeyDown), nor at higher level synthetic event handling (KeyStroke/KeyChar). I think we'd better keep the change you legitimately incriminated, and I would rather try to understand all the implications of translateCode and when/how it handles the modifier states instead...

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Hi Nicolas,
Thank you for the insightful research. This does seem like an area that benefits from greater documentation, attention, and cleanup, because of many generations of backwards-compatibility, multilingual support, and cross-platform code.

I need to ask, though: why then is this only a problem on 32-bit Linux VMs but not 64-bit Linux VMs?

Thanks,
Tim

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

I don't understand why this change was made.

2065   if (charCode >= 1 && charCode <= 26) {

2066     /* check for Ctrl-letter that gets translated into charCode 1-26 instead of letters a-z */

2067     KeySym keysym = *symbolic;

2068     if (keysym >= XK_a && keysym <= XK_z)

2069       return (int)'a' + (keysym - XK_a);

2070     if (keysym >= XK_A && keysym <= XK_Z)

2071       return (int)'A' + (keysym - XK_A);

Paraphrasing feenkcom/gtoolkit#9 found by @akgrant43 ...

Girba: A piece of information about this bug. There is an inconsistency in Mac and Linux VMs when it comes to keyboard events:
-- OSX generates keyDown: events for keys like shift , ctrl or meta when I press the corresponding keys, while
-- Linux VM generates keyDown: events for ctrl when we release the key
Our issue is at a lower level then Morphic.
@syrel used logging statements in HandMorph>>#generateKeyboardEvent:. It appears that it is not called when we press a modifier key on Linux.

syrel: Indeed, the keyboard events for shift are sent but the question is when they are sent. We state that they are not sent when we press the shift button, but instead when we release it. This is a very important and crucial difference that does not let us to have simple and clear shortcut handling in Bloc on linux. In Bloc by design we assume that whenever a key is pressed - we get a keydown event (hence it has down in it), right after that for printable characters we get keystroke (or input) event that gets resent every X seconds depending on OS settings and finally when user releases her finger we get keyup event. Unfortunately this does not happen for keys like shift, alt and ctrl while perfectly works for regular keys like A, M, Z.
@akgrant43 I have no idea why it works for you but doesn’t work for multiple other people. We are debugging this for the latest Pharo7 64bit with latest VM on Linux Mint 64bit.

akgrant43: I'm not seeing this behaviour (Ubuntu 16.04).
Adding logging to:
Morph>>handleKeyDown:
Morph>>handleKeyUp:
Shows them being called at the expected times, i.e. as the ctrl key is being pressed and released.
Linux Mint, if I remember correctly, is typically based on a fairly old version of Ubuntu (not that mine is recent :-)). Maybe there's a change in the kernel or graphics stack?

j-brant: I have gotten the keyboard shortcuts to work under my Linux Mint installation. There were a few issues. First, the KeyboardKey class was using the wrong key table for Unix. Second, the right Unix key table was missing the right control & alt keys. These were addressed in https://pharo.manuscript.com/f/cases/22769 and have been integrated into the latest Pharo build.
.
The other issues were VM issues. When you pressed and then released either the control or shift keys, the correct key would be recorded as pressed, but character code $? would be reported when the key was released. This causes the editor to believe the control key was still pressed even after it was released since it never got the key up for the control. The other VM issue was that A-Z keys being pressed when the control was pressed was being reported as character code 1-26 instead of the letters a-z. On the other platforms, they are reported as letters with the control key pressed. On Linux, they were reported with the control key down, but with character codes 1-26. To fix the VM issues, I added some code to the x2sqKeyPlain method in sqUnixX11.c file before the return (j-brant/opensmalltalk-vm@37e3abf):

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Paraphasing http://forum.world.st/Linux-Squeak-VM-X11-keyboard-changes-td5099584.html

kksubbu: EventSensor classPool associationAt: #KeyDecodeTable is different
between the good and bad VM. The good one has 184 entries while the bad
one has only 16 entries :-( See attached pics taken from the bad and good VM.
.
The bad 32b VM is returning a different modifier bit (64 instead of 16) in the keyboard event compared to the good VMs.
.
With HandMorph showEvents: true
Pressing Ctrl shows up as "keyDown Ctrl-Opt-û" (251). Now if I press p
it shows "keyStroke Ctrl-p (112)". If I release p key, it continues to
show "keyUp Ctrl-p (112)". If I release Ctrl now, it shows "keyUp û (251)".
In the good image, pressing p after Ctrl shows up as "Cmd-p (112)"

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Paraphasing http://forum.world.st/Windows-cog-vm-Keyboard-events-related-about-keycode-mapping-td4330359.html

guillep: I was playing to add Function Key support in the windows vm (yep, always the same :P), and looking at the code, I saw this in the recordKeyboardEvent:

  evt = (sqKeyboardEvent*) sqNextEventPut();
...
  evt->charCode = keymap[keyCode & 0xff];

the problem with that line is that KeyDown and KeyUp events send VirtualKeycodes as keycodes and the Char event sends a unicode char value. And, it makes collisions, since for example:
-- $p char value is 112 ; and
-- F1 virtual code value is 112 too

And other keys are the same, image-side having the same keycode with different keys.
To distinguish these keys, the following provides to the image a Char event the keycode without mapping (code link for more context):

    case WM_CHAR:
    case WM_SYSCHAR:
      /* Note: VK_RETURN is recorded as virtual key ONLY */
      if(keyCode == 13) return 1;
+      charCode = keyCode;
      pressCode = EventKeyChar;
     break
...
-  evt->charCode = keymap[keyCode & 0xff];
+  evt->charCode = charCode? charCode : keymap[keyCode & 0xff];

andreas: The change makes no sense (it will break most non-ascii input like accents, umlauts, etc). You really shouldn't be using character events for handling function keys. There is no 'F11 Character' in any character encoding world-wide so trying to represent F11 as a character is completely futile. You need to use keyDown and keyUp events, since F11 & friends are KEYs not CHARACTERs.

guille: Sure! I'm not generating Char events for F11 ;). Since F11 does not generate Char events.
The problem is that the p key generates the same KeyDown event than F11 for the vm. I'm only fixing that p-key and F1-key have different keyCodes for KeyDown and KeyUp events.
Now, utf32Code should be only used on KeyChar events, and I don't care about them.
.
How does it work now?
(KeyDown, KeyUp) and KeyChar events send different keyCodes to the image.
Right now works because in no place in the image keyDown: is handled.
.
More on, in the unix vm, the behavior is the following:
when pressing F1:
-- key: 112 char: 112 type: keyDown
-- key: 112 char: 112 type: keyUp
when pressing p:
-- key: 112 char: 112 type: keyDown
-- key: 112 char: 112 type: keystroke
-- key: 112 char: 112 type: keyUp
when pressing shift + p (P):
-- key: 254 char: 254 type: keyDown
-- key: 80 char: 80 type: keyDown
-- key: 80 char: 80 type: keystroke
-- key: 80 char: 80 type: keyUp
After adding support for Function keys, when pressing F1:
key: 16 char: 16 type: keyDown
key: 16 char: 16 type: keyUp
.
The question is: Should KeyDown, KeyUp and KeyChar events for the same key produce the same keyCode? I think yes. Because the keyCode indicates the key pressed in the keyboard. Doesn't it?

andreas: It does. Which is precisely the reason why it can't produce different values when you press the shift key with it. The KEY you are pressing does not change depending on the modifier; your OS decides that the combination of the Shift key and the P key produces an uppercase P character.

guille: The main problem is that I want keycodes to be the same through the three platforms (unix, mac and windows). And for that, I have to do a mapping somewhere (this piece of code I wrote today was to play and see how it behaves, because the codes are to be defined :) ).
So, I thought those conversions could be done through the keymap array.
Maybe that's not the way to do the mapping, but the mapping should be done somewhere.

andreas: There was general agreement that probably the best way to do this would be based on the X11/keysym.h because it seems to be the most complete source for key codes. It's just that that nobody sat down and wrote the mapping from VK_XXX to XK_YYY.

guillep: Hmm, that would be good, I'll have a look tomorrow :). Thanks!

What was the conclusion of looking at that @guillep?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Side comment 1 from http://forum.world.st/Windows-cog-vm-Keyboard-events-related-about-keycode-mapping-td4330359.html

@jvuletich: On the mac, when control or command are pressed, for any keystroke the unshifted code is returned, even if shift is pressed.
For instance, let's assume an US keyboard. So, [shift]+[,] generates $< and [shift]+[.] generates $>.
If you run 'Sensor kbdTest' and press [shift]+[,] you correctly get $<,
but if you press [control]+[shift]+[,] or [command]+[shift]+[,] you get $,.
This is bad, because to detect [command]+[<] or [control]+[<] you need to write code that not only
needs to know about the platform, but also about the keyboard layout, as
in many layouts $< is not generated by doing [shift]+[,], but by some
other combination. The same happens with most non-alphabetic keys, that
usually differ in different keyboard layouts.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Side comment 2 from http://forum.world.st/Windows-cog-vm-Keyboard-events-related-about-keycode-mapping-td4330359.html

@jvuletich: There is a completely separated issue, and it happens both in Windows
and Mac. Here, [ctrl] + [an alphabetic key] substracts 64 from the code.
So, [ctrl]+[c] generates code 3. This is consistent with the traditional
meaning of the ctrl key (in dumb terminals and DOS), but it makes
impossible for the image to tell (for example) between [ctrl]+[Enter]
and [ctrl]+[m]. The image might want to use these keystrokes for
different things, so it would be much better not to substract 64 in the
VM and let the image handle it. I know it could be done by handling key
down and key up events, but this would also require code that is not
only platform dependent but also needs to know the mapping between key
codes and characters in each platform.
.
In general, I think that [control], [command] and [alt/option] should
not affect the character code of a keystroke, they should only set the
appropriate flag so the image can decide what to do with them. This is
completely different for [shift], as [shift] does indeed modify the
character generated.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Thank you, Nicolas, for your detailed analysis of the code path for handling keystrokes in the editor. Initially, I too traced this path and noticed that EventSensor>>processEvent: maps keystrokes using KeyDecodeTable before calling processKeyboardEvent. Key alt-d correctly invoked cmdActions in both 32b and 64b images but ctrl-d invoked shiftCmdActions in 32b image instead of cmdActions. The KeyDecodeTable is different in these two images.

I agree that the sqUnixX11.c file is very messy and confusing. It mixes X11 terms (raw keycode and mapped keysym) with Squeak specific terms (keycode, charcode). JB's patch broke 32b image but not 64b. I proposed a partial rollback of this patch to get TimJ going while we sort out the mess.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

The change was made for this reason:
The other VM issue was that A-Z keys being pressed when the control was pressed was being reported as character code 1-26 instead of the letters a-z. On the other platforms, they are reported as letters with the control key pressed. On Linux, they were reported with the control key down, but with character codes 1-26.
I only tested the change on a 64-bit Pharo image. Playing around a little with a 64-bit Squeak 5.2 image, it appears that Squeak is likely translating the character codes 1-26 specially. I added some debugging code to EventSensor>>fetchMoreEvents (in the whileFalse: loop):

type = EventTypeKeyboard 
    ifTrue: [Transcript show: (eventBuffer at: 3) printString , ' - ', (eventBuffer at: 5) printString; cr; flush].

When running with the old vm and pressing/releasing Ctrl-a, I get this printed to the transcript:

251 - 6
1 - 2
1 - 2
1 - 2
63 - 0

When running with the new vm I get:

251 - 6
97 - 2
97 - 2
97 - 2
251 - 0

The new vm generates the correct codes, but Ctrl-a isn't selecting the whole text. Watching the events using HandMorph showEvents: true, you can see that the image is converting the old vm's Ctrl-a into Cmd-a, but it isn't converting the new vm's Ctrl-a into Cmd-a.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

I agree that the sqUnixX11.c file is very messy and confusing. It mixes X11 terms (raw keycode and mapped keysym) with Squeak specific terms (keycode, charcode). JB's patch broke 32b image but not 64b. I proposed a partial rollback of this patch to get TimJ going while we sort out the mess.

Are you sure that it is working in a 64-bit image? I wasn't able to use Ctrl-a in my 64-bit vm using a Squeak 5.2 image with the new vm.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Are you sure that it is working in a 64-bit image? I wasn't able to use Ctrl-a in my 64-bit vm using a Squeak 5.2 image with the new vm.

I haven't tested it. I am just taking others' word that they tested a 64-bit VM and could not reproduce the problem I was reporting on my 32-bit VM.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.