joshgoebel / keyszer

a smart, flexible keymapper for X11 (a fork/reboot of xkeysnail )
Other
69 stars 15 forks source link

Fix for macro and Unicode sequence reliability #127

Closed RedBearAK closed 1 year ago

RedBearAK commented 1 year ago

Removing the sync line from the send_key_action function fixes problems with macros and Unicode keystroke sequences that fail partway through.

This change appears to resolve a long-standing problem with macro keystroke sequences and Unicode keystroke sequences failing partway through in various circumstances.

No negative side effects are known or have yet been observed.

RedBearAK commented 1 year ago

@joshgoebel

The linting check failed but it appears to be something about how the linting is configured. You need an API key for a safety check? Doesn't seem to be anything I can do about that on my end.

joshgoebel commented 1 year ago

It's saying we need to update setuptools... because the old version has a vulnerability...

joshgoebel commented 1 year ago

Might leave this open a while (for testing)... it's weird to think we'd need no ACKS... and with this removed the only acks we are getting are going to be original ones that are passed thru (immediately) from the real keyboard (since they are unmappable events)... the proper solution here might be to rewrite the input/modify layer such that it can understands entire key sequences - not just individual events...

IE we have:

So we'd need to understand and rewrite them all.. after this patch we're just remapping key events and passing all scan and ack events straight thru with 0 processing. Amazing that seems to work so well.

I wonder what would happen if we just removed ALL the acks.

RedBearAK commented 1 year ago

Amazing that seems to work so well.

Yes, I am also surprised that this doesn't appear to have any negative consequences that I've yet noticed. Everything seems fine.

I wonder what would happen if we just removed ALL the acks.

This probably isn't what you meant, but I took a wild guess and tried adding this to the start of on_event:

# @benchit
def on_event(event, device):
    if event.type == ecodes.EV_SYN:
        return

The results seem... unequivocally bad. Like the system thinks I'm constantly holding or typing some keys. Had to bail and take it out right away.

I'll continue to use this patch of removing the sync line, and update this if I ever run into anything odd. So far, nothing.

Probably is a good idea to let this one sit here and cook for quite some time. 👍🏽

joshgoebel commented 1 year ago

This probably isn't what you meant, but ... results seem... unequivocally bad.

Exactly what I meant, and kind of what I expected... so strange though.

RedBearAK commented 1 year ago

For reference, apparently syn() injects a SYN_REPORT, not an EV_SYN event (unless that's in the write() queue?).

Not that any of that means anything to me yet.

This is the entire syn() function definition.

    def syn(self):
        '''
        Inject a ``SYN_REPORT`` event into the input subsystem. Events
        queued by :func:`write()` will be fired. If possible, events
        will be merged into an 'atomic' event.
        '''

        _uinput.write(self.fd, ecodes.EV_SYN, ecodes.SYN_REPORT, 0)
joshgoebel commented 1 year ago

For reference, apparently syn() injects a SYN_REPORT, not an EV_SYN event

Same thing, well technically EV_SYN is the whole category... but SYN_REPORT is the only event I'm aware of being used in actual practice.

Events queued by :func:write() will be fired.

This is likely why we were still calling it, if we never called it (and also didn't randomly pass it thru) then any events queued would never be "fired" (and as you've seen that's a no go). So perhaps the secret sauce is in exactly when SYN is exerted?

If possible, events will be merged into an 'atomic' event.

I'd love to know more about this...

RedBearAK commented 1 year ago

But everything seems to keep working as it normally does, so when/why would write() just be queuing events?

No idea.

joshgoebel commented 1 year ago

I assume it's queuing until the "untransformed" syns come thru directly from the real keyboard... which flushes the queue. Why that works so much better than flushing it explicitly after each keypress is what remains a mystery.

RedBearAK commented 1 year ago

Ah. Interesting. That triggered a thought.

I once again blocked all EV_SYN events. But, before restarting the keymapper I made sure to re-enable the sync call in both send_key_action and send_event.

If I understand things correctly, this means the only syn events are coming from the keymapper now, and none from the real keyboard.

After messing around for a few minutes, everything seems to be working fine.

No dialog focus issues.

No immediately apparent problems with macro reliability.

The "0, 0, 0" events are no longer in the log, but there is an object emitted by the sync function.

###### output.py - __send_sync:
    self=<keyszer.output.Output object at 0x7f90e95cfa60>

So this is one apparently viable way to do things. If it's better or worse seems to be kind of a toss-up at the moment, without more information. But if there were ever a possibility that queued write() events might get stuck or delayed if no "real" syn showed up at the right time from the real keyboard, I would assume that completely taking over the job of issuing syns like this would keep that from ever happening. (?)

joshgoebel commented 1 year ago

Yeah, that is the more "correct" approach IMHO, but really you'd also need some sort of state machine to account for KB + mouse devices (or running a separate mouse thru)... because you have events that are not EV_KEY events (like movement)... so if someone was using a keyboard/mouse we want to steal all their "keys" (and mouse buttons are keys, yes) and so we'd handle SYN for them, but since we pass thru the movement - we'd also need to pass thru SYN events (but only for movement!).

If we failed to do that you could only move the mouse if you hit a key on the keyboard soon after, or some ridiculousness like that... so really I think in a perfect world:

Right now we're doing both which, means sometimes we double up, which evidently some systems don't like.

RedBearAK commented 1 year ago

If we failed to do that you could only move the mouse if you hit a key on the keyboard soon after, or some ridiculousness like that...

Mmmm... not having any trouble with my touchpad movement or clicking. Or click-selecting things. Even if I don't mess with the keyboard at all for a while as I do touchpad stuff.

Not saying you're wrong, but I haven't experienced any issue so far.

so if someone was using a keyboard/mouse

Ah, so this would be a problem with a combined device (or, I assume, any other pointing device that happens to be "grabbed" during startup)? Yeah... I could see that being an issue. So for me it's not a problem because only my keyboard device is being grabbed.

Can syn events from a keyboard be easily distinguished from syn events from a pointing device? Looking at the event codes documentation it doesn't seem like there is a clear delineation between them.

👿 It's never easy.

Right now we're doing both which, means sometimes we double up, which evidently some systems don't like.

Yeah, so we're back to the best way seemingly being to just let the real keyboard send the syn events, allow them through, and not to bother with any artificial ones, which... seems to work fine at least 99.9% of the time. And fixes some problems.

joshgoebel commented 1 year ago

Can syn events from a keyboard be easily distinguished from syn events from a pointing device?

Events originate from a specific device. But in combo devices there isn't that delineation.

let the real keyboard send the syn events, allow them through, and not to bother with any artificial ones

No because we sometimes generate events that aren't in line with the keyboard... like if you hit CMD now, but we hit CMD a second later...(with hibernate) so we need the SYN now, not 1 second ago...

RedBearAK commented 1 year ago

No because we sometimes generate events that aren't in line with the keyboard... like if you hit CMD now, but we hit CMD a second later...(with hibernate) so we need the SYN now, not 1 second ago...

I'm not sure what this is referring to. Like if there is a sleep delay in an output macro?

The "real" syn events are often out of order, and almost always come out prior to the transformed EV_KEY events. Yet this doesn't seem to be causing any problems.

Can you give me an example of how I can deliberately cause a problematic delay between the real key press and the virtual output? I want to understand this better.

Wait, I feel like this should be a problem with long macros, since there is only one "real" combo of keys pressed and then a ton of virtual keystrokes to create the macro output. Yet I have no seen any kind of problem when testing macros like this:

keymap("General GUI", {
    C("RC-Shift-Alt-t"):        
        [
            C("Enter"),
            ST("##-0---------#####################################################---------0-##"),C("Enter"),
            ST("##-1-----------------------------------------------------------------------1-##"),C("Enter"),
            ST("##-2--------- This is a set of strings to test macro reliability. ---------2-##"),C("Enter"),
            ST("##-3-----------------------------------------------------------------------3-##"),C("Enter"),
            ST("##-4------------ The quick brown fox jumps over the lazy dog. -------------4-##"),C("Enter"),
            ST("##-5-----------------------------------------------------------------------5-##"),C("Enter"),
            ST("##-6--------- Now is the time for all good men to come to the aid ---------6-##"),C("Enter"),
            ST("##-7--------- of their country. Lines all the same length? Good!! ---------7-##"),C("Enter"),
            ST("##-8-----------------------------------------------------------------------8-##"),C("Enter"),
            ST("##-9----------------- Is the Unicode rose visible? Pass!! -----------------9-##"),C("Enter"),
            ST("##-0-----------------------------------------------------------------------0-##"),C("Enter"),
            ST("##-1---------#####################################################---------1-##"),C("Enter"),
            UC(0x1F339),C("Enter"),
            C("Enter"),
        ],

If the code has the artificial sync line blocked, shouldn't there be some sort of problem outputting all this?

joshgoebel commented 1 year ago

I'm not sure what this is referring to.

I mean suspend. Where you hit a key but we don't exert it until we know what you're doing - waiting to see if it might be part of a macro that doesn't actually include that key - so we don't "False" press it.

are often out of order, and almost always come out prior to the transformed EV_KEY events.

Yes, very strange.

Wait, I feel like this should be a problem with long macros

Exactly - and also surprising.

RedBearAK commented 1 year ago

I mean suspend.

Yeah, that's the one that usually comes out after the "0, 0, 0" event in the log, instead of before. Suspended keys. Meanwhile the release event syn is typically at the very end of the sequence of events, so maybe that's why it all keeps working.

I'm also wondering if the timestamp parts of the event are identical for the EV_KEY and the syn event that comes through before it, so it just ends up not really being an issue. Like out-of-order network packets getting pieced back together in the correct sequence. Or if, again, the syn event from the "release" is what pulls it all out of the fire and makes it appear to work. But press and release are really separate things, which is why the syns are normally needed... trailing off in confusion... 🤷🏽‍♂️

It's almost like the virtual keyboard keystrokes are somehow creating a syn event for each keystroke anyway, despite the lack of an explicit call. Yet blocking all the real syns without allowing any virtual syn calls definitely causes a huge problem.

Macros definitely come out fine without needing another real keystroke to "flush the queue". So... 🤷🏽‍♂️ 🤷🏽‍♂️

RedBearAK commented 1 year ago

@joshgoebel

Bad news. Shift-clicking and Cmd-clicking seem to be unreliable with the sync line disabled. Unless the modifier is held for a certain period of time, most likely corresponding to the repeat delay.

This is with the suspend time set to zero, so it seems that the modifier key press is not immediately recognized if there is no artificial syn involved. Holding for long enough for the key to start repeating will of course cause a "real" syn to come through, but then that's basically back to a long suspend time for a Mod+click to be recognized.

I wasn't certain what was going on at first, since this isn't the greatest keyboard. I thought maybe I was just not pressing the modifier key solidly enough, because the failure happens intermittently. But the issue goes away when re-activating the sync line. So it seems this is the effect of a missing syn at the right time, for the modifier press.

So this "fix" definitely has a down side. Don't know where to go from here.

The dialog focus issue can still be minimized by leaving the suspend time set to zero, or extremely short (0.01s). With the potential issues that implies.

RedBearAK commented 1 year ago

I'm going to close this PR since, as documented above, it turns out that removing the sync line is an ill-advised solution that makes Mod+clicking unreliable.

PR #134 is a much less invasive approach for fixing macro, hotkey and Unicode sequence glitches. Optional and user-guided injection of custom keystroke delays.