plover icon indicating copy to clipboard operation
plover copied to clipboard

Add config option for sleeping between key presses

Open oberien opened this issue 4 years ago • 15 comments

Summary of changes

On Linux (haven't tested other OS) Firefox seems to drop some key events if they are sent too quickly. One extreme example is stroking SER to get "certificate", followed by SRER to change it to "server". In Firefox, often times the beginning "c" or "ce" of "certificate" aren't deleted, which results in "ceserver". Sometimes a backspace key is even applied after "server" has been written, resulting in "ceserve" and similar.
To work around that problem, I implemented a config option, which allows setting a timeout in milliseconds, which is applied between all emulated keys, delaying the key events so that Firefox processes them correctly in the correct order. For me about 5 milliseconds seems to be the optimum.

Open Questions

  • [ ] Do I need to / how should I update the .pot file?
  • [ ] Is changing the KeyboardEmulation.{send_backspaces,send_string} functions in some os-implementations from static to non-static a breaking API change that needs to be documented for towncrier?
  • [ ] To not cause config-file problems, I added an if-condition to int_option, which sets the value to the default if value is None. This results in the config option added by this PR to default to its default value. Before doing this, I got an error message during startup of plover that the config option is missing in the config file. Is this an appropriate solution, or is there a different way that I should use for config-file migration?

Pull Request Checklist

  • [x] Changes have tests
  • [x] News fragment added in news.d. See documentation for details

oberien avatar Jun 20 '20 20:06 oberien

We need to test this on all platforms (I can do Mac and Windows pretty easily.) One consideration is to test a mixed-format translation like hi {#shift(t) h e r e} 👋{#shift(1)} which will cover normal key presses, Unicode, and keyboard shortcuts. I think we want the delay to be consistent between all of them?

morinted avatar Oct 23 '20 17:10 morinted

No problem on Linux. (including when two

Note that key_combo ({#keys}) are not delayed. This should be documented somewhere.

If the maximum is not specified, it defaults to 99. https://doc.qt.io/qt-5/qspinbox.html#maximum-prop (It would not makes a lot of sense for the value to be more than 99 anyway)

(perhaps IntOption should make this clearer with Python default parameter, or set some default that makes more sense (INT_MAX?), rather than relying on Qt's default behavior)

Nobody needs the delay value to be float, right?

It's usually not a problem, but the output is non-interruptible, even after Plover output is disabled.

user202729 avatar Oct 29 '20 03:10 user202729

Thanks for the review. I fixed all your comments.

Note that key_combo ({#keys}) are not delayed

I missed that. On OSX send_key_combination calls _send_sequence, which does already have the delay. On Linux X and Windows I added the delay to the send_key_combination function as well.

If the maximum is not specified, it defaults to 99.

I did not know that. For debugging it might make sense to allow values of over 1 second (e.g. to more easily see each emulated key press). I added a maximum delay of 100'000. I guess 100 seconds between each key press should be more than enough for everyone :)

Nobody needs the delay value to be float, right?

I think 1ms between key presses should be granular enough. 1ms delay + let's say 1ms code runtime per press is 500 keys per second. That's over 80 words per second(!) assuming English with 6 keys / word on average.

Edit: CircleCI failing shouldn't be my fault.

oberien avatar Oct 31 '20 15:10 oberien

More about the issue.

Even with this merged in, I still get terrible result with typing characters that needs keymap modifications in Chrome (on X). Obviously, because the key map is modified right before the key is typed in.

I see there are some ways to fix that:

  • Wait for X milliseconds between changing the layout and pressing the key. (time before changing layout: X, time after changing layout: X)
  • Change the layout before waiting the X milliseconds. (time before changing layout: X, time after changing layout: 0)
  • Wait for X/2 milliseconds between pressing the previous key and changing the layout, and X/2 milliseconds between changing the layout and pressing the next key. (time before changing layout: X/2, time after changing layout: X/2) -> Implemented in https://github.com/user202729/plover/commit/f4cb21c5f6d178b06340a77c68629d370d0b5944 .
  • Make another configurable option. (time before changing layout: X', time after changing layout: X)
  • Change the key map in mass as long as it fits in the table, and wait for X milliseconds after that.

Possible issues with all the options:

  • Confusing for the end user.
  • Make the delay between successive key presses uneven (only looks annoying).
  • Cause problem with other programs.
  • Make the total time to output things too slow.
  • Requires redesign of the algorithm. (actually I'm not entirely sure if the algorithm already does that)

user202729 avatar Feb 20 '21 00:02 user202729

I want this feature

petercpark avatar May 24 '21 22:05 petercpark

Instead of a uniform sleep between events, what if we just fill a buffer with all of the events and send them out at an even pace? It would be far less erratic, and farming that task out to another thread would also prevent the current thread from being blocked by sleep().

fourshade avatar Jun 22 '21 02:06 fourshade

Instead of a uniform sleep between events, what if we just fill a buffer with all of the events and send them out at an even pace? It would be far less erratic, and farming that task out to another thread would also prevent the current thread from being blocked by sleep().

What's the difference? Isn't "uniform sleep" the same as "even pace"?

user202729 avatar Jun 22 '21 02:06 user202729

What's the difference? Isn't "uniform sleep" the same as "even pace"?

What I mean is, instead of running "send event, sleep for x ms" every time an event comes in (and those come in unevenly), just add each event to a buffer. On another thread, we loop "if there's something in the buffer, pop the head and send it. then sleep regardless".

fourshade avatar Jun 22 '21 02:06 fourshade

What I mean is, instead of running "send event, sleep for x ms" every time an event comes in (and those come in unevenly), just add each event to a buffer. On another thread, we loop "if there's something in the buffer, pop the head and send it. then sleep regardless".

Actually, even though the (keyboard sending) thread is blocked, the behavior is exactly the same as would be in your suggested implementation.

If it wasn't, it would be a bug.

user202729 avatar Jun 22 '21 02:06 user202729

Actually, even though the (keyboard sending) thread is blocked, the behavior is exactly the same as would be in your suggested implementation.

If it wasn't, it would be a bug.

In theory, yes, it should be exactly the same. But it may not be in practice due to the way Python handles threads and blocking. It's probably worth a shot at least.

fourshade avatar Jun 22 '21 02:06 fourshade

When I use the feature it appears to be the same. Do you have weird behavior on your machine?

user202729 avatar Jun 22 '21 02:06 user202729

I haven't tried it on my machine; I was just thinking of a possible way to mitigate timing issues. send_string() is called by the engine's thread, and I just get a bad feeling (programmer spidey-sense?) when I see code that blocks the main thread like that. It may not matter on some or even most machines, but this bug is very machine/application sensitive.

fourshade avatar Jun 22 '21 03:06 fourshade

If this PR has lost some traction, I'd be happy to help with it. I've also implemented your suggestion locally, @fourshade, but at the moment I'm only running on Pop!_OS with an X server, so the quite extreme delays between stroke release and key output mean that I'm not certain if the extra thread is actually doing anything useful!

zeramorphic avatar Dec 02 '21 18:12 zeramorphic

Any updates on merging this?

alesya-h avatar Aug 27 '22 10:08 alesya-h

I most likely won't revisit this PR. Feel free to fork my state, rebase it, fix it and open a new PR. At that point I'll close this one.

oberien avatar Aug 27 '22 11:08 oberien