remove the extra delay to repeat after pressing and holding a mapped key

[NeKJ]

When pressing and holding down the hyperkey and then pressing and holding a mapped key there is a small delay until the mapped keycode is emitted.

This is most evident with the Delay Interval before starting the Key Repeating. i.e.:

Assuming a configured system Interval Key Delay of 250ms.

1. press and hold down an arrow key, notice the time it takes to start repeating.
2. now press and hold the hyperkey of touchcursor
3. while holding the hyperkey, press and hold the same arrow key as above, to start repeating

expected result: One key event is emitted and the cursor moves once immediately, then the repeating events of the key start after the system's Interval Key Delay e.g. 250ms.

actual result: NO key event is emitted and the cursor doesn't move at all. Repeating key events start a noticeable bit longer than the system's Interval Key Delay. e.g., 300-400ms instead of 250ms.

This PR removes this extra delay and is on par as with the real keys.

[Adda0]

Hello, thank you for the PR. Thinking about it now, the expected behaviour makes sense and mapped keys should indeed behave as described. I will inspect further and test the introduced change in the following days. Have a good one.

[donniebreve]

I'm pretty sure this breaks some of the test cases. The delay state is there to handle some quick transitions in and out of the hyper state.

[Adda0]

I thought so, too. Surprisingly, all current tests pass and I did not encounter any weird behaviour with fast typing while I was testing the change. Of course, it changes the emitting logic in a way that now we would emit a key immediately after pressing both hyper and mapped key down, instead of waiting for releasing the key / keeping mapped key pressed down.

I think it is a reasonable issue and request for improvement. However, there is no way to better handle this than emitting a key immediately after pressing both hyper and mapped key down. That is caused because at this time, we cannot know whether a hyper key will be released before the mapped key or only after the mapped key is released.

As far as I can tell, this is either this or nothing. When I tried to implement this behaviour in my C++ port, I have a test for fast typing where the change showed in a now failing test. Touchcursor firstly emits a mapped binding, only after that (with the next key event) realizes a hyper key is released before a mapped key and emits both hyper and mapped keys as if there was no mapping (fast typing with overlapping keys).

Depends on what approach we prefer, I would say. Do we want to emit hyper bindings every time we have a chance and, for example, skip emitting normal keys when the hyper key is released before a mapped key and behave as expected with holding mapped keys down? Or do we want to keep the delay, emit only after we know a hyper key is released only after a mapped key is / mapped key is kept pressed down, but get a clearly delayed response for the first hyper binding emit?

[NeKJ]

So there is a case where this is not working well. Let's try to find a solution for that too.

This is what I understand is the issue:

1. pressing and holding hyper key
2. then, pressing and holding a mapped key and immediately after that, release the hyper key

expected result:

1. touchcursor emits mapped key in DOWN state
2. stops and emits nothing else

actual result:

1. touchcursor emits mapped key in DOWN state
2. then at the next key event, touchcursor emits hyper UP, and original key DOWN state.

Did I get that right? If yes, then the fix should involve fixing the last wrong 2 emits.

[Adda0]

Not exactly. Although, you have the general idea right. The current behaviour without your PR is implemented in such a way to prevent emitting a hyper binding if hyper key is not pressed down for the whole time of pressing mapped keys down and releasing mapped keys again. That prevents Touchcursor from emitting hyper bindings in so-called “fast typing with overlapping keys” for hyper key and some mapped keys.

The problem is, we cannot know at the time of pressing a mapped key down for the first time which key event will come next: hyper key up (currently, Touchcursor should not bind a hyper binding to a hyper + mapped keys down) or mapped key up (Touchcursor should map a hyper binding instead of emitting hyper key and mapped key events).

Therefore, if we want to implement your feature, the logic would change as follows: we would immediately emit a hyper binding, after only pressing hyper key down and mapped key down, without regarding the following key events. Your change implements expected behaviour as it is for normal typing without Touchcursor. However, we have to consider whether it would inconvenience the user if suddenly, after normally typing (not wanting to emit a hyper binding), they would get a hyper binding emitted instead of, for example, space and j, because just before they released space bar, they already pressed j, which, in my experience, might happen for normal keyboard users somewhat often.

Current tests all pass. The failing test in my port is the following:

        SECTION("Fast typing with overlapping key events") {

            SECTION("Space down, mapped down, space up, mapped up") {
                // The mapped keys should not be converted.
                expected = "57:1 36:1 57:0 36:0 ";
                type(config, 8, KEY_SPACE, EVENT_KEY_DOWN, KEY_J, EVENT_KEY_DOWN, KEY_SPACE, EVENT_KEY_UP, KEY_J, EVENT_KEY_UP);
                REQUIRE(expected == outputString);
            }
        }

We should, by our original logic, get emitted events 57:1 36:1 57:0 36:0 for typed sequence of KEY_SPACE, EVENT_KEY_DOWN, KEY_J, EVENT_KEY_DOWN, KEY_SPACE, EVENT_KEY_UP, KEY_J, EVENT_KEY_UP (format KEY,UP/DOWN, ...). Instead, we get 105:1 57:1 36:1 57:0 36:0 with this PR included. That is, we get the hyper binding down (105:1) emitted (and never get the release event for this hyper binding), then realize that hyper key was released first and proceed as by our logic dictates: do not convert the hyper + mapped key to a hyper binding, instead emit their original key events.

This is, however, a modified version of Touchcursor, as I had to change a few emits here and there to support more than one hyper key, with separate and/or common bindings, permanent remappings (as in #47) and more. Therefore, it is possible that this output is unique to the C++ port and does not exist in this original version. Nevertheless, the logic would have to change. I thought it is alright with this Touchcursor version as I have not encountered similar issues here, but I may be wrong.

If we want to proceed with the change in emitting logic, this should be something to consider. The change in logic is possible, but it is either this (with fixed emitted key events: only hyper bindings down and up, instead of hyper binding down and original key events for both hyper and mapped key) or we keep the logic as is, without ability to remove the delay, nothing in between, due to the lack of knowledge of user's next action (next key event).

[NeKJ]

Well, I can't reproduce this in the "real world". Sure enough, if I deliberately press the keys in the wrong way it indeed will produce this undesired behavior. But I don't see why the software should mitigate for something that is wrong itself in the first place: pressing the keys with the wrong order and in the wrong timing.

I am a relatively fast typer, and I usually type at 100WPM (up to ~120). I used monkeytype.com taking the same test twice, once with TC (with this PR applied) running and once with it stopped and I got the same WPM, without any issues or misspress/missed keys caused by the aforementioned "use case". In fact I need to deliberately try many times to even make this "misstype" happen.

Are there actual use cases, where this becomes an issue in the real world? Go ahead, try it yourself using monkeytype, or whichever other typing test you like best.

[NeKJ]

Update to the previous post: I was wrong, I can't reproduce this behavior at all. I did a test by pressing and releasing both the hyper+i (mapped to up arrow) together, as simultaneously as possible and extremely rapidly repeating this a lot of times. In all cases, the correct key was emitted. Either the hyper was released before the "i" key and as expected, the keys SPACE and "i" were emitted, or the UP ARROW key was emitted, if the hyper was released after the "i" key. I tested this on both a PS/2 keyboard (real PS/2 connection) and a mechanical USB one. They both gave the same results. So no, it doesn't seem that I can actually reproduce this issue at all.

However, I noticed a different issue that possibly may be somehow related: for some of those simultaneous 2-key press-and-immediately release, after release, the TC was "stuck" in the "hyper" state. Maybe this is the issue we are talking about? or is this another one?

[donniebreve]

// normal typing
[sd, md, mu, su] failed. expected: '105:1 105:0 ', output: '105:1 105:1 105:0 '

// fast typing (this is the one I figured would fail)
[sd, md, su, mu] failed. expected: '57:1 36:1 57:0 36:0 ', output: '105:1 57:1 36:1 57:0 36:0 '

My goal for this project is to be as nonintrusive as possible during normal typing. Some of the other projects I tried before (including the original) would sometimes give me undesired keys if I typed too quickly. I consider myself a proficient typist, no where near @NeKJ's 100WPM :smiling_face_with_tear:, but this output would bother me.

Even so... I guess we could add an option to skip the delay state of the state machine? I might be open to adding a configuration option to get the behavior you want @NeKJ, I just don't think it should be the default behavior. I have had a feeling in the back of my mind that the state machine needs some cleanup anyway... someday.

[donniebreve]

@NeKJ here's a test you can do:

Change your configuration to this:

[Hyper]
HYPER1=KEY_SPACE 

[Bindings]
KEY_Q=KEY_BACKSPACE
KEY_W=KEY_BACKSPACE
KEY_E=KEY_BACKSPACE
KEY_R=KEY_BACKSPACE
KEY_T=KEY_BACKSPACE
KEY_Y=KEY_BACKSPACE
KEY_U=KEY_BACKSPACE
KEY_I=KEY_BACKSPACE
KEY_O=KEY_BACKSPACE
KEY_P=KEY_BACKSPACE
KEY_A=KEY_BACKSPACE
KEY_S=KEY_BACKSPACE
KEY_D=KEY_BACKSPACE
KEY_F=KEY_BACKSPACE
KEY_G=KEY_BACKSPACE
KEY_H=KEY_BACKSPACE
KEY_J=KEY_BACKSPACE
KEY_K=KEY_BACKSPACE
KEY_L=KEY_BACKSPACE
KEY_Z=KEY_BACKSPACE
KEY_X=KEY_BACKSPACE
KEY_C=KEY_BACKSPACE
KEY_V=KEY_BACKSPACE
KEY_B=KEY_BACKSPACE
KEY_N=KEY_BACKSPACE
KEY_M=KEY_BACKSPACE
KEY_COMMA=KEY_BACKSPACE
KEY_PERIOD=KEY_BACKSPACE

and see if your Monkeytype score goes down. You should see a lot of unwanted character deletions.

[Adda0]

// normal typing [sd, md, mu, su] failed. expected: '105:1 105:0 ', output: '105:1 105:1 105:0 ' // fast typing (this is the one I figured would fail) [sd, md, su, mu] failed. expected: '57:1 36:1 57:0 36:0 ', output: '105:1 57:1 36:1 57:0 36:0 '

Surprisingly enough, these tests pass on my end.

I have had a feeling in the back of my mind that the state machine needs some cleanup anyway... someday.

As I have been implementing Touchcursor in C++, I have a similar feeling. In general, the state machine could use a bit of documentation, cleaning and/or revision. One day.

The failures might be minimal and occur only in some specific cases. However, as the project mission is to be as unintrusive as possible, you are right about not wanting to emit unnecessary modified bindings as possible unless Touchcursor can be absolutely sure you want to emit a key. Therefore, I like the idea of optional settings in the config file of this character. This might indeed be something worth exploring.

@NeKJ here's a test you can do:

There is the problem I have tried to describe above visible. Both approaches are theoretically correct. The difference is that the original Touchcursor emits a hyper key bind only after the mapped key was pressed and released, instead of emitting immediately after the mapped key is pressed, as it is the case in this PR. If we want to preserve the unintrusiveness, we would need to “hide” the immediate emit behind an additional option in the config file.

[NeKJ]

Indeed, I did a bunch of tests with your configuration and in some of them I got the "backspace" emission. So, yes, definitely there is something not going perfectly well here. However, I'm pretty confident that this is a bug that can be fixed. Maybe a careful reviewing of the state machine might be in order. Anyway for the record here is my results:

With this PR and this configuration:

# Touch Cursor Linux configuration file

# Find this line using 'cat /proc/bus/input/devices | less' and copy the full Name="" line here
[Device]
Name="Soarer Soarer's Keyboard Converter Keyboard"
#Name="GASIA PS2toUSB Adapter"
#Name="USB Keyboard"
#Name="Matias Matias Keyboard"

# Hyper key
[Hyper]
HYPER1=KEY_SPACE

# KEY_1=KEY_2
# https://github.com/torvalds/linux/blob/master/include/uapi/linux/input-event-codes.h
[Bindings]
# # Default bindings for IJKLHNUOMPY
# KEY_I=KEY_UP
# KEY_J=KEY_LEFT
# KEY_K=KEY_DOWN
# KEY_L=KEY_RIGHT
# KEY_H=KEY_PAGEUP
# KEY_N=KEY_PAGEDOWN
# KEY_U=KEY_HOME
# KEY_O=KEY_END
# KEY_M=KEY_DELETE
# KEY_P=KEY_BACKSPACE
# KEY_Y=KEY_INSERT

KEY_Q=KEY_BACKSPACE
KEY_W=KEY_BACKSPACE
KEY_E=KEY_BACKSPACE
KEY_R=KEY_BACKSPACE
KEY_T=KEY_BACKSPACE
KEY_Y=KEY_BACKSPACE
KEY_U=KEY_BACKSPACE
KEY_I=KEY_BACKSPACE
KEY_O=KEY_BACKSPACE
KEY_P=KEY_BACKSPACE
KEY_A=KEY_BACKSPACE
KEY_S=KEY_BACKSPACE
KEY_D=KEY_BACKSPACE
KEY_F=KEY_BACKSPACE
KEY_G=KEY_BACKSPACE
KEY_H=KEY_BACKSPACE
KEY_J=KEY_BACKSPACE
KEY_K=KEY_BACKSPACE
KEY_L=KEY_BACKSPACE
KEY_Z=KEY_BACKSPACE
KEY_X=KEY_BACKSPACE
KEY_C=KEY_BACKSPACE
KEY_V=KEY_BACKSPACE
KEY_B=KEY_BACKSPACE
KEY_N=KEY_BACKSPACE
KEY_M=KEY_BACKSPACE
KEY_COMMA=KEY_BACKSPACE
KEY_PERIOD=KEY_BACKSPACE

quick test results from monkeytype.com:

[donniebreve]

Both approaches are theoretically correct.

I disagree. Other projects solve this with timers, but this project determines when you really want to be in the "layer" using a state machine (as does the original). That means you have to do something physical to move the state machine along. Ultimately, timers may be the way to go for user experience. I started writing this project with a timer... but when issues arose, I looked at the original and thought the state machine was the key. Now I'm starting to think that the key to proper behavior is the Queue.

In any case, as is, I will not be accepting this PR, and after further thought, I don't really like the idea of making this configurable either. @NeKJ I suggest reducing your key repeat timeout if you are using arrow keys for large movements.

[Adda0]

I went back to look at this PR. I have tried again to implement this behaviour and must agree with @donniebreve that for the current implementation using state machine it is not possible to “predict” what the user wants to be typed out. Hence, I agree with the decision to close this PR. Unless we change the state machine, this feature cannot work without possibly emitting a wrong sequence of key codes.