Refactor Keyboard Input Code, Improve Test Coverage, Fix Bugs #612

jgehrig · 2019-12-08T03:00:48Z

Several outstanding bugs are related to the way key input is handled. This code is difficult to modify and validate due to the usage of #ifdefs. Each platform has different behavior, and Unit Tests only validate the active platform.

This change does the following:

Get rid of all #ifdefs, to make the code easier to read.
Split platform specific behavior into separate compilation units.
Add UT coverage for all platforms, regardless of active platform.

I've also added fixes for Issue #593 and Issue #607.

Issue 607: The QKeyEvent for "<" contains a ShiftModifier. We need to remove this and send <lt> instead of <S-lt>.
Issue 593: Some versions of Qt can send Key_Super_L. We should ignore this event along with other modifier-only key events.
Issue 579: Cannot map <A-...> on MacOS. Keyboard layout "UnicodeHexInput" required.
Issue 510: Alt special key input does not work on MacOS. We are currently sending <A-...> instead of ....

jgehrig · 2019-12-10T05:42:17Z

I have seen this same issue before in Pull Request #573. Perhaps this is an unreliable UnitTest?

FAIL!  : NeovimQt::Test::connectToNeovimTCP() Compared values are not the same
   Actual   (c->errorCause())             : NoError
   Expected (NeovimConnector::SocketError): SocketError
   Loc: [/Users/travis/build/equalsraf/neovim-qt/test/tst_neovimconnector.cpp(65)]

Both changes trigger the issue, but don't touch neovimconnector in any meaningful way. Neither change shows any observable build or runtime issues. I am not sure what is happening here...

src/gui/input.cpp

equalsraf · 2019-12-11T22:56:21Z

I have seen this same issue before in Pull Request #573. Perhaps this is an unreliable UnitTest?
FAIL!  : NeovimQt::Test::connectToNeovimTCP() Compared values are not the same
   Actual   (c->errorCause())             : NoError
   Expected (NeovimConnector::SocketError): SocketError
   Loc: [/Users/travis/build/equalsraf/neovim-qt/test/tst_neovimconnector.cpp(65)]
Both changes trigger the issue, but don't touch neovimconnector in any meaningful way. Neither change shows any observable build or runtime issues. I am not sure what is happening here...

Reran a couple of times locally and in TravisCI:

locally in my Linux laptop it works
for this PR it keeps failing
on the master branch it succeeded

Aah the fun this will be ...

equalsraf · 2019-12-11T23:00:43Z

From #134 it seems that on Mac OS X I was seeing key: < with modifier Shift and text: <. This was a while back and probably in a pt_PT layout though.

jgehrig · 2019-12-11T23:11:57Z

Gahhh! The failure was deterministic for the SVG changes too, but only on MacOS. Very Strange.

I have MacOS VM that I can debug on. This will be interesting indeed...

equalsraf · 2019-12-12T00:23:06Z

Mmmm, there is also the possibility that there is some issue with CI. I have enabled daily builds for the master branch. I'll try to test it a bit too in #615

jgehrig · 2019-12-12T00:31:53Z

With the SVG changes, I could reproduce the issue locally on MacOS every time.

jgehrig · 2019-12-13T06:27:21Z

Based on looking at these failures in the debugger, my best guess is unreliable timeout/wait logic causes these failures.

In all cases NeovimConnector::connectToHost(...) is called, and executes correctly.

In the passing tests, a breakpoint at NeovimConnector::setError(...) is reached. In the failing tests, breakpoints set at NeovimConnector::setError(...) and NeovimConnector::discoverMetadata(...) are never reached.

I can make tst_neovimconnector pass with either hack below:

NeovimConnector* NeovimConnector::connectToHost(const QString& host, int port)
...
	s->connectToHost(host, port);
+	s->waitForConnected();
	return c;
}

    void connectToNeovimTCP() {
        ...
        // The signal might be emited before we get to connect
-        SPYWAIT(onError);
+        SPYWAIT(onError, 50000 /*msec*/);

        QCOMPARE(c->errorCause(), NeovimConnector::SocketError);
        c->deleteLater();
    }

Maybe we can improve the test reliability by adjusting timeout values...

There are also a few very slow tests; maybe this is a good opportunity to improve the test performance too?

jgehrig · 2019-12-15T06:16:25Z

@equalsraf

The last commit fixes the Travis error. AppVeyor is failing to download a nightly release of Neovim.

For Travies, MacOS takes ~50 seconds to timeout for the TCP socket sent to 127.0.0.1:64999 in some cases. Based on the debugger, we aren't touching anything in the execution path of this test.

Adding a 2-minute timeout resolves the issue, SPYWAIT(onError, 120000 /*msec*/);.

Do you have any opinions on such a solution?

Other that the long timeout, I don't like how Neovimconnect::m_error is initialized as NoError. It might be better to initialize it as NotConnected, and set QAbstractSocket::connected() to positively set NoError. Not sure how involved such a change would be...

The test could then become:

SPYWAIT(onError, 2500 /*msec*/);

QVERIFY(c->errorCause() == NeovimConnector::SocketError ||
    c->errorCause() == NeovimConnector::NotConnected);

jgehrig · 2019-12-15T06:19:40Z

I also improved the test performance. Only a handful of SPYWAITS were causing most of the spinlock. Manually commenting and lowering the timeout value for these cases seems worthwhile.

I can spin off these changes, or include them in their own commit with this Pull Request.

equalsraf · 2019-12-15T17:30:31Z

ACK I'm good with merging the fix along with this CR.

jgehrig · 2019-12-18T04:22:43Z

@equalsraf

That's all I've got for this PR, I think it is ready... Let me know if you feel it needs anything else :)

mikew · 2019-12-26T05:35:07Z

No control / command / meta combos are working for me with this PR. On macOS Catalina, running ...

nvim-qt -- -u NONE
:split
^w^w

... and the focus doesn't move between splits.

src/gui/input.cpp

jgehrig · 2019-12-26T16:01:51Z

Thanks @mikew!

MacOS is a difficult platform for me to debug/test on, since I don't own a Mac...

I would like to write a unit test for this scenario, so it isn't regressed in the future. Could you provide some debug output for a few of the key sequences in question?

This patch will provide debug output for the QKeyEvents I need to fix the issue and create UTs:

diff -r a4b43d295f61 src/gui/shell.cpp
--- a/src/gui/shell.cpp Mon Dec 16 02:23:57 2019 -0500
+++ b/src/gui/shell.cpp Thu Dec 26 10:58:19 2019 -0500
@@ -1070,6 +1070,8 @@
                this->setCursor(Qt::BlankCursor);
        }
 
+       qDebug() << "QKeyEvent ev:" << ev;
+
        const QString inp = Input::convertKey(ev->text(), ev->key(), ev->modifiers());
        if (inp.isEmpty()) {
                QWidget::keyPressEvent(ev);

jgehrig · 2020-01-09T02:28:34Z

#510 should be fixed. There is now some MacOS specific logic to remove the <A-...>, which is incorrectly sent with special character sequences.

#579 should be fixed now if the user selects a keyboard layout that does not conflict with the bindings, such as Unicode Hex Input.

@mikew Are you still able to look the code over, and give everything a quick test?

Several outstanding bugs are related to the way key input is handled. This code is difficult to modify and validate due to the usage of #ifdefs. Each platform has different behavior, and UTs only validate the active platform. This change does the following: 1) Get rid of all #ifdefs, to make the code easier to read. 2) Split platform specific behavior into separate compilation units. 3) Add UT coverage for all platforms, regardless of active platform. 4) Remove unused logic.

Some versions of Qt will send key events using Key_Super_L and Key_Super_R. We should handle these keys like other modifier keys. If the event contains only modifier keys it should be ignored. UTs added for ignoring various modifier key combination, as well as this specific scenario.

The Shift key modifier is redundant, and should be removed before sending the event to Neovim. The "<" should be sent as "<lt>".

A handful of tests are hitting SPYWAIT timeouts, causing dramatic slow-downs in the overall test performance. Comments have been added to these tests, and the timeout values adjusted for better performance.

The following test failure has been observed on MacOS: ``` FAIL! : NeovimQt::Test::connectToNeovimTCP() Compared values are not the same Actual (c->errorCause()) : NoError Expected (NeovimConnector::SocketError): SocketError Loc: [/Users/travis/build/equalsraf/neovim-qt/test/tst_neovimconnector.cpp(65)] ``` This issue occurs when the SPYWAIT times out before the TCP connection failure. Add a QVERIFY to catch the timeout, and increase timeout value to 2x the longest observed connection failure time on MacOS (~50s).

This statement is useful for writing Unit Tests, and for helping to debug input event issues. Having the code here, and commented out is useful for showing users how to print the correct information.

MacOS can send special characters when the Alt key is held. The Alt key modifier should be removed in these cases so the underlying character is sent as input. Before: Alt + A -> "<A-å>" After: Alt + A -> "å"

The input for uppercase letters is not well-formed. The Shift Key is implied for uppercase character, and should not be explicitly sent. Before: Shift + X: "<S-X>" After: Shift + X: "X"

The MacOS Alt behavior prevents simple key mappings like <A-x>. With this change, users can now map <A-...> with the proper keyboard layout, Unicode Hex Input. Before: Alt + X -> "" After: Alt + X -> "<A-x>"

jgehrig · 2020-01-22T03:36:32Z

@equalsraf This should be ready to go.

Everything I have tested looks reasonable on Mac/Linux/Windows. I am not aware of any outstanding issues.

equalsraf · 2020-01-23T23:42:48Z

Merged. Thanks @jgehrig. This is a tremendous amount of work.

jgehrig · 2020-01-24T00:02:50Z

Thanks for the kind words! :)

equalsraf · 2020-01-24T00:34:57Z

https://github.com/equalsraf/neovim-qt/graphs/contributors 👏

equalsraf reviewed Dec 11, 2019

View reviewed changes

src/gui/input.cpp Show resolved Hide resolved

jgehrig mentioned this pull request Dec 13, 2019

keymap russian-jcuken(win/mac) for russian capital BE (Б) does not work #608

Closed

jgehrig force-pushed the jg-input-ut-coverage branch 2 times, most recently from f79c289 to 17c27f2 Compare December 14, 2019 07:20

jgehrig force-pushed the jg-input-ut-coverage branch 3 times, most recently from 39e5d52 to acd46a6 Compare December 17, 2019 14:31

mikew reviewed Dec 26, 2019

View reviewed changes

src/gui/input.cpp Outdated Show resolved Hide resolved

jgehrig force-pushed the jg-input-ut-coverage branch from acd46a6 to a97edf4 Compare December 26, 2019 16:29

jgehrig mentioned this pull request Dec 27, 2019

Alt Gr does not work #510

Closed

jgehrig force-pushed the jg-input-ut-coverage branch 2 times, most recently from 186de4b to a97edf4 Compare December 28, 2019 15:25

jgehrig force-pushed the jg-input-ut-coverage branch 3 times, most recently from 08c5409 to 5e9790f Compare January 8, 2020 23:51

jgehrig force-pushed the jg-input-ut-coverage branch from 799c7ab to 68bdd38 Compare January 9, 2020 02:39

jgehrig force-pushed the jg-input-ut-coverage branch from 68bdd38 to 8ca3750 Compare January 9, 2020 02:42

jgehrig added 2 commits January 8, 2020 21:45

jgehrig force-pushed the jg-input-ut-coverage branch from 8ca3750 to 4f805ac Compare January 9, 2020 02:45

jgehrig added 7 commits January 9, 2020 10:46

Issue 607 Key "<" sends as <S-lt>

62487c6

The Shift key modifier is redundant, and should be removed before sending the event to Neovim. The "<" should be sent as "<lt>".

Improve test performance

5f2bb20

A handful of tests are hitting SPYWAIT timeouts, causing dramatic slow-downs in the overall test performance. Comments have been added to these tests, and the timeout values adjusted for better performance.

Add commented QKeyEvent qDebug statements

1ac82b7

This statement is useful for writing Unit Tests, and for helping to debug input event issues. Having the code here, and commented out is useful for showing users how to print the correct information.

Issue 510 Fix MacOS Alt special character input

dd30e10

MacOS can send special characters when the Alt key is held. The Alt key modifier should be removed in these cases so the underlying character is sent as input. Before: Alt + A -> "<A-å>" After: Alt + A -> "å"

Fix uppercase character input for convertKey

4bfe7af

The input for uppercase letters is not well-formed. The Shift Key is implied for uppercase character, and should not be explicitly sent. Before: Shift + X: "<S-X>" After: Shift + X: "X"

Issue 579 Cannot map <A-...> on MacOS

ac5d5fa

The MacOS Alt behavior prevents simple key mappings like <A-x>. With this change, users can now map <A-...> with the proper keyboard layout, Unicode Hex Input. Before: Alt + X -> "" After: Alt + X -> "<A-x>"

jgehrig force-pushed the jg-input-ut-coverage branch from 4f805ac to ac5d5fa Compare January 9, 2020 17:22

equalsraf merged commit e13251a into equalsraf:master Jan 23, 2020

jgehrig deleted the jg-input-ut-coverage branch January 24, 2020 05:55

justinmk mentioned this pull request Apr 28, 2020

vim-sneak does not register < character correctly (displays as <80><fc>^B<) justinmk/vim-sneak#249

Closed

jgehrig mentioned this pull request Aug 6, 2020

Key mapping not working #210

Closed

jgehrig mentioned this pull request Sep 24, 2020

^K<< doesn’t work, ^K< puts <S-lt> #755

Closed

jgehrig mentioned this pull request Nov 10, 2020

Unable to map < key in nvim-qt. But works on terminal nvim #790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Keyboard Input Code, Improve Test Coverage, Fix Bugs #612

Refactor Keyboard Input Code, Improve Test Coverage, Fix Bugs #612

jgehrig commented Dec 8, 2019 •

edited

Loading

jgehrig commented Dec 10, 2019

equalsraf commented Dec 11, 2019

equalsraf commented Dec 11, 2019

jgehrig commented Dec 11, 2019

equalsraf commented Dec 12, 2019

jgehrig commented Dec 12, 2019

jgehrig commented Dec 13, 2019

jgehrig commented Dec 15, 2019

jgehrig commented Dec 15, 2019

equalsraf commented Dec 15, 2019

jgehrig commented Dec 18, 2019

mikew commented Dec 26, 2019

jgehrig commented Dec 26, 2019

jgehrig commented Jan 9, 2020

jgehrig commented Jan 22, 2020

equalsraf commented Jan 23, 2020

jgehrig commented Jan 24, 2020

equalsraf commented Jan 24, 2020

Refactor Keyboard Input Code, Improve Test Coverage, Fix Bugs #612

Refactor Keyboard Input Code, Improve Test Coverage, Fix Bugs #612

Conversation

jgehrig commented Dec 8, 2019 • edited Loading

jgehrig commented Dec 10, 2019

equalsraf commented Dec 11, 2019

equalsraf commented Dec 11, 2019

jgehrig commented Dec 11, 2019

equalsraf commented Dec 12, 2019

jgehrig commented Dec 12, 2019

jgehrig commented Dec 13, 2019

jgehrig commented Dec 15, 2019

jgehrig commented Dec 15, 2019

equalsraf commented Dec 15, 2019

jgehrig commented Dec 18, 2019

mikew commented Dec 26, 2019

jgehrig commented Dec 26, 2019

jgehrig commented Jan 9, 2020

jgehrig commented Jan 22, 2020

equalsraf commented Jan 23, 2020

jgehrig commented Jan 24, 2020

equalsraf commented Jan 24, 2020

jgehrig commented Dec 8, 2019 •

edited

Loading