[PATCH 0/1] ShellPkg: Add support for input with separately reported modifiers

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH 0/1] ShellPkg: Add support for input with separately reported modifiers
@ 2020-02-10 10:18 Vitaly Cheptsov
  2020-02-10 10:18 ` [PATCH 1/1] " Vitaly Cheptsov
  0 siblings, 1 reply; 13+ messages in thread
From: Vitaly Cheptsov @ 2020-02-10 10:18 UTC (permalink / raw)
  To: devel

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

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.

We request this to be merged in edk2-stable202002.

Vitaly Cheptsov (1):
  ShellPkg: Add support for input with separately reported modifiers

 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(-)

-- 
2.21.1 (Apple Git-122.3)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-02-10 10:18 [PATCH 0/1] ShellPkg: Add support for input with separately reported modifiers Vitaly Cheptsov
@ 2020-02-10 10:18 ` Vitaly Cheptsov
  2020-02-14 11:55   ` Vitaly Cheptsov
  2020-02-19  6:55   ` [edk2-devel] " Gao, Zhichao
  0 siblings, 2 replies; 13+ messages in thread
From: Vitaly Cheptsov @ 2020-02-10 10:18 UTC (permalink / raw)
  To: devel

[-- Attachment #1: Type: text/plain, Size: 10964 bytes --]

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@icloud.com>
Reviewed-by: Vitaly Cheptsov <vit9696@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
+          //
           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)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-02-10 10:18 ` [PATCH 1/1] " Vitaly Cheptsov
@ 2020-02-14 11:55   ` Vitaly Cheptsov
  2020-02-19  6:55   ` [edk2-devel] " Gao, Zhichao
  1 sibling, 0 replies; 13+ messages in thread
From: Vitaly Cheptsov @ 2020-02-14 11:55 UTC (permalink / raw)
  To: devel, Gao, Liming

[-- Attachment #1: Type: text/plain, Size: 10043 bytes --]

Replying as per Liming's request for this to be merged into edk2-stable202002.

On Mon, Feb 10, 2020 at 13:18, Vitaly Cheptsov <vit9696@protonmail.com> wrote:

> 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@icloud.com>
> Reviewed-by: Vitaly Cheptsov <vit9696@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
> + //
> 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)

[-- Attachment #2: Type: text/html, Size: 12543 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-02-10 10:18 ` [PATCH 1/1] " Vitaly Cheptsov
  2020-02-14 11:55   ` Vitaly Cheptsov
@ 2020-02-19  6:55   ` Gao, Zhichao
  2020-02-19 12:15     ` Vitaly Cheptsov
  1 sibling, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2020-02-19  6:55 UTC (permalink / raw)
  To: devel@edk2.groups.io, vit9696@protonmail.com

Hi Vitaly,

See the comment below:

> -----Original Message-----
> From: devel@edk2.groups.io <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
> 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@icloud.com>
> Reviewed-by: Vitaly Cheptsov <vit9696@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 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@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [zhichao.gao@intel.com]
> -=-=-=-=-=-=


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-02-19  6:55   ` [edk2-devel] " Gao, Zhichao
@ 2020-02-19 12:15     ` Vitaly Cheptsov
  2020-02-20  0:27       ` Gao, Zhichao
  0 siblings, 1 reply; 13+ messages in thread
From: Vitaly Cheptsov @ 2020-02-19 12:15 UTC (permalink / raw)
  To: Gao, Zhichao, devel@edk2.groups.io

[-- Attachment #1: Type: text/plain, Size: 11356 bytes --]

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@intel.com> wrote:

> Hi Vitaly,
>
> See the comment below:
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <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
>> 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@icloud.com>
>> Reviewed-by: Vitaly Cheptsov <vit9696@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 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@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@intel.com]
>> -=-=-=-=-=-=

[-- Attachment #2: Type: text/html, Size: 14766 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-02-19 12:15     ` Vitaly Cheptsov
@ 2020-02-20  0:27       ` Gao, Zhichao
  2020-03-27 11:00         ` Vitaly Cheptsov
  0 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2020-02-20  0:27 UTC (permalink / raw)
  To: vit9696, devel@edk2.groups.io

[-- Attachment #1: Type: text/plain, Size: 12000 bytes --]

Sorry for my mistake. Then I have no other comments for this patch.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>

Thanks,
Zhichao

From: vit9696 <vit9696@protonmail.com>
Sent: Wednesday, February 19, 2020 8:15 PM
To: Gao, Zhichao <zhichao.gao@intel.com>; 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.gao@intel.com<mailto:zhichao.gao@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
>
> 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@icloud.com<mailto:usrsse2@icloud.com>>
> Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com<mailto:vit9696@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 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@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@intel.com]
> -=-=-=-=-=-=



[-- Attachment #2: Type: text/html, Size: 18666 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-02-20  0:27       ` Gao, Zhichao
@ 2020-03-27 11:00         ` Vitaly Cheptsov
  2020-04-02  6:57           ` Gao, Zhichao
       [not found]           ` <1601EE5ED9DEC05E.18513@groups.io>
  0 siblings, 2 replies; 13+ messages in thread
From: Vitaly Cheptsov @ 2020-03-27 11:00 UTC (permalink / raw)
  To: Gao, Zhichao
  Cc: devel@edk2.groups.io, Andrew Fish, Laszlo Ersek,
	Marvin Häuser, Mike Kinney, Ni, Ray


[-- Attachment #1.1: Type: text/plain, Size: 13261 bytes --]

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@intel.com> написал(а):
> 
> Sorry for my mistake. Then I have no other comments for this patch.
> Reviewed-by: Zhichao Gao <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>
>  
> Thanks,
> Zhichao
>  
>  <>From: vit9696 <vit9696@protonmail.com> 
> Sent: Wednesday, February 19, 2020 8:15 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; 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.gao@intel.com <mailto:zhichao.gao@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 <usrsse2@icloud.com <mailto:usrsse2@icloud.com>>
> > Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@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 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+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub> [zhichao.gao@intel.com]
> > -=-=-=-=-=-=
> 


[-- Attachment #1.2: Type: text/html, Size: 22735 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-03-27 11:00         ` Vitaly Cheptsov
@ 2020-04-02  6:57           ` Gao, Zhichao
       [not found]           ` <1601EE5ED9DEC05E.18513@groups.io>
  1 sibling, 0 replies; 13+ messages in thread
From: Gao, Zhichao @ 2020-04-02  6:57 UTC (permalink / raw)
  To: devel@edk2.groups.io, vit9696@protonmail.com, Rothman, Michael A
  Cc: Andrew Fish, Laszlo Ersek, Marvin Häuser, Kinney, Michael D,
	Ni, Ray

[-- Attachment #1: Type: text/plain, Size: 13894 bytes --]

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] On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, March 27, 2020 7:01 PM
To: Gao, Zhichao <zhichao.gao@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Marvin Häuser <mhaeuser@outlook.de>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@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@intel.com<mailto:zhichao.gao@intel.com>> написал(а):

Sorry for my mistake. Then I have no other comments for this patch.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>

Thanks,
Zhichao

From: vit9696 <vit9696@protonmail.com<mailto:vit9696@protonmail.com>>
Sent: Wednesday, February 19, 2020 8:15 PM
To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@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.gao@intel.com<mailto:zhichao.gao@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
>
> 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@icloud.com<mailto:usrsse2@icloud.com>>
> Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com<mailto:vit9696@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@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>]
> -=-=-=-=-=-=



[-- Attachment #2: Type: text/html, Size: 29163 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
       [not found]           ` <1601EE5ED9DEC05E.18513@groups.io>
@ 2020-04-02  8:28             ` Gao, Zhichao
  2020-04-17 18:14               ` Vitaly Cheptsov
  0 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2020-04-02  8:28 UTC (permalink / raw)
  To: devel@edk2.groups.io, Gao, Zhichao, vit9696@protonmail.com,
	Rothman, Michael A
  Cc: Andrew Fish, Laszlo Ersek, Marvin Häuser, Kinney, Michael D,
	Ni, Ray

[-- Attachment #1: Type: text/plain, Size: 16475 bytes --]

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] On Behalf Of Gao, Zhichao
Sent: Thursday, April 2, 2020 2:57 PM
To: devel@edk2.groups.io; vit9696@protonmail.com; Rothman, Michael A <michael.a.rothman@intel.com>
Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Marvin Häuser <mhaeuser@outlook.de>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@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] On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, March 27, 2020 7:01 PM
To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Andrew Fish <afish@apple.com<mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Marvin Häuser <mhaeuser@outlook.de<mailto:mhaeuser@outlook.de>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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@intel.com<mailto:zhichao.gao@intel.com>> написал(а):

Sorry for my mistake. Then I have no other comments for this patch.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>

Thanks,
Zhichao

From: vit9696 <vit9696@protonmail.com<mailto:vit9696@protonmail.com>>
Sent: Wednesday, February 19, 2020 8:15 PM
To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@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.gao@intel.com<mailto:zhichao.gao@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
>
> 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@icloud.com<mailto:usrsse2@icloud.com>>
> Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com<mailto:vit9696@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@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>]
> -=-=-=-=-=-=



[-- Attachment #2: Type: text/html, Size: 41011 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-04-02  8:28             ` Gao, Zhichao
@ 2020-04-17 18:14               ` Vitaly Cheptsov
  2020-04-22  1:55                 ` Gao, Zhichao
  0 siblings, 1 reply; 13+ messages in thread
From: Vitaly Cheptsov @ 2020-04-17 18:14 UTC (permalink / raw)
  To: Gao, Zhichao, Rothman, Michael A
  Cc: devel@edk2.groups.io, Andrew Fish, Laszlo Ersek,
	Marvin Häuser, Kinney, Michael D, Ni, Ray


[-- Attachment #1.1: Type: text/plain, Size: 18133 bytes --]

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.gao@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>; vit9696@protonmail.com <mailto:vit9696@protonmail.com>; Rothman, Michael A <michael.a.rothman@intel.com <mailto:michael.a.rothman@intel.com>>
> Cc: Andrew Fish <afish@apple.com <mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Marvin Häuser <mhaeuser@outlook.de <mailto:mhaeuser@outlook.de>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@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.gao@intel.com <mailto:zhichao.gao@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Andrew Fish <afish@apple.com <mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Marvin Häuser <mhaeuser@outlook.de <mailto:mhaeuser@outlook.de>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@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.gao@intel.com <mailto:zhichao.gao@intel.com>> написал(а):
>
> Sorry for my mistake. Then I have no other comments for this patch.
> Reviewed-by: Zhichao Gao <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>
>
> Thanks,
> Zhichao
>
>  <>From: vit9696 <vit9696@protonmail.com <mailto:vit9696@protonmail.com>> 
> Sent: Wednesday, February 19, 2020 8:15 PM
> To: Gao, Zhichao <zhichao.gao@intel.com <mailto:zhichao.gao@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.gao@intel.com <mailto:zhichao.gao@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 <usrsse2@icloud.com <mailto:usrsse2@icloud.com>>
> > Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@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+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub> [zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>]
> > -=-=-=-=-=-=
> 
>
> 


[-- Attachment #1.2: Type: text/html, Size: 53534 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-04-17 18:14               ` Vitaly Cheptsov
@ 2020-04-22  1:55                 ` Gao, Zhichao
  2020-04-22 16:55                   ` Vitaly Cheptsov
  0 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2020-04-22  1:55 UTC (permalink / raw)
  To: vit9696, Rothman, Michael A
  Cc: devel@edk2.groups.io, Andrew Fish, Laszlo Ersek,
	Marvin Häuser, Kinney, Michael D, Ni, Ray

[-- Attachment #1: Type: text/plain, Size: 17989 bytes --]

Hi Vitaly,

I don’t know how to deal with such spec ambiguous issue. So I consult Michael Rothman who is one of the owner of the UEFI spec.
BTW, you can’t change the firmware, why don’t you provide a specific shell spec build by yourselves to fix the issue. Is there any limitation?

Michael Rothman,

Do you have a chance to view this issue? Just view the summary below is enough.

Thanks,
Zhichao

From: vit9696 <vit9696@protonmail.com>
Sent: Saturday, April 18, 2020 2:14 AM
To: Gao, Zhichao <zhichao.gao@intel.com>; Rothman, Michael A <michael.a.rothman@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Marvin Häuser <mhaeuser@outlook.de>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

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.gao@intel.com<mailto:zhichao.gao@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] On Behalf Of Gao, Zhichao
Sent: Thursday, April 2, 2020 2:57 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com>; Rothman, Michael A <michael.a.rothman@intel.com<mailto:michael.a.rothman@intel.com>>
Cc: Andrew Fish <afish@apple.com<mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Marvin Häuser <mhaeuser@outlook.de<mailto:mhaeuser@outlook.de>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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] On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, March 27, 2020 7:01 PM
To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Andrew Fish <afish@apple.com<mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Marvin Häuser <mhaeuser@outlook.de<mailto:mhaeuser@outlook.de>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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@intel.com<mailto:zhichao.gao@intel.com>> написал(а):

Sorry for my mistake. Then I have no other comments for this patch.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>

Thanks,
Zhichao

From: vit9696 <vit9696@protonmail.com<mailto:vit9696@protonmail.com>>
Sent: Wednesday, February 19, 2020 8:15 PM
To: Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@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.gao@intel.com<mailto:zhichao.gao@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
>
> 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@icloud.com<mailto:usrsse2@icloud.com>>
> Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com<mailto:vit9696@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@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>]
> -=-=-=-=-=-=




[-- Attachment #2: Type: text/html, Size: 47332 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-04-22  1:55                 ` Gao, Zhichao
@ 2020-04-22 16:55                   ` Vitaly Cheptsov
  2020-04-24  9:20                     ` Laszlo Ersek
  0 siblings, 1 reply; 13+ messages in thread
From: Vitaly Cheptsov @ 2020-04-22 16:55 UTC (permalink / raw)
  To: Gao, Zhichao, Rothman, Michael A
  Cc: devel@edk2.groups.io, Andrew Fish, Laszlo Ersek,
	Marvin Häuser, Kinney, Michael D, Ni, Ray


[-- Attachment #1.1: Type: text/plain, Size: 19912 bytes --]

Zhichao,

We already build a customised shell for ourselves, but the problem with this change is that it is invasive. To make it work we need to patch EDK II, and this is strongly undesired as maintaining EDK II patches is very tiring. In fact, currently this is the only change we have that needs a patch in EDK II. Since this particular change is something we believe should actually be in EDK II, we decided to upstream it.

In fact, there are really many mistakes in UEFI spec even besides this one, but we do not know where to report them. E.g. last thing we saw was ByRegisterNotify description in LocateHandleBuffer, which was mistakingly copy-pasted from LocateHandle.

Best regards,
Vitaly

> 22 апр. 2020 г., в 04:55, Gao, Zhichao <zhichao.gao@intel.com> написал(а):
> 
> Hi Vitaly,
>
> I don’t know how to deal with such spec ambiguous issue. So I consult Michael Rothman who is one of the owner of the UEFI spec.
> BTW, you can’t change the firmware, why don’t you provide a specific shell spec build by yourselves to fix the issue. Is there any limitation?
>
> Michael Rothman,
>
> Do you have a chance to view this issue? Just view the summary below is enough.
>
> Thanks,
> Zhichao
>
> From: vit9696 <vit9696@protonmail.com> 
> Sent: Saturday, April 18, 2020 2:14 AM
> To: Gao, Zhichao <zhichao.gao@intel.com>; Rothman, Michael A <michael.a.rothman@intel.com>
> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Marvin Häuser <mhaeuser@outlook.de>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
>
> 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.gao@intel.com <mailto:zhichao.gao@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>; vit9696@protonmail.com <mailto:vit9696@protonmail.com>; Rothman, Michael A <michael.a.rothman@intel.com <mailto:michael.a.rothman@intel.com>>
> Cc: Andrew Fish <afish@apple.com <mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Marvin Häuser <mhaeuser@outlook.de <mailto:mhaeuser@outlook.de>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@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.gao@intel.com <mailto:zhichao.gao@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Andrew Fish <afish@apple.com <mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Marvin Häuser <mhaeuser@outlook.de <mailto:mhaeuser@outlook.de>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@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.gao@intel.com <mailto:zhichao.gao@intel.com>> написал(а):
>
> Sorry for my mistake. Then I have no other comments for this patch.
> Reviewed-by: Zhichao Gao <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>
>
> Thanks,
> Zhichao
>
>  <>From: vit9696 <vit9696@protonmail.com <mailto:vit9696@protonmail.com>> 
> Sent: Wednesday, February 19, 2020 8:15 PM
> To: Gao, Zhichao <zhichao.gao@intel.com <mailto:zhichao.gao@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.gao@intel.com <mailto:zhichao.gao@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 <usrsse2@icloud.com <mailto:usrsse2@icloud.com>>
> > Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com <mailto:vit9696@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+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub> [zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>]
> > -=-=-=-=-=-=
> 
>
> 


[-- Attachment #1.2: Type: text/html, Size: 64355 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
  2020-04-22 16:55                   ` Vitaly Cheptsov
@ 2020-04-24  9:20                     ` Laszlo Ersek
  0 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2020-04-24  9:20 UTC (permalink / raw)
  To: vit9696, Rothman, Michael A, Andrew Fish
  Cc: Gao, Zhichao, devel@edk2.groups.io, Marvin Häuser,
	Kinney, Michael D, Ni, Ray

On 04/22/20 18:55, vit9696 wrote:

> In fact, there are really many mistakes in UEFI spec even besides
> this one, but we do not know where to report them. E.g. last thing we
> saw was ByRegisterNotify description in LocateHandleBuffer, which was
> mistakingly copy-pasted from LocateHandle.
Please join the UEFI forum:

https://uefi.org/membership

And then you get access to the Mantis bug tracker where you can file
tickets about UEFI spec issues.

(Most recently, I filed
<https://mantis.uefi.org/mantis/view.php?id=2095> just this Monday,
about some typos in the UEFI spec.)

Personally, I used the "Company Representative Sign Up" form, because
Red Hat had already been a "contributing member" company
<https://uefi.org/members>.

If your company doesn't want that kind of (paid) membership, I think you
-- plural -- could become an "adopter", or you -- singular -- could even
become an "individual adopter".

The page <https://uefi.org/members> lists a number of individual
adopters, so it's definitely a functional process. And I believe
individual adopters can access the Mantis bug tracker.

Michael and Andrew, please correct the above if necessary.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-04-24  9:20 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-10 10:18 [PATCH 0/1] ShellPkg: Add support for input with separately reported modifiers Vitaly Cheptsov
2020-02-10 10:18 ` [PATCH 1/1] " Vitaly Cheptsov
2020-02-14 11:55   ` Vitaly Cheptsov
2020-02-19  6:55   ` [edk2-devel] " Gao, Zhichao
2020-02-19 12:15     ` Vitaly Cheptsov
2020-02-20  0:27       ` Gao, Zhichao
2020-03-27 11:00         ` Vitaly Cheptsov
2020-04-02  6:57           ` Gao, Zhichao
     [not found]           ` <1601EE5ED9DEC05E.18513@groups.io>
2020-04-02  8:28             ` Gao, Zhichao
2020-04-17 18:14               ` Vitaly Cheptsov
2020-04-22  1:55                 ` Gao, Zhichao
2020-04-22 16:55                   ` Vitaly Cheptsov
2020-04-24  9:20                     ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox