Issue 720: Spanish Keyboard Layout Accents

[jgehrig]

Issue #720: The [ character is not handled properly on Spanish Keyboard Layouts.

The Spanish Layout contains keys like ` and &. These keys add an accent to the next character typed.

For example:

QKeyEvent ev: QKeyEvent(KeyPress, Key_QuoteLeft, ShiftModifier, text="^") // Ignored, add accent to next key
QKeyEvent ev: QKeyEvent(KeyPress, Key_E, text="è")

Supporting this input mode will be interesting and may not be possible without adding state to NeovimQt::Input::convertKey.

[codecov-commenter]

Codecov Report

Merging #721 into master will increase coverage by 0.07%. The diff coverage is 86.11%.

@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
+ Coverage   20.98%   21.05%   +0.07%     
==========================================
  Files          73       73              
  Lines       28100    28134      +34     
==========================================
+ Hits         5896     5925      +29     
- Misses      22204    22209       +5     

Impacted Files	Coverage Δ
src/gui/shell.cpp	43.54% <0.00%> (-0.09%)	:arrow_down:
src/gui/input.cpp	77.50% <81.25%> (+0.10%)	:arrow_up:
test/tst_input_common.cpp	100.00% <100.00%> (ø)
test/tst_input_mac.cpp	100.00% <100.00%> (ø)
test/tst_input_unix.cpp	100.00% <100.00%> (ø)
test/tst_input_win32.cpp	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5038ad1...8b44af0. Read the comment docs.

[codecov-io]

Codecov Report

Merging #721 into master will increase coverage by 0.10%. The diff coverage is 91.30%.

@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
+ Coverage   21.17%   21.28%   +0.10%     
==========================================
  Files          73       73              
  Lines       28137    28181      +44     
==========================================
+ Hits         5958     5998      +40     
- Misses      22179    22183       +4     

Impacted Files	Coverage Δ
src/gui/shell.cpp	43.98% <0.00%> (-0.09%)	:arrow_down:
src/gui/input.cpp	91.46% <87.50%> (-0.54%)	:arrow_down:
test/tst_input_common.cpp	100.00% <100.00%> (ø)
test/tst_input_mac.cpp	100.00% <100.00%> (ø)
test/tst_input_unix.cpp	100.00% <100.00%> (ø)
test/tst_input_win32.cpp	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 83b39c3...6218900. Read the comment docs.

[jgehrig]

@alfredobarroso

Sorry for the huge delay here. Is there any chance you could do some testing with the current iteration?

I am not familiar with these keyboard layouts... It is working to the best of my knowledge (and my knowledge is not much haha). Each OS has slightly different behavior here. It would be great to get a sanity check from someone familiar with these keys.

Could you pay explicit attention to [ and ] on MacOS?

I observed the following non-working key events: https://github.com/equalsraf/neovim-qt/blob/6218900952c2b45e134ccb506b4e14b060a908ec/test/tst_input_common.cpp#L291-L297

However, this could just be a layout mis-configuration... Linux/Windows behave differently from my MacOS Spanish layout.

[alfredobarroso]

@alfredobarroso

Sorry for the huge delay here. Is there any chance you could do some testing with the current iteration?

I am not familiar with these keyboard layouts... It is working to the best of my knowledge (and my knowledge is not much haha). Each OS has slightly different behavior here. It would be great to get a sanity check from someone familiar with these keys.

Could you pay explicit attention to [ and ] on MacOS?

I observed the following non-working key events: https://github.com/equalsraf/neovim-qt/blob/6218900952c2b45e134ccb506b4e14b060a908ec/test/tst_input_common.cpp#L291-L297

However, this could just be a layout mis-configuration... Linux/Windows behave differently from my MacOS Spanish layout.

Sadly I don't have access to my Macbook. My country has limited mobility due to COVID so until the restrictions are lifted I can't pick it up :(

[jgehrig]

Sorry to hear that :(

I can relate. Here in the Northeastern States we were hit hard early and had aggressive shutdowns. No fun, strange times...

Let me know when you're able to test. The changes are ready for check-in pending validation. Any additional patching will be quick and easy.

[jgehrig]

@alfredobarroso

Friendly ping. I hope the COVID situation has improved for you!

[codecov-commenter]

Codecov Report

Merging #721 (bfae2aa) into master (1151c6d) will decrease coverage by 0.65%. The diff coverage is 93.05%.

:exclamation: Current head bfae2aa differs from pull request most recent head cae0636. Consider uploading reports for the commit cae0636 to get more accurate results

@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
- Coverage   22.75%   22.09%   -0.66%     
==========================================
  Files          81       74       -7     
  Lines       29031    28385     -646     
==========================================
- Hits         6606     6273     -333     
+ Misses      22425    22112     -313     

Impacted Files	Coverage Δ
src/gui/shell.cpp	44.96% <0.00%> (-6.97%)	:arrow_down:
src/gui/input.cpp	91.71% <82.35%> (-1.41%)	:arrow_down:
src/gui/input_mac.cpp	87.80% <100.00%> (+0.30%)	:arrow_up:
test/tst_input_mac.cpp	100.00% <100.00%> (ø)
test/tst_input_unix.cpp	100.00% <100.00%> (ø)
test/tst_input_win32.cpp	100.00% <100.00%> (ø)
src/gui/treeview.cpp	44.18% <0.00%> (-17.72%)	:arrow_down:
src/gui/shelloptions.h	87.50% <0.00%> (-12.50%)	:arrow_down:
src/gui/scrollbar.cpp	58.33% <0.00%> (-7.44%)	:arrow_down:
... and 38 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1151c6d...cae0636. Read the comment docs.

[jgehrig]

@alfredobarroso

Do you have the ability to test this on MacOS again?

This should be ready for merge, but I'm not familiar with the Spanish keyboard layout. It would be good to have a native user test this before I hit the merge button.

Thanks!

[alfredobarroso]

Hi @jgehrig

Sorry for the HUGE delay. I've been away from home for most of the year without any access to a Mac. Now I'm back at home for a few days; I'll test it this afternoon (about 12 hours from now) and let you know.

[alfredobarroso]

Hi @jgehrig

I've tested again and some of the key combinations that failed on my previous tests are now working, but other seems to still be broken:

Key Combination	Expected	Obtained	Result
Option + '+'	]	]	OK
Option + 'ç'	}	}	OK
Option + '1'	Pipe	Pipe	OK
Option + '2'	@	@	OK
Option + '6'	¬	¬	OK
Option + '`'	[	ESC	FAIL
Option + '´'	{	ESC	FAIL
Option + '3'	#	ESC	FAIL
Option + 'º'	\	ESC	FAIL

'ESC' means that it behaves like pressing escape; it puts you into normal mode.

I got weird messages when compiling so I'm not 100% sure this is not a problem with my compilation.

I'm taking the Mac this time so count on me for any retests.

[jgehrig]

@alfredobarroso

Excellent. Thank you very much!

That table is very helpful. One quick ask: can you add a QKeyEvent column?

I just modified the PR to include some additional diagnostics for Debug builds (-DCMAKE_BUILD_TYPE=Debug).

Additional Table Column:

Key Combination	Expected	Obtained	Result	QKeyEvent
Option + '+'	]	]	OK	???
...	...	...	...	...

The QKeyEvent info is helpful both in fixing the FAIL cases and in adding test coverage for all cases.

Alternatively, if it is too hard to pick out the specific events, you can copy/pipe the output to a Logs.txt file and upload it.

---

'ESC' means that it behaves like pressing escape; it puts you into normal mode.

With the new diagnostics, it should be easier to identify the specific key; Very likely <Esc>.

I got weird messages when compiling so I'm not 100% sure this is not a problem with my compilation.

Let's see the build logs, just to be extra cautious. You should be able to drag/drop the log file into your comment.

Can you copy-paste the logs into a *.txt file?

I'm taking the Mac this time so count on me for any retests.

Awesome! Thanks :+1:

Hopefully we can get this fixed quickly.

[alfredobarroso]

Here's the table with the QKeyEvent data:

Key Combination	Expected	Obtained	Result	QKeyEvent
Option + '+'	]	]	OK	QKeyEvent ev: QKeyEvent(KeyPress, Key_Plus, AltModifier, text="]") "]"
Option + 'ç'	}	}	OK	QKeyEvent ev: QKeyEvent(KeyPress, Key_Ccedilla, AltModifier, text="}") "}"
Option + '1'	|	|	OK	QKeyEvent ev: QKeyEvent(KeyPress, Key_1, AltModifier, text="|") "|"
Option + '2'	@	@	OK	QKeyEvent ev: QKeyEvent(KeyPress, Key_2, AltModifier, text="@") "@"
Option + '6'	¬	¬	OK	QKeyEvent ev: QKeyEvent(KeyPress, Key_6, AltModifier, text="¬") "¬"
Option + '`'	[	ESC	FAIL	QKeyEvent ev: QKeyEvent(KeyPress, Key_BracketLeft, AltModifier, text="[") "<A-[>"
Option + '´'	{	ESC	FAIL	QKeyEvent ev: QKeyEvent(KeyPress, Key_BraceLeft, AltModifier, text="{") "<A-{>"
Option + '3'	#	ESC	FAIL	QKeyEvent ev: QKeyEvent(KeyPress, Key_3, AltModifier, text="#") "<A-#>"
Option + 'º'	\	ESC	FAIL	QKeyEvent ev: QKeyEvent(KeyPress, Key_masculine, AltModifier, text="\\") "<A-Bslash>"

The build log looks good but I'm attaching it just in case. I think yesterday I mistook the brew install log for the build log.

build-log.txt

[jgehrig]

I wish Qt had a better mechanism for detecting keyboard layout/localization...

The remaining issues are AltModifier related, and very similar to #837 and #833. The [ { and \ characters will be tricky. The QKeyEvents are indistinguishable from valid sequences on other layouts.

We'll need to carefully pick behavior that minimizes negative impact across layouts. Probably disallow <A-{> and similar on MacOS?

[alfredobarroso]

Maybe, but I'm not sure if that would break other layouts :(

[jgehrig]

Maybe, but I'm not sure if that would break other layouts :(

It will (to some degree).

My Alt + [ (US Layout) is identical to yours. We'll probably just disable Alt + {}[] for everyone. This is probably an acceptable trade-off. Layouts other than Spanish use Alt for []{} and similar characters.