Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #22: Support for low-level key suppression #38

Merged
merged 15 commits into from Jan 19, 2017
Merged

Issue #22: Support for low-level key suppression #38

merged 15 commits into from Jan 19, 2017

Conversation

ghost
Copy link

@ghost ghost commented Dec 23, 2016

This resolves issue #22

@ghost
Copy link
Author

ghost commented Dec 25, 2016

I've timed this implementation and it appears to be well within the set limit of a few milliseconds. What currently needs to be done are two things:

  1. Suppression needs to be suspended for certain keys according to the timeout behavior.

@ghost
Copy link
Author

ghost commented Dec 26, 2016

  1. Users need to be able to remove key suppression with the remove_hotkey function. This will require handle tracking in the high-level API and table recompilation.

@ghost
Copy link
Author

ghost commented Dec 26, 2016

  1. Tests need to be adjusted to use the builtin press and release functions.
  2. advance needs to be set to False for key down events.

@ghost
Copy link
Author

ghost commented Dec 26, 2016

3 and 4 are complete.

@boppreh
Copy link
Owner

boppreh commented Jan 1, 2017

Hi xoviat

Is this feature ready? I tried testing with "ctrl+a" and the combo is still sent to other programs.

import keyboard
keyboard.add_hotkey('ctrl+a', lambda: print('trigger'), suppress=True)
keyboard.wait('esc')

Also, I'm having trouble understanding what is happening _suppress.py. For example, what exactly is _table? It seems to be a nested dictionary of tuples and further dictionaries, but that's a as far as I got. Maybe a more descriptive name, or some comment explaining its function?

@boppreh
Copy link
Owner

boppreh commented Jan 1, 2017

(I was typing a longish comment and pressed some hotkey that made github refresh the page, losing my text. Oh, the irony.)

One more thing I don't understand is the timeout support.

Timeout was added for hotkeys because multi-step combinations are stateful, with no way for the user to know in which step they are in. Imagine having a hotkey a, b, pressing "a", going for a coffee, then coming back and pressing "b". That's surprising behavior and should be avoided.

On the other hand, if there is a a+b hotkey, the user would have to place a rock on the "a" key. In that case it's perfectly acceptable to trigger the hotkey when they press "b", because what matters is that both keys were depressed together at one point in time.

However on your code I see that "multistep combinations are currently unsupported", yet you still went the extra length to support timeouts. Could you give me some insight into this?

@ghost
Copy link
Author

ghost commented Jan 1, 2017

However on your code I see that "multistep combinations are currently unsupported", yet you still went the extra length to support timeouts. Could you give me some insight into this?

This was from an earlier commit. Multistep combinations should now be supported.

@ghost
Copy link
Author

ghost commented Jan 1, 2017

@boppreh

The problem is currently that the suppression table is set to suppress the "ctrl" key, but it receives the "left control" key. It works with the following patch:

        if key in ('left control', 'right control'):
            key = 'ctrl'

Obviously this is not going to actually work; I'll need to find out how the keys are mapped and then convert the names.

@ghost
Copy link
Author

ghost commented Jan 1, 2017

On the other hand, if there is a a+b hotkey, the user would have to place a rock on the "a" key. In that case it's perfectly acceptable to trigger the hotkey when they press "b", because what matters is that both keys were depressed together at one point in time.

This is also not consistent with my implementation, but it will be trivial to fix.

@ghost
Copy link
Author

ghost commented Jan 1, 2017

Also, I'm having trouble understanding what is happening _suppress.py. For example, what exactly is _table? It seems to be a nested dictionary of tuples and further dictionaries, but that's a as far as I got. Maybe a more descriptive name, or some comment explaining its function?

KeyTable._keys is constant until the the set of keys to suppress is changed. The entire KeyTable class is based on a recursive model, which makes it fast and very confusing at the same time. Each time a key is pressed, is_key_allowed checks KeyTable._keys and KeyTable._table and based on the results reassigns KeyTable._table to point to a different location in the KeyTable._key dictionary. I struggled with the names and came up with these, but they aren't very good.

@ghost
Copy link
Author

ghost commented Jan 1, 2017

One major oversight on my part is that the keys need to be re-input if the key combination is not completed. I should have that up by tomorrow.

@boppreh
Copy link
Owner

boppreh commented Jan 2, 2017

It occurred to me that _table is a finite state machine, which makes it the perfect model for quick processing of inputs.

@ghost
Copy link
Author

ghost commented Jan 3, 2017

Well, I say it's done now, as in I couldn't find any faults, but let's see.

@ghost
Copy link
Author

ghost commented Jan 3, 2017

One other thing to consider is that it does appear that keys pressed by the high-level API are not exempt from key suppression, something that may need to be changed.

@glitchassassin
Copy link
Collaborator

If this is low-level, I assume that means it's a global "always on" suppression, right? Is there a way to limit it to "while the program has focus"?

@boppreh
Copy link
Owner

boppreh commented Jan 3, 2017

@glitchassassin dealing with programs and windows is another entirely different can of worms. I know you are looking for a drop-in replacement for AutoHotkey, but this library is for low level input only. This does not preclude another library from using it to implement this kind of missing feature, however.

My litmus test for new features is: "does this make sense in a Windows box, Linux box, and a headless Raspberry Pi"?

@xoviat I'm taking a look at the code and will merge or come back with feedback shortly. Again, thank you for your excellent contribution.

@glitchassassin
Copy link
Collaborator

Low-level support works fine for my use case, no worries there. I'm just curious about the scope.

@ghost
Copy link
Author

ghost commented Jan 19, 2017

@boppreh I hate to push, but what is the current status of this?

@boppreh
Copy link
Owner

boppreh commented Jan 19, 2017

It's your contribution, you have every right to push.

Current status is as follows. I got a new test machine, more locked down. Tried vanilla keyboard (without your patch) and results were not good. Many small errors, and just plainly unreliable.

I'm trying to get to the bottom of this before adding more complexity. I believe I'll have some free time today, hopefully I'll be able to fix the small issues and then apply your patch later tonight.

@boppreh boppreh merged commit d3bf224 into boppreh:master Jan 19, 2017
@boppreh
Copy link
Owner

boppreh commented Jan 19, 2017

The deed is done.

It is not at quite the level I want yet. But suppress defaults to False, and I can delay releasing a new version, so any problems are not as serious.

First, there were a few small bugs that I fixed myself. For example _winkeyboard.py->process_key had a path that just returned, without any values, throwing an error on low_level_keyboard_handler that expected a pair of values. Then there was the treatment of left and right on key names, which naively split the key name and took the last word (this breaks horrendously on multi-word keys, like page up, which also now clashes with the key up).

But those bugs were small and easy to fix.

The next issue is that keys that were not released don't seem to count toward the next hotkey suppression. For example, try adding a hotkey for ctrl+a, then holding ctrl while you press a repeatedly. The first time it's suppressed correctly, but not the following ones. I'm not sure this is easy to fix, and will almost certainly add delay (it'll have to re-check an arbitrarily long chain of keys before).

The last and largest issue is Linux support of key suppression, but that's another whole can of worms.

Still, good job. That was a highly requested, complicated task, and I loved your FSM implementation with nested-tables. I'm definitely stealing the idea for future projects.

Thank you very much for your contribution xoviat. If you have any issues about how I merged or changed your code, feel free to complain and I'll do my best to appease your worries.

@ghost
Copy link
Author

ghost commented Jan 19, 2017

The next issue is that keys that were not released don't seem to count toward the next hotkey suppression. For example, try adding a hotkey for ctrl+a, then holding ctrl while you press a repeatedly. The first time it's suppressed correctly, but not the following ones. I'm not sure this is easy to fix, and will almost certainly add delay (it'll have to re-check an arbitrarily long chain of keys before).

It seems like it would need only to check the previous key, which would probably slow it down by a third. Unfortunately, I probably won't have time to fix that issue because I've been struggling with pywin32 (which is both widely used and is about to be serious trouble due to deprecation).

@boppreh
Copy link
Owner

boppreh commented Jan 19, 2017

I'm not sure it's only the previous key. The user could very well shortcut ctrl+shift+alt+a, or even a+b+c+d....

But don't worry, you've contributed enough already.

boppreh added a commit that referenced this pull request Aug 14, 2018
The first version that could suppress events was introduced by @xoviat
via #38. It was a separate module, using nested dictionaries to simulate
a state machine that had to be updated on every hotkey registered. It
also handled the entire hotkey process, including multi-steps and
timeouts. It was very complex, and had some bugs that we weren't sure
were fixable with that architecture.

The second version was developed in the `suppress` branch for over an
year. It used an explicit state machine for each key, divided by several
attributes (is it a modifier, was it processed by a key, etc). It
processed only single-step hotkeys. Multi-step hotkeys were simulated by
adding and removing hotkeys as necessary, and keeping track separately of
which events were suppressed from previous steps. This version worked
better, with fewer bugs and somewhat simpler, but the execution was
still hard to understand and some bugs were creeping in.

This commit introduces the third attempt at making a resiliant and
simple to understand key event suppression system. It borrows the
concept of multi-step hotkeys as being sequences of single-step hotkeys,
but revamps everything else.

The meat of the code is a set of global variables and a decision tree.

The decision tree is executed for each incoming event, and uses the
global state to decide if a given event should be:
1) Suppressed. It's part of a blocking hotkey and at least one callback
returned False. The event is blocked with no hope of recovery. For key
down events, the corresponding key up event will also be suppressed.
2) Delayed. This event is part of a *subset* of one or more blocking
hotkeys. We block it now, but may decide to resend it later.
3) Allowed. The event is passed along normally. If there were any
pending events, they are resent before allowing this one.

The global variables keep track of which keys the OS reports as
currently pressed (i.e. physically pressed keys), which keys were
allowed to be passed (i.e. logically pressed keys, from the point of
view of other applications), which key presses have been tentatively
suppressed, and which key presses were definitely suppressed by hotkeys.

It's still complex, but much easier to understand than the two previous
versions, doesn't have the same bugs (I hope it has no bugs at all), and
is much easier to debug and amend.

Third time is the charm, wish me luck.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants