Zhichao, This is correct. I did not notice the message previously, but otherwise everything outlined here is accurate. Let me know if further input can be performed from my side for this to land.
Best regards, Vitaly > 2 апр. 2020 г., в 11:28, Gao, Zhichao <zhichao....@intel.com> написал(а): > > 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@edk2.groups.io <mailto:devel@edk2.groups.io> > [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of Gao, > Zhichao > Sent: Thursday, April 2, 2020 2:57 PM > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; > vit9...@protonmail.com <mailto:vit9...@protonmail.com>; Rothman, Michael A > <michael.a.roth...@intel.com <mailto:michael.a.roth...@intel.com>> > Cc: Andrew Fish <af...@apple.com <mailto:af...@apple.com>>; Laszlo Ersek > <ler...@redhat.com <mailto:ler...@redhat.com>>; Marvin Häuser > <mhaeu...@outlook.de <mailto:mhaeu...@outlook.de>>; Kinney, Michael D > <michael.d.kin...@intel.com <mailto:michael.d.kin...@intel.com>>; Ni, Ray > <ray...@intel.com <mailto:ray...@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@edk2.groups.io <mailto:devel@edk2.groups.io> > [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of > Vitaly Cheptsov via Groups.Io > Sent: Friday, March 27, 2020 7:01 PM > To: Gao, Zhichao <zhichao....@intel.com <mailto:zhichao....@intel.com>> > Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Andrew Fish > <af...@apple.com <mailto:af...@apple.com>>; Laszlo Ersek <ler...@redhat.com > <mailto:ler...@redhat.com>>; Marvin Häuser <mhaeu...@outlook.de > <mailto:mhaeu...@outlook.de>>; Kinney, Michael D <michael.d.kin...@intel.com > <mailto:michael.d.kin...@intel.com>>; Ni, Ray <ray...@intel.com > <mailto:ray...@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 > <https://bugzilla.tianocore.org/show_bug.cgi?id=2510> > > > 20 февр. 2020 г., в 03:27, Gao, Zhichao <zhichao....@intel.com > <mailto:zhichao....@intel.com>> написал(а): > > Sorry for my mistake. Then I have no other comments for this patch. > Reviewed-by: Zhichao Gao <zhichao....@intel.com > <mailto:zhichao....@intel.com>> > > Thanks, > Zhichao > > <>From: vit9696 <vit9...@protonmail.com <mailto:vit9...@protonmail.com>> > Sent: Wednesday, February 19, 2020 8:15 PM > To: Gao, Zhichao <zhichao....@intel.com <mailto:zhichao....@intel.com>>; > devel@edk2.groups.io <mailto:devel@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....@intel.com > <mailto:zhichao....@intel.com>> wrote: > Hi Vitaly, > > See the comment below: > > > -----Original Message----- > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly > > Cheptsov via Groups.Io > > Sent: Monday, February 10, 2020 6:18 PM > > To: devel@edk2.groups.io <mailto:devel@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 > > <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 <usrs...@icloud.com > > <mailto:usrs...@icloud.com>> > > Reviewed-by: Vitaly Cheptsov <vit9...@protonmail.com > > <mailto:vit9...@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 > > <https://edk2.groups.io/g/devel/message/54122> > > Mute This Topic: https://groups.io/mt/71133729/1768756 > > <https://groups.io/mt/71133729/1768756> > > Group Owner: devel+ow...@edk2.groups.io <mailto:devel+ow...@edk2.groups.io> > > Unsubscribe: https://edk2.groups.io/g/devel/unsub > > <https://edk2.groups.io/g/devel/unsub> [zhichao....@intel.com > > <mailto:zhichao....@intel.com>] > > -=-=-=-=-=-= > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57544): https://edk2.groups.io/g/devel/message/57544 Mute This Topic: https://groups.io/mt/71133729/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: OpenPGP digital signature
