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

Keyboard mapping: add mapped key string to raw key event #4447

Closed
wants to merge 3 commits into from

Conversation

yatli
Copy link
Contributor

@yatli yatli commented Aug 6, 2020

No description provided.

@yatli
Copy link
Contributor Author

yatli commented Aug 6, 2020

Getting the error:

error : MembersMustExist : Member 'public void Avalonia.Input.Raw.RawKeyEventArgs..ctor(Avalonia.Input.IKeyboardDevice, System.UInt64, Avalonia.Input.IInputRoot, Avalonia.Input.Raw.RawKeyEventType, Avalonia.Input.Key, Avalonia.Input.RawInputModifiers)

So that's perhaps a compat dealbreaker..
Add ctor that ignores the mapped key.

@yatli
Copy link
Contributor Author

yatli commented Aug 6, 2020

OSX side not implemented. Don't really know how to bite the objc+com(?)+sharpgen thing #_# (sorry

@kekekeks
Copy link
Member

kekekeks commented Aug 6, 2020

How would that work with various IME systems once they are implemented?

@kekekeks
Copy link
Member

kekekeks commented Aug 6, 2020

Also, I don't see any the user API changes, only internal raw event args.

@yatli
Copy link
Contributor Author

yatli commented Aug 6, 2020

Also, I don't see any the user API changes, only internal raw event args.

Yeah just realized that. Adding it to KeyEventArgs now.

@yatli
Copy link
Contributor Author

yatli commented Aug 6, 2020

How would that work with various IME systems once they are implemented?

I think IME input events should be routed as text input messages plus ImeProcessedKey just like the Windows side.
For KeyEventArg they can be reported as raw keys, because the committed character isn't confirmed until the user commits the sequence (usually with space/enter etc.) anyway

@kekekeks
Copy link
Member

kekekeks commented Aug 6, 2020

I don't see such property in neither WPF or UWP, why do we need it?

@yatli
Copy link
Contributor Author

yatli commented Aug 6, 2020

see: yatli/fvim#36

@maxkatz6
Copy link
Member

maxkatz6 commented Aug 6, 2020

@kekekeks uwp has Key and OriginalKey (unmapped) props https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.input.keyroutedeventargs?view=winrt-19041

@maxkatz6
Copy link
Member

maxkatz6 commented Aug 6, 2020

And I have not looked detailedly on original issue, but probably Character prop is needed as well https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.input.characterreceivedroutedeventargs?view=winrt-19041 (note, it is from another event in uwp). Not sure, does Avalonia have it, can't check now.

_owner,
RawKeyEventType.KeyDown,
KeyInterop.KeyFromVirtualKey(ToInt32(wParam), ToInt32(lParam)),
WindowsKeyboardDevice.Instance.StringFromVirtualKey((uint)ToInt32(wParam)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be corrected. StringFromVirtualKey takes ctrl and alt into consideration and will translate a key into a control sequence.

@kekekeks
Copy link
Member

kekekeks commented Aug 6, 2020

@maxkatz6 UWP seems to provide virtual key mapping, not characters.

@yatli
Copy link
Contributor Author

yatli commented Aug 6, 2020

@kekekeks I think I can give a single requirement that cannot be satisfied with the current framework: to detect Control-# on all keyboard mappings.

TextInput doesn't give key modifiers. You get "#"
KeyEventArgs doesn't give key mappings. You get "en-US key at # position"

KeyEventArgs actually has interesting behavior (tested on Windows) -- it supports key mapping to some degree.
For a Swedish keyboard layout, the '[' on en-US should be å or Å (shifted).
Currently in Avalonia it reports ']' or '}' respectively. So it's neither en-US nor the Swedish keyboard.

@kekekeks
Copy link
Member

kekekeks commented Aug 6, 2020

Before introducing such an API that doesn't exist in UWP/WPF, we need to

  1. check how UWP and WPF achieve the same result
  2. check how it's done in Qt and GTK.

Otherwise we are bound to miss something important.

@yatli
Copy link
Contributor Author

yatli commented Aug 6, 2020

For Qt, see neovim-qt: https://github.com/equalsraf/neovim-qt/blob/4352846be7f7f42738c568eafaba965a20bb7a45/src/gui/input.cpp#L132

QString convertKey(const QKeyEvent& ev) noexcept
{
	QString text{ ev.text() };
...

https://doc.qt.io/qt-5/qkeyevent.html#text

@grokys
Copy link
Member

grokys commented Sep 30, 2020

What's the status on this PR?

@workgroupengineering
Copy link
Contributor

workgroupengineering commented Sep 30, 2020

instead of modifying RawKeyEventArgs, isn't it better to create an extesion method like this?

public interface IKeyMapper
{
   string ToKeyString(RawKeyEventArgs arg, CultureInfo culture);
   ...
}


public static string? ToKeyString(this RawKeyEventArgs args, CultureInfo culture = null )
{
     var mapper = AvaloniaLocator.GetService<IKeyMapper>();
     return mapper?.ToKeyString(arg, culture);
}

@kekekeks
Copy link
Member

kekekeks commented Oct 1, 2020

  1. Using locator in new code is heavily discouraged unless there is a very good reason why it can't be done in a different way
  2. The information about key mappings is heavily platform-dependent and there is no guarantee that information from RawKeyEventEventArgs would be enough to get the mapped string.

@workgroupengineering
Copy link
Contributor

1. Using locator in new code is heavily discouraged unless there is a **very** good reason why it can't be done in a different way

Locator in this case is useful for injecting the specific keymapper by platform, we could implement macOsKeyMapper, iOSKeyMapper, etc ...

2. The information about key mappings is heavily platform-dependent and there is no guarantee that information from RawKeyEventEventArgs would be enough to get the mapped string.

that's true, but any platform could subclass RawKeyEventArgs and add platform-specific information. Only the keymapper of the specific platform would see the difference.

@kekekeks
Copy link
Member

kekekeks commented Oct 1, 2020

Locator in this case is useful for injecting the specific keymapper by platform, we could implement macOsKeyMapper, iOSKeyMapper, etc ...

Which is not a valid reason to add something to service locator. Stuff like that should be resolved via the platform layer or embedded into the event args via the same platform layer. Service locator is pure evil that's here because it's been there in the early days (and it's also kinda troublesome to manage rendering-related stuff without a locator).

@workgroupengineering
Copy link
Contributor

I think that there is no absolute evil or absolute good, it all depends on the use made of it.
In this case I don't think it is.
By linking the key string to the event, it would be much more complicated to retrieve key strings in a specific culture.

@maxkatz6
Copy link
Member

maxkatz6 commented Nov 22, 2020

@maxkatz6 UWP seems to provide virtual key mapping, not characters.

Yep, you are right. I misleaded point of PR back then.

I just found that in UWP there is relatively new event for controls - UIElement.CharacterReceived
Which has char Character { get; } in the args.

There is also old global event with another arguments CoreWindow.CharacterReceived

Upd: although it's very similar to our TextInput event.

@yatli
Copy link
Contributor Author

yatli commented Nov 22, 2020

Another approach is to provide API to query "what text this keystroke will produce".
This could also play nicer with IME because we can get the info from the query that IME keystrokes are "queued", and will only produce text when it's committed. Like discussed in #3538, the query will translate the raw key data into one of the following:

type KeystrokeEffect =
| Key of Key // keyboard mapping agnostic special keys e.g. Enter, Backspace etc.
| Translated of char // keyboard mapping specific key translation
| ImePreEdit of string // IME preedit update
| ImeCommit of string // IME commit, clears preedit and input text
| ImeCancel of string // IME cancel, clears preedit and convert the preedit to input text

We just need to keep the relevant platform-specific keystroke data, in sync with the IME states.

I can give it a try if you guys think this is reasonable.

@mcmikecreations
Copy link

Has any progress been done since the last message?

@yatli
Copy link
Contributor Author

yatli commented Feb 11, 2021

The core team has designed and implemented the IME model, I have to read it and adjust my proposal so not to break it :)

@mcmikecreations
Copy link

The core team has designed and implemented the IME model, I have to read it and adjust my proposal so not to break it :)

I have the same problem as this user of fvim and I'm looking forward to a fix that I can use in my project. Adding keyboard mapping would benefit a lot of projects!

@jmacato
Copy link
Member

jmacato commented Aug 1, 2021

Closing this PR for inactivity. Ping me if this is updated accordingly.

@jmacato jmacato closed this Aug 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants