ch11ng / exwm

Emacs X Window Manager
2.86k stars 137 forks source link

Detect unavailable keys properly. Fixes #592. #932

Open sarg opened 7 months ago

sarg commented 7 months ago

xcb:keysyms:event->keysyms returns '((0 . 0)) when key is unavailable.

Using the same check as here to handle this: https://github.com/ch11ng/exwm/blob/9d035d713ee2692ea095dc6e0668ecabeb550845/exwm-input.el#L889-L890

Update: noticed that xcb:keysyms:event->keysyms returns nil when the keycode is not present in the current layout.

medranocalvo commented 6 months ago

Ok, so we were checking in different ways in different places, we now check consistently in both. I like this change, thank you very much. I have to study xcb:keysyms:event->keysyms before merging, will get to it as soon as possible. (We are also using xcb:keysyms:event->keysyms in exwm-xim; should be reviewed as well.)

medranocalvo commented 5 months ago

@Stebalien, @minad: I would appreciate your thoughts on this issue. The question is whether this patch is fixing the symptoms; see below.

The documentation function xcb:keysyms:event->keysyms says that the function returns a list of keysyms or '((0 . 0)).

@sarg notes that:

Update: noticed that xcb:keysyms:event->keysyms returns nil when the keycode is not present in the current layout.

Well, strictly speaking nil is a list. But does it make sense to differentiate between "keysym not recognized" and "no keycode for keysym"? Should both lead to '((0 . 0))?

Below a half-baked patch implementing that, for exposition.

diff --git i/xcb-keysyms.el w/xcb-keysyms.el
index 8999d71..8c0926a 100644
--- i/xcb-keysyms.el
+++ w/xcb-keysyms.el
@@ -683,7 +683,8 @@ (defconst xcb:keysyms:-xf86-keys
               modifiers (apply #'logior modifiers)))
       (let ((keycode (xcb:keysyms:keysym->keycode obj keysym))
             extra-modifiers)
-        (when (/= 0 keycode)
+        (if (zerop keycode)
+            '((0 . 0))
           (setq extra-modifiers (xcb:keysyms:keycode->keysym obj keycode nil)
                 ;; Always try without other modifier.
                 extra-modifiers (append '(0) extra-modifiers)
@@ -695,10 +696,10 @@ (defconst xcb:keysyms:-xf86-keys
                                                obj keycode modifier))
                                          keysym)
                                   modifier))
-                              extra-modifiers))))
-        (mapcar (lambda (extra-modifier)
-                  (cons keysym (logior (or modifiers 0) extra-modifier)))
-                extra-modifiers)))))
+                              extra-modifiers)))
+          (mapcar (lambda (extra-modifier)
+                    (cons keysym (logior (or modifiers 0) extra-modifier)))
+                  extra-modifiers))))))

 (cl-defmethod xcb:keysyms:keysym->event ((_obj xcb:connection) keysym
                                          &optional mask allow-modifiers)
minad commented 5 months ago

@medranocalvo I will look at this as soon as I can set aside some time for EXWM.

Stebalien commented 5 months ago

Personally, I'd just return nil in all cases and update the call sites to match. That should make it easier to correctly check for failure.

But that would be a breaking change.

But does it make sense to differentiate between "keysym not recognized" and "no keycode for keysym"?

The only case I can think of is if one wanted to re-implement xdotool and:

  1. Try to convert to a keycode.
  2. See that there is no keycode for the keysym. The tool should fail here if the keysym isn't recognized.
  3. Bind a temporary keycode.
  4. Send that temporary keycode.
  5. Unbind the temporary keycode.

This, e.g., lets one type emoji.

However, that's a very contrived case.