[edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
Gao, Zhichao
zhichao.gao at intel.com
Thu Apr 2 08:28:52 UTC 2020
Summarize the issue, if anything incorrect, please help to correct it. Thanks.
Background:
Uefi spec ambiguous description:
> When interpreting the data from this function, it should be noted that if a class
> of printable characters that are normally adjusted by shift modifiers (e.g. Shift
> Key + "f" key) would be presented solely as a KeyData.Key.UnicodeChar without the
> associated shift state. So in the previous example of a Shift Key + "f" key being
> pressed, the only pertinent data returned would be KeyData.Key.UnicodeChar with
> the value of "F". This of course would not typically be the case for non-printable
> characters such as the pressing of the Right Shift Key + F10 key since the
> corresponding returned data would be reflected both in the
> KeyData.KeyState.KeyShiftState and KeyData.Key.ScanCode values.
Some firmware vendor see the “if” as an option to implement the keyboard driver and they choose to implement in the other way.
Ideal: shift + UnicodeKey/UnScancodeKey == report as ==> “shifted” UnicodeKey/UnScancodeKey
Ctrl + UnicodeKey == report as ==> UnicodeKey – ‘A’/’a’ + 1
Other way: shift + UnicodeKey/UnScancodeKey == report as ==> shift state + “shifted” UnicodeKey/UnScancodeKey
Ctrl + UnicodeKey == report as ==> ctrl state + UnicodeKey – ‘A’/’a’ + 1
I.e. do not remove the shift state.
That would cause the shell edit/Hexedit missing handling the “shifted” key when run shell in such firmware.
Vitaly’s solution add the support the handle “other implementation”.
If we take Vitaly’s solution, we should fixed all the applications in edk2 not only in shell edit/Hexeidt function.
And when we catch another request same with this issue, we would follow the same progress.
Thanks,
Zhichao
From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of Gao, Zhichao
Sent: Thursday, April 2, 2020 2:57 PM
To: devel at edk2.groups.io; vit9696 at protonmail.com; Rothman, Michael A <michael.a.rothman at intel.com>
Cc: Andrew Fish <afish at apple.com>; Laszlo Ersek <lersek at redhat.com>; Marvin Häuser <mhaeuser at outlook.de>; Kinney, Michael D <michael.d.kinney at intel.com>; Ni, Ray <ray.ni at intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
Hi Micheal Rothman,
Can you help to review this issue? Because of the uefi spec issue, some firmware implemented the different behavior.
Should the edk2 code to handle such issue? If yes, here maybe the situation:
1. Change for all apps -> uefi spec update and accept such behavior with description -> Done.
2. Change for all apps -> uefi spec update to remove the ambiguous and reject other behavior -> removal the change in first step.
Hi Vitaly,
I used to think it is an additional support for different implementation because of the spec. But if we approve this patch, all the app in edk2 using the combo key function should be update.
Using shell’s ctrl+’c’ as an example, it need to update at the same time. Same with SCT tool.
Thanks,
Zhichao
From: devel at edk2.groups.io<mailto:devel at edk2.groups.io> [mailto:devel at edk2.groups.io] On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, March 27, 2020 7:01 PM
To: Gao, Zhichao <zhichao.gao at intel.com<mailto:zhichao.gao at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Andrew Fish <afish at apple.com<mailto:afish at apple.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at redhat.com>>; Marvin Häuser <mhaeuser at outlook.de<mailto:mhaeuser at outlook.de>>; Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
Hello,
Requesting to merge this into edk2-stable202005 for the reasons explained in BZ[1]. I assume there is no real objection for this, only the approach we make such changes in the future, but this can be postponed as we encounter more of such problems.
Best regards,
Vitaly
[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2510
20 февр. 2020 г., в 03:27, Gao, Zhichao <zhichao.gao at intel.com<mailto:zhichao.gao at intel.com>> написал(а):
Sorry for my mistake. Then I have no other comments for this patch.
Reviewed-by: Zhichao Gao <zhichao.gao at intel.com<mailto:zhichao.gao at intel.com>>
Thanks,
Zhichao
From: vit9696 <vit9696 at protonmail.com<mailto:vit9696 at protonmail.com>>
Sent: Wednesday, February 19, 2020 8:15 PM
To: Gao, Zhichao <zhichao.gao at intel.com<mailto:zhichao.gao at intel.com>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
Zhichao,
Thanks for your review. The comment is correct, as ShiftOnlyState means the state where only shift (and no other modifiers) can be pressed or not.
Best wishes,
Vitaly
On Wed, Feb 19, 2020 at 09:55, Gao, Zhichao <zhichao.gao at intel.com<mailto:zhichao.gao at intel.com>> wrote:
Hi Vitaly,
See the comment below:
> -----Original Message-----
> From: devel at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Vitaly
> Cheptsov via Groups.Io
> Sent: Monday, February 10, 2020 6:18 PM
> To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with
> separately reported modifiers
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2510
>
> Some firmwares:
> - Report Shift modifier even when they report upper-case unicode letter.
> - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
>
> This change provides support for these firmwares preserving the compatibility
> with the previous input handling.
>
> Signed-off-by: Michael Belyaev <usrsse2 at icloud.com<mailto:usrsse2 at icloud.com>>
> Reviewed-by: Vitaly Cheptsov <vit9696 at protonmail.com<mailto:vit9696 at protonmail.com>>
> ---
> ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37
> ++++++++++++++------
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28
> ++++++++++-----
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 ++++
> ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11
> +++---
> 4 files changed, 58 insertions(+), 24 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> index df530f1119..9615a4dfbd 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> @@ -1381,8 +1381,8 @@ MainCommandDisplayHelp (
> continue;
> }
>
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
> - (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> + if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> //
> // For consoles that don't support/report shift state,
> // CTRL+W is translated to L'W' - L'A' + 1.
> @@ -1390,14 +1390,17 @@ MainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == L'w') || (KeyData.Key.UnicodeChar == L'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> @@ -1834,7 +1837,8 @@ MainEditorKeyInput (
> EFI_KEY_DATA KeyData;
> EFI_STATUS Status;
> EFI_SIMPLE_POINTER_STATE MouseState;
> - BOOLEAN NoShiftState;
> + BOOLEAN NoModifierState;
> + BOOLEAN ShiftOnlyState;
>
> do {
>
> @@ -1886,17 +1890,28 @@ MainEditorKeyInput (
> //
> StatusBarSetRefresh();
> //
> - // NoShiftState: TRUE when no shift key is pressed.
> + // NoModifierState: TRUE when no modifier key is pressed.
> //
> - NoShiftState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0) || (KeyData.KeyState.KeyShiftState ==
> EFI_SHIFT_STATE_VALID);
> + NoModifierState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID);
> + //
> + // ShiftOnlyState: TRUE when no modifier key except Shift is pressed.
> + //
> + ShiftOnlyState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || ((KeyData.KeyState.KeyShiftState
> + & ~(EFI_SHIFT_STATE_VALID |
> + EFI_LEFT_SHIFT_PRESSED | EFI_RIGHT_SHIFT_PRESSED)) == 0);
> //
> // dispatch to different components' key handling function
> //
> if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey(&KeyData)) {
> Status = EFI_SUCCESS;
> - } else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) ||
> ((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <=
> SCAN_PAGE_DOWN)))) {
> + } else if ((ShiftOnlyState && (KeyData.Key.ScanCode == SCAN_NULL))
> + || (NoModifierState && (KeyData.Key.ScanCode >= SCAN_UP) &&
> (KeyData.Key.ScanCode <= SCAN_PAGE_DOWN))) {
> + //
> + // alphanumeric keys with or without shift, or arrow keys without shift
> + //
This is unmatched with the comments. It only handles the alphanumeric keys with shift.
Thanks,
Zhichao
> Status = FileBufferHandleInput (&KeyData.Key);
> - } else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) &&
> (KeyData.Key.ScanCode <= SCAN_F12)) {
> + } else if (NoModifierState && (KeyData.Key.ScanCode >= SCAN_F1)
> + && (KeyData.Key.ScanCode <= SCAN_F12)) {
> Status = MenuBarDispatchFunctionKey (&KeyData.Key);
> } else {
> StatusBarSetStatusString (L"Unknown Command"); diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> index 35b0b701e8..d053059220 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> @@ -130,6 +130,8 @@ InputBarRefresh (
> UINTN EventIndex;
> UINTN CursorRow;
> UINTN CursorCol;
> + BOOLEAN ShiftPressed;
> + BOOLEAN ModifiersPressed;
>
> //
> // variable initialization
> @@ -180,17 +182,23 @@ InputBarRefresh (
> if (EFI_ERROR (Status)) {
> continue;
> }
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0) &&
> - (KeyData.KeyState.KeyShiftState != EFI_SHIFT_STATE_VALID)) {
> - //
> - // Shift key pressed.
> - //
> + ModifiersPressed = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) != 0)
> + && (KeyData.KeyState.KeyShiftState !=
> + EFI_SHIFT_STATE_VALID);
> +
> + //
> + // TRUE if Shift is pressed and no other modifiers are pressed
> + //
> + ShiftPressed = ModifiersPressed &&
> + ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_SHIFT_PRESSED |
> + EFI_RIGHT_SHIFT_PRESSED)) == 0);
> +
> + if (ModifiersPressed && !ShiftPressed) {
> continue;
> }
> //
> // pressed ESC
> //
> - if (KeyData.Key.ScanCode == SCAN_ESC) {
> + if (!ModifiersPressed && KeyData.Key.ScanCode == SCAN_ESC) {
> Size = 0;
> Status = EFI_NOT_READY;
> break;
> @@ -198,9 +206,10 @@ InputBarRefresh (
> //
> // return pressed
> //
> - if (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN) {
> + if (!ModifiersPressed
> + && (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> + KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN)) {
> break;
> - } else if (KeyData.Key.UnicodeChar == CHAR_BACKSPACE) {
> + } else if (!ModifiersPressed && KeyData.Key.UnicodeChar ==
> + CHAR_BACKSPACE) {
> //
> // backspace
> //
> @@ -213,7 +222,8 @@ InputBarRefresh (
>
> }
> }
> - } else if (KeyData.Key.UnicodeChar <= 127 && KeyData.Key.UnicodeChar >=
> 32) {
> + } else if ((!ModifiersPressed || ShiftPressed)
> + && KeyData.Key.UnicodeChar <= 127 &&
> + KeyData.Key.UnicodeChar >= 32) {
> //
> // VALID ASCII char pressed
> //
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> index ca8bc506d9..eabbf3c571 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> @@ -190,11 +190,17 @@ MenuBarDispatchControlHotKey (
> //
> // For consoles that supports/reports shift state,
> // make sure only CONTROL is pressed.
> + // For some consoles that report shift state,
> + // Ctrl+A is still translated to 1 (UnicodeChar).
> //
> if ((KeyData->Key.UnicodeChar >= L'A') && (KeyData->Key.UnicodeChar <=
> L'Z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'A' + 1;
> } else if ((KeyData->Key.UnicodeChar >= L'a') && (KeyData->Key.UnicodeChar
> <= L'z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'a' + 1;
> + } else if ((KeyData->Key.UnicodeChar >= L'A' - L'A' + 1) && (KeyData-
> >Key.UnicodeChar <= L'Z' - L'A' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> + } else if ((KeyData->Key.UnicodeChar >= L'a' - L'a' + 1) && (KeyData-
> >Key.UnicodeChar <= L'z' - L'z' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> }
> }
> if ((SCAN_CONTROL_Z < ControlIndex)
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> index a00df49d38..394e531c16 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.
> +++ c
> @@ -137,14 +137,17 @@ HMainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> --
> 2.21.1 (Apple Git-122.3)
>
>
> -=-=-=-=-=-=
> Groups.io<http://Groups.io> Links: You receive all messages sent to this group.
>
> View/Reply Online (#54122): https://edk2.groups.io/g/devel/message/54122
> Mute This Topic: https://groups.io/mt/71133729/1768756
> Group Owner: devel+owner at edk2.groups.io<mailto:devel+owner at edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao at intel.com<mailto:zhichao.gao at intel.com>]
> -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56889): https://edk2.groups.io/g/devel/message/56889
Mute This Topic: https://groups.io/mt/71133729/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200402/567e8485/attachment.htm>
More information about the edk2-devel-archive
mailing list