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

W32 ime classic-n-experimental #12752

Closed
wants to merge 8 commits into from

Conversation

ant0sha
Copy link
Contributor

@ant0sha ant0sha commented Aug 9, 2023

To untangle complicated circumstances introduced in v8.2.4807 old way of interpreting keyboard input via TranslateMessage() is reverted to be default again. At the same time, new/experimental "input method" which is using ToUnicode() Win API call is still available, in case some users are already dependent on it. To change between both "input methods" in run-time testing events infrastructure is (mis?)used:

call test_mswin_event('set_keycode_trans_strategy', {'strategy': 'experimental'})
call test_mswin_event('set_keycode_trans_strategy', {'strategy': 'classic'})

Documentation is updated accordingly.

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #12752 (16b8c8a) into master (84bc00e) will decrease coverage by 0.69%.
Report is 39 commits behind head on master.
The diff coverage is 4.72%.

@@            Coverage Diff             @@
##           master   #12752      +/-   ##
==========================================
- Coverage   82.10%   81.41%   -0.69%     
==========================================
  Files         160      160              
  Lines      193690   193773      +83     
  Branches    43492    43495       +3     
==========================================
- Hits       159025   157762    -1263     
- Misses      21819    23273    +1454     
+ Partials    12846    12738     -108     
Flag Coverage Δ
huge-clang-none 82.72% <ø> (-0.01%) ⬇️
linux 82.72% <ø> (-0.01%) ⬇️
mingw-x64-HUGE 76.57% <4.72%> (-0.04%) ⬇️
mingw-x86-HUGE ?
windows 76.57% <4.72%> (-1.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/testing.c 93.82% <0.00%> (-0.38%) ⬇️
src/gui_w32.c 23.00% <4.76%> (-16.89%) ⬇️

... and 57 files with indirect coverage changes

@k-takata
Copy link
Member

The word "input method" has already different meaning, I think.
https://en.wikipedia.org/wiki/Input_method

It might be better to use another term. (Not sure what is the best term, though. "key event handling"?)

@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 10, 2023

The word "input method" has already different meaning, I think. https://en.wikipedia.org/wiki/Input_method

It might be better to use another term. (Not sure what is the best term, though. "key event handling"?)

Yes, "key event handling" would fit of course, with perhaps a little disadvantage of being too unspecific...

How about - my personal favorite: "keyboard layout translation strategy" (strategy in a sense of design pattern "strategy") // shortened "kbdlayout_trans_strategy"

Or perhaps: "keycode translation strategy" // shortened "keycode_trans_strategy"

Yet another variant: "keyboard layout handling" // shortened "kbdlayout_handling"

Let me know your preference to update PR accordingly.

@k-takata
Copy link
Member

I checked the document of TranslateMessage:
https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-translatemessage

Translates virtual-key messages into character messages.

I think "keycode translation strategy" is good.

Word "strategy" is used in the code (inspired by "strategy" design
pattern); in documentation it seem to be more appropriate to
use the word "method" instead.
@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 10, 2023

I think "keycode translation strategy" is good.

updated accordingly, please check

@k-takata
Copy link
Member

I'm thinking if using test_mswin_event() is okay.
This cannot be used on the tiny builds.

@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 10, 2023

I'm thinking if using test_mswin_event() is okay.
This cannot be used on the tiny builds.

~~~~ Update: please ignore this comment  ~~~~~~~~
 /* removing all *.o files manually solved my issue with TINY build
  * now the question is really, how to change the keycode_trans_strategy in TINY w32 GVIM
 */

Hm... Do we have such biest "gvim tiny version" at all ? Or it is perhaps issue with makefile for Mingw. I am using cross compiler with MINGW from linux - Make_cyg_ming.mak (with CROSS=yes), set "FEATURES=TINY" there, instead of "FEATURES=HUGE" and get bunch of errors when I try to build tiny gvim for windows, linking step fails [1].

Are there perhaps some good instructions how to build it otherwise? I quickly checked offline version of Tony's page but it is only talking about how to build unix flavors, nothing about cross-compile.

(Well I can try later to build on another machine with cygwin mingw perhaps with more luck but... Will appreciate short advise to save time)

[1] failure from linker on opensuse with cross-mingw for TINY version

/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/alloc.o:alloc.c:(.rdata$.refptr.alloc_fail_id[.refptr.alloc_fail_id]+0x0): undefine
d reference to `alloc_fail_id'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/alloc.o:alloc.c:(.rdata$.refptr.alloc_fail_repeat[.refptr.alloc_fail_repeat]+0x0): 
undefined reference to `alloc_fail_repeat'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/alloc.o:alloc.c:(.rdata$.refptr.alloc_fail_countdown[.refptr.alloc_fail_countdown]+
0x0): undefined reference to `alloc_fail_countdown'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arabic.o:arabic.c:(.rdata$.refptr.p_tbidi[.refptr.p_tbidi]+0x0): undefined referenc
e to `p_tbidi'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arabic.o:arabic.c:(.rdata$.refptr.p_arshape[.refptr.p_arshape]+0x0): undefined refe
rence to `p_arshape'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x1086): undefined reference to `error_if_popup_window'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x1333): undefined reference to `error_if_popup_window'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x1619): undefined reference to `error_if_popup_window'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x1909): undefined reference to `error_if_popup_window'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x3454): undefined reference to `find_win_by_nr_or_id'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x34a6): undefined reference to `check_for_opt_number_ar
g'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x34bc): undefined reference to `tv_get_number'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x352b): undefined reference to `check_for_opt_number_arg'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x354c): undefined reference to `find_tabwin'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x3579): undefined reference to `check_for_opt_number_arg'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x35cd): undefined reference to `find_win_by_nr_or_id'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x35fb): undefined reference to `tv_get_number_chk'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x3665): undefined reference to `tv_get_number_chk'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x3674): undefined reference to `rettv_list_alloc'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x36ab): undefined reference to `list_append_string'
/usr/lib64/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld: gobjx86-64/arglist.o:arglist.c:(.text+0x36d6): undefined reference to `check_for_opt_number_ar
...

@chrisbra
Copy link
Member

call test_mswin_event('set_w32_ime', {'ime': 'experimental'})
call test_mswin_event('set_w32_ime', {'ime': 'classic'})

Is this just for now or are we going with some new option value?

@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 10, 2023

call test_mswin_event('set_w32_ime', {'ime': 'experimental'})
call test_mswin_event('set_w32_ime', {'ime': 'classic'})

Is this just for now or are we going with some new option value?

Hi @chrisbra , right now the plan is like this:

	:call test_mswin_event('set_keycode_trans_strategy', {'strategy': 'experimental'})
	:call test_mswin_event('set_keycode_trans_strategy', {'strategy': 'classic'})

@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 10, 2023

Please ignore my comment about problems with TINY GVIM build (removing all *.o files manually solved my issue with it)

Now the question is really, how to change the keycode_trans_strategy in TINY w32 GVIM. To introduce new normal option for that sounds somehow silly... In the beginning I was thinking we will check some environment variable once and do lazy initialize the strategy if it is not yet initialized - but that way we can only have one state per running instance, which cannot be changed on the fly, which quite restrict the possibilities for experimenting. Are there some similar "hidden developer back doors" existing, beside to let user to call some special function?

@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 10, 2023

In the meantime I will implement fallback check first time, if environment variable, let us say VIM_KEYCODE_TRANS_STRATEGY is defined, and if yes, it value (classic|experimental) will be used to initialize the strategy first time. So no dynamic change on the fly will be possible in TINY gvim.

Quoting the docu:

Alternatively (this method is especially useful for the TINY GVIM build, where
test_mswin_event() cannot be called), environment variable
VIM_KEYCODE_TRANS_STRATEGY can be set to the desired value (`experimental' or
`classic'), to override default. E.g., type in dos prompt:
>
        set VIM_KEYCODE_TRANS_STRATEGY=experimental
        gvim.exe
<
@k-takata
Copy link
Member

I think there are some options:

  1. Add a new option to enable the experimental keycode translation.
    Bram didn't like to add new options. But, if necessary, we can still add a new option.
  2. Don't support the experimental keycode translation in the tiny builds.
    Tiny builds users may not use the experimental keycode translation?
  3. Use the environment variable.

I want to hear others' opinions.

@chrisbra
Copy link
Member

I have been wondering:

  • could it happen that users need to use mappings from one keycode translation and different mappings from other keycode translations?

@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 12, 2023

I have been wondering:

* could it happen that users need to use mappings from one keycode translation and different mappings from other keycode translations?

Kind of, especially on non-English keyboard layouts - with 'experimental' strategy one can start to map things, which are usually "overriden" by TranlsateMessage() in 'classic' strategy.

Examples

  • consider German keyboard layout and mapping imap <C-ü> something
    • With 'classic' strategy it is void, because <C-ü> never reaches _OnChar(), but Windows API TranslateMessage() silently converts it to (\027) instead.
    • With 'experimental' strategy it works - but on the other hand, users seems to prefer to have here emitted always

Also what puzzles me yet: it do not works to map <C-ü> <ESC> on German keyboard layout to mimic 'classic' behavior, it only works with imap ... - not investigated why, since the point now is not to "fix" the 'experimental' strategy, but come back to the "lost" 'classic' strategy with least possible effort, at the same time keeping the door open if there are some users out there who get hooked on new 'experimental' strategy - and made available through it new mappings.

Another example:

  • consider Cyrillic keyboard layout and mapping imap <C-ы> something

    • With 'classic' strategy it is void, because <C-ы> never reaches _OnChar(), but Windows API TranslateMessage() silently converts it to .
    • With 'experimental' strategy it works - but on the other hand, users seems to prefer to have here emitted always
  • consider Cyrillic keyboard layout and mapping imap <C-с> something (russian "es" letter)

    • With 'classic': (normal english "C")
    • With 'experimental' - "something" is inserted but users seems to prefer (english "C") here
  • consider Cyrillic keyboard layout and mapping imap <C-м> something (russian "em" letter)

    • With 'classic': (english "V")
    • With 'experimental' - "something" is inserted but users seems to prefer (english "V") here

Basically for Cyrillic layout seems that most russian letters with CTRL producing with 'classic' their underlying latin-key control codes (but strangely not like on German! - well this is a question to Microsoft and their TranslateMessage() implementation) and that is what users prefer to have restored back. However after more then one year of 'experimental' strategy being in the wild, we are never sure we break someone configuration by changing back to 'classic' - without supporting some migration possibility. Also I am not sure how 'classic' works with completely and completely non standard keyboard layouts like 'bepo' (https://en.wikipedia.org/wiki/B%C3%89PO) and if there more advantages on such layouts we have from 'experimental'.

@yegappan yegappan added this to the backlog milestone Aug 13, 2023
@ant0sha
Copy link
Contributor Author

ant0sha commented Aug 15, 2023

This is assigned to "backlog" milestone right now.

Should fix:
#12595
#10615

@chrisbra
Copy link
Member

chrisbra commented Jan 4, 2024

can we use the 'keyprotocol' option here?

MiguelBarro added a commit to MiguelBarro/vim that referenced this pull request Jan 14, 2024
Signed-off-by: GuyBrush <miguel.barro@live.com>
chrisbra pushed a commit that referenced this pull request Jan 14, 2024
Problem:  win32: Ctrl-D cannot be used to close a pipe
Solution: Properly detect Ctrl-D when reading from a pipe
          (GuyBrush)

Enabling Ctrl-D for gvim pipeline input
and apply defensive programming on account of PR #12752
so that once PR 12752 is merged, CTRL-D will keep on working

closes: #13849

Signed-off-by: GuyBrush <miguel.barro@live.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>
@chrisbra chrisbra closed this in 68d9472 Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants