[9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[Timothy Robert Bednarzyk]

I have recently started using 9front on a couple of different machines
(including a native installation on my Thinkpad T470) and use drawterm
on my main laptop (which uses the X11 window manager dwm on Artix Linux)
to connect to one of them. However, on that laptop, I have the caps lock
key remapped to be another control key (ctrl:no_caps) and I have it set
to toggle caps lock when I press both shift keys at the same time
(shift:both_capslock). As it turns out, when the latter xmodmap option
is set, XLookupString interprets shift key release events as caps lock
key release events. This was causing my middle and right mouse buttons
to effectively be swapped permanently after pressing a shift key even
once in a drawterm session (although I didn't know why it was happening
until I did some research and found someone else mentioning them having
shift-related problems in a different program whenever they were using
shift:both_capslock).

I ended up solving this by utilizing the rather strange and unintuitive
fact that the xkey event's state still has the ShiftMask bit set during
the release event (even for actual shift key release events, according
to what I found during my research) and using that to determine whether
the release event should be interpreted as being for a shift key or for
the caps lock key. This likely will cause the middle and right mouse
buttons to unswap if the user is holding down the shift key while
releasing the caps lock key, but considering how unlikely of a scenario
that is and the fact that it is only a temporary and mild problem (all
they would have to do is release and re-press the shift key), I think
this fix is acceptable enough (unless someone else can think of a better
solution, which I would greatly appreciate).

diff --git a/gui-x11/x11.c b/gui-x11/x11.c
index b4f1ad3..8f2b694 100644
--- a/gui-x11/x11.c
+++ b/gui-x11/x11.c
@@ -882,7 +882,16 @@ xkeyboard(XEvent *e)
 			break;
 		case XK_Shift_Lock:
 		case XK_Caps_Lock:
-			k = Kcaps;
+			/*
+			 * Using the xmodmap option shift:both_capslock causes shift
+			 * release events to appear as capslock release events instead.
+			 * Not having this causes the middle and right mouse buttons to
+			 * be swapped permanently after the shift key is pressed once.
+			 */
+			if(e->xany.type == KeyRelease && (e->xkey.state & ShiftMask))
+				k = Kshift;
+			else
+				k = Kcaps;
 			break;
 		case XK_Scroll_Lock:
 			k = Kscroll;

--
trb

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[Jacob Moody]

On 6/28/24 18:53, Timothy Robert Bednarzyk wrote:
> I have recently started using 9front on a couple of different machines
> (including a native installation on my Thinkpad T470) and use drawterm
> on my main laptop (which uses the X11 window manager dwm on Artix Linux)
> to connect to one of them. However, on that laptop, I have the caps lock
> key remapped to be another control key (ctrl:no_caps) and I have it set
> to toggle caps lock when I press both shift keys at the same time
> (shift:both_capslock). As it turns out, when the latter xmodmap option
> is set, XLookupString interprets shift key release events as caps lock
> key release events. This was causing my middle and right mouse buttons
> to effectively be swapped permanently after pressing a shift key even
> once in a drawterm session (although I didn't know why it was happening
> until I did some research and found someone else mentioning them having
> shift-related problems in a different program whenever they were using
> shift:both_capslock).
> 
> I ended up solving this by utilizing the rather strange and unintuitive
> fact that the xkey event's state still has the ShiftMask bit set during
> the release event (even for actual shift key release events, according
> to what I found during my research) and using that to determine whether
> the release event should be interpreted as being for a shift key or for
> the caps lock key. This likely will cause the middle and right mouse
> buttons to unswap if the user is holding down the shift key while
> releasing the caps lock key, but considering how unlikely of a scenario
> that is and the fact that it is only a temporary and mild problem (all
> they would have to do is release and re-press the shift key), I think
> this fix is acceptable enough (unless someone else can think of a better
> solution, which I would greatly appreciate).

Seems reasonable to me. Thanks for debugging this.
My only suggestion would be to axe the "Not having this causes the mouse ...".
What it causes is the shift key to be held down, which then causes the mouse button swapping.
Drawterm itself isn't doing that bit, it's rio.

Thanks,
moody

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[Timothy Robert Bednarzyk]

> Seems reasonable to me. Thanks for debugging this.
> My only suggestion would be to axe the "Not having this causes the mouse ...".
> What it causes is the shift key to be held down, which then causes the mouse button swapping.
> Drawterm itself isn't doing that bit, it's rio.

While it is true that it technically isn't drawterm itself causing that
to happen, it also isn't _just_ rio---holding the shift key after
starting acme directly (instead of starting rio and then running acme
inside a rio window) still swaps the mouse buttons. Given that I wasn't
also experiencing a problem where everything I typed was capitalized as
if it thought that I was just constantly holding down the shift key, I
was only trying to provide context to what can actually be observed
without specially handling "caps lock" key releases. That being said,
I'm perfectly fine with either rephrasing or entirely axing that.

--
trb

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[Jacob Moody]

On 6/29/24 06:16, Timothy Robert Bednarzyk wrote:
>> Seems reasonable to me. Thanks for debugging this.
>> My only suggestion would be to axe the "Not having this causes the mouse ...".
>> What it causes is the shift key to be held down, which then causes the mouse button swapping.
>> Drawterm itself isn't doing that bit, it's rio.
> 
> While it is true that it technically isn't drawterm itself causing that
> to happen, it also isn't _just_ rio---holding the shift key after
> starting acme directly (instead of starting rio and then running acme
> inside a rio window) still swaps the mouse buttons. Given that I wasn't
> also experiencing a problem where everything I typed was capitalized as
> if it thought that I was just constantly holding down the shift key, I
> was only trying to provide context to what can actually be observed
> without specially handling "caps lock" key releases. That being said,
> I'm perfectly fine with either rephrasing or entirely axing that.
> 

Sure rio is a standin for whatever "main" graphical application you're running.
I just want to avoid oversharing in the comment to raise more questions then
it answers. I thought that the later portions of the comment can be safely
extrapolated from the cause of the bug as is described, so I feel it's filler.

This is your work though not mine so I won't die no this hill over some comment
bikeshedding. Let me know what you would like.


Thanks,
moody

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[Timothy Robert Bednarzyk]

> I just want to avoid oversharing in the comment to raise more questions then
> it answers. I thought that the later portions of the comment can be safely
> extrapolated from the cause of the bug as is described, so I feel it's filler.

Honestly, fair enough. I'm probably so used to seeing filler comments
(hell, I've genuinely seen "my funky dunky code" as a comment in some
code that I encountered at my day job) that I've just sort of
internalized them and have become guilty of making them myself. That
being said, I still do like having _something_ provide context to what
would otherwise appear to be completely nonsensical.

> This is your work though not mine so I won't die no this hill over some comment
> bikeshedding. Let me know what you would like.

I agree with hiro that it would be good enough to just put a link to
this thread as the comment if this change actually gets pushed upstream.
However, as I already said in my reply to hiro, I want to try to get the
actual problem in X11 fixed first so that there's no need to specially
handle broken behavior in drawterm itself.

--
trb

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[hiro]

what exactly are you receiving from x11 when you "enable" your
double-shift-caps-lock?

1. shiftdown_left 2. shiftdown_right 3. capslockdown 4. shiftup_left
5. shiftup_right 6. capslocksup ?
which of these are visible, or are they in a different order? e.g. i
can easily imagine something like 123645

On Sat, Jun 29, 2024 at 1:53 AM Timothy Robert Bednarzyk <mail@trbsw.dev> wrote:
>
> I have recently started using 9front on a couple of different machines
> (including a native installation on my Thinkpad T470) and use drawterm
> on my main laptop (which uses the X11 window manager dwm on Artix Linux)
> to connect to one of them. However, on that laptop, I have the caps lock
> key remapped to be another control key (ctrl:no_caps) and I have it set
> to toggle caps lock when I press both shift keys at the same time
> (shift:both_capslock). As it turns out, when the latter xmodmap option
> is set, XLookupString interprets shift key release events as caps lock
> key release events. This was causing my middle and right mouse buttons
> to effectively be swapped permanently after pressing a shift key even
> once in a drawterm session (although I didn't know why it was happening
> until I did some research and found someone else mentioning them having
> shift-related problems in a different program whenever they were using
> shift:both_capslock).
>
> I ended up solving this by utilizing the rather strange and unintuitive
> fact that the xkey event's state still has the ShiftMask bit set during
> the release event (even for actual shift key release events, according
> to what I found during my research) and using that to determine whether
> the release event should be interpreted as being for a shift key or for
> the caps lock key. This likely will cause the middle and right mouse
> buttons to unswap if the user is holding down the shift key while
> releasing the caps lock key, but considering how unlikely of a scenario
> that is and the fact that it is only a temporary and mild problem (all
> they would have to do is release and re-press the shift key), I think
> this fix is acceptable enough (unless someone else can think of a better
> solution, which I would greatly appreciate).
>
> diff --git a/gui-x11/x11.c b/gui-x11/x11.c
> index b4f1ad3..8f2b694 100644
> --- a/gui-x11/x11.c
> +++ b/gui-x11/x11.c
> @@ -882,7 +882,16 @@ xkeyboard(XEvent *e)
>                         break;
>                 case XK_Shift_Lock:
>                 case XK_Caps_Lock:
> -                       k = Kcaps;
> +                       /*
> +                        * Using the xmodmap option shift:both_capslock causes shift
> +                        * release events to appear as capslock release events instead.
> +                        * Not having this causes the middle and right mouse buttons to
> +                        * be swapped permanently after the shift key is pressed once.
> +                        */
> +                       if(e->xany.type == KeyRelease && (e->xkey.state & ShiftMask))
> +                               k = Kshift;
> +                       else
> +                               k = Kcaps;
>                         break;
>                 case XK_Scroll_Lock:
>                         k = Kscroll;
>
> --
> trb

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[Timothy Robert Bednarzyk]

> what exactly are you receiving from x11 when you "enable" your
> double-shift-caps-lock?
>
> 1. shiftdown_left 2. shiftdown_right 3. capslockdown 4. shiftup_left
> 5. shiftup_right 6. capslocksup ?
> which of these are visible, or are they in a different order? e.g. i
> can easily imagine something like 123645

Below, to the left of each colon represents what I am physically doing
and to the right represents the events that occur in X11. On the left, I
will use PL/PR for me pressing the left/right shift keys and RL/RR for
releasing the left/right shift keys. On the right, I will use your
numbers.

PL, RL: 1, 6.

PR, RR: 2, 6.

PL, PR, RL, RR: 1, 3, 6, 5.

PL, PR, RR, RL: 1, 3, 6, 6.

PR, PL, RL, RR: 2, 3, 6, 6.

PR, PL, RR, RL: 2, 3, 6, 4.

Interestingly, if I press one shift key, then the other, release the
first shift key, and then release the second one, I do actually get the
release event for the second shift key. I also don't get a press event
for that second shift key, but that makes sense to me since pressing the
second shift key is supposed to be interpreted as pressing caps lock
anyways. Perhaps most interesting to me though is that if I instead
release the second shift key before the first one I get two events for
releasing the caps lock in a row and zero events for releasing shift
keys.

While the press/release events work strangely with xmodmap's
shift:both_capslock option, the modifier masks do behave normally and I
believe that is why I have never noticed a problem in any other program
in the ~2/3 of a year that I've been using it (and why I wasn't stuck
typing in all caps through drawterm despite the mouse buttons staying
swapped). Perhaps a more robust solution to the problem would be to
probe the current state of the modifiers when handling things such as
mouse button presses rather than to rely on press/release events, but I
don't know nearly enough to know how difficult or feasible doing that
would be.

--
trb

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[hiro]

this does seem like your xmodmap is causing a bug in x11.
the behavior is completely non-sensical. maybe nobody ever tested this
x11 feature? ;)
not sure how much we should care about workarounds for bugs that only
happen in an edge-case of a (mis-?)configuration.
otoh a few well commented lines that contain a good justification for
this braindamage, which now exists after such deep analysis wouldn't
bother me personally... even if it's just a link to this thread.

On Sat, Jun 29, 2024 at 7:19 PM Timothy Robert Bednarzyk <mail@trbsw.dev> wrote:
>
> > what exactly are you receiving from x11 when you "enable" your
> > double-shift-caps-lock?
> >
> > 1. shiftdown_left 2. shiftdown_right 3. capslockdown 4. shiftup_left
> > 5. shiftup_right 6. capslocksup ?
> > which of these are visible, or are they in a different order? e.g. i
> > can easily imagine something like 123645
>
> Below, to the left of each colon represents what I am physically doing
> and to the right represents the events that occur in X11. On the left, I
> will use PL/PR for me pressing the left/right shift keys and RL/RR for
> releasing the left/right shift keys. On the right, I will use your
> numbers.
>
> PL, RL: 1, 6.
>
> PR, RR: 2, 6.
>
> PL, PR, RL, RR: 1, 3, 6, 5.
>
> PL, PR, RR, RL: 1, 3, 6, 6.
>
> PR, PL, RL, RR: 2, 3, 6, 6.
>
> PR, PL, RR, RL: 2, 3, 6, 4.
>
> Interestingly, if I press one shift key, then the other, release the
> first shift key, and then release the second one, I do actually get the
> release event for the second shift key. I also don't get a press event
> for that second shift key, but that makes sense to me since pressing the
> second shift key is supposed to be interpreted as pressing caps lock
> anyways. Perhaps most interesting to me though is that if I instead
> release the second shift key before the first one I get two events for
> releasing the caps lock in a row and zero events for releasing shift
> keys.
>
> While the press/release events work strangely with xmodmap's
> shift:both_capslock option, the modifier masks do behave normally and I
> believe that is why I have never noticed a problem in any other program
> in the ~2/3 of a year that I've been using it (and why I wasn't stuck
> typing in all caps through drawterm despite the mouse buttons staying
> swapped). Perhaps a more robust solution to the problem would be to
> probe the current state of the modifiers when handling things such as
> mouse button presses rather than to rely on press/release events, but I
> don't know nearly enough to know how difficult or feasible doing that
> would be.
>
> --
> trb

Re: [9front] [PATCH] Fix shift release events in X11 drawterm with shift:both_capslock xmodmap option

[Timothy Robert Bednarzyk]

> this does seem like your xmodmap is causing a bug in x11.

I do actually have to apologize, I've had these options set in an X11
configuration file for months and had forgotten which program was used
to enable the option on the fly. Prior to sending my first e-mail, I had
quickly looked up the option to re-remember what program it was, and
xmodmap came up first (and, incidentally, I now remember that when I was
first attempting to do something similar to those options, I had tried
to do it with xmodmap first). As it turns out, those options can 
actually be set by setxkbmap, not xmodmap. The specific X11 program that
actually contains the options is mostly an inconsequential detail here,
I realize, but I wanted to correct myself regardless.

> the behavior is completely non-sensical. maybe nobody ever tested this
> x11 feature? ;)

I wouldn't be surprised if some emacs users were using the same options
as me; after all, I originally set them when I wanted to attempt to try
out emacs and was following the advice of many who suggest remapping
caps lock to control (but still wanted to have some way of toggling caps
lock). While I ended up not using emacs too much (honestly, I think I
prefer even ed over emacs), I did like the options that I'd set and have
kept them since. Drawterm was the first program that I've used where
this insane behavior in X11 actually surfaced as a problem, so I also
wouldn't be surprised if I was the first one to thoroughly test the
option and discover exactly how it's screwy.

> not sure how much we should care about workarounds for bugs that only
> happen in an edge-case of a (mis-?)configuration.

That is a good point. Originally, I was so focused on "fixing" the "bug"
in drawterm (when I should've just gone to bed) that I hadn't even
considered just getting the actual X11 bug fixed where it should be once
I learned what was causing the problem. I'll try to get it fixed
wherever appropriate first; this change to drawterm can be saved as a
last resort if someone determines that the current X11 behavior is not
to be changed for some bs reason like "compatibility", or even worse:
claims that it's actually the correct and intended behavior.

> otoh a few well commented lines that contain a good justification for
> this braindamage, which now exists after such deep analysis wouldn't
> bother me personally... even if it's just a link to this thread.

Should this change actually be applied to upstream drawterm, I fully
agree that the comment should just contain a link to this thread.

--
trb