Re: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console

[Ni, Ruiyu]

Can you check which keyboard driver are you using?
The keyboard driver is expected to translate "SHIFT" + "3" to "#" (without 
Shift state).
I know that some keyboard driver doesn't do that correctly.
E.g.: SHIFT + "3" is translated to "#" but the SHIFT state is not masked off.

[Sorry I thought I sent the mail out days ago]

> -Original Message-
> From: jim.dai...@dell.com [mailto:jim.dai...@dell.com]
> Sent: Wednesday, May 23, 2018 3:01 AM
> To: Ni, Ruiyu 
> Cc: Carsey, Jaben ; fel...@mail.ru; edk2-
> de...@lists.01.org
> Subject: RE: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read
> console
> 
> Ray,
> 
> The patch in the message below was applied to the tree on 12 Feb this year
> (5563281fa2b31093a1cbd415553b9264c5136e89).
> 
> Part of the change to MainTextEditor.c causes an issue where I cannot enter 
> (at
> least some) shifted punctuation.  For example, after this check in I cannot 
> edit a
> shell script and create a comment because I cannot enter the "#" character.
> When I try to type "#", the status bar simply shows "Unknown Command".
> 
> I don't really understand the change, but if in the MainEditorKeyInput 
> function in
> file MainTextEditor.c I delete the "NoShiftState" check from the first "else 
> if"
> below:
> 
> +//
> +// dispatch to different components' key handling function
> +//
> +if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey()) {
> +  Status = EFI_SUCCESS;
> +} else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) ||
> ((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <=
> SCAN_PAGE_DOWN {
> +  Status = FileBufferHandleInput ();
> +} else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) &&
> (KeyData.Key.ScanCode <= SCAN_F12)) {
> +  Status = MenuBarDispatchFunctionKey ();
> +} else {
> +  StatusBarSetStatusString (L"Unknown Command");
> +  FileBufferMouseNeedRefresh = FALSE;
> +}
> 
> then I am able to enter "#" and other characters that I previously was unable 
> to
> enter.
> 
> Can you have a look at this?  I assume any shell binary built with this 
> change will
> have a similar issue.
> 
> Thanks,
> Jim
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu
> Ni
> Sent: Monday, February 12, 2018 9:34 AM
> To: edk2-devel@lists.01.org
> Cc: Jaben Carsey; Felix
> Subject: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=682
> 
> Edit and HexEdit commands assume that SimpleTxtIn translates
> Ctrl+ key combinations into Unicode control characters
> (0x1-0x1A).
> 
> Such translation does not seem to be required by the UEFI spec.
> Shell should not rely on implementation specific behavior.
> It should instead use SimpleTextInEx to read Ctrl+ key 
> combinations.
> 
> The patch changes edit and hexedit to only consumes SimpleTextInEx.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Reported-by: Felix 
> Cc: Felix 
> Cc: Jaben Carsey 
> ---
>  .../Edit/MainTextEditor.c  | 135 +-
>  .../Edit/TextEditorTypes.h |  21 ++-
>  .../UefiShellDebug1CommandsLib/EditInputBar.c  |  34 +++-
>  .../UefiShellDebug1CommandsLib/EditInputBar.h  |   6 +-
>  .../UefiShellDebug1CommandsLib/EditMenuBar.c   |  38 +++-
>  .../UefiShellDebug1CommandsLib/EditMenuBar.h   |   6 +-
>  .../HexEdit/HexEditorTypes.h   |  25 +--
>  .../HexEdit/MainHexEditor.c| 205 
> +
>  8 files changed, 309 insertions(+), 161 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> index 14f51dff19..a197f80a40 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> @@ -1,7 +1,7 @@
>  /** @file
>Implements editor interface functions.
> 
> -  Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved. 
> +  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.
> + 
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distributi

Re: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console

[Jim.Dailey]

Ray,

The patch in the message below was applied to the tree on 12 Feb this
year (5563281fa2b31093a1cbd415553b9264c5136e89).

Part of the change to MainTextEditor.c causes an issue where I cannot
enter (at least some) shifted punctuation.  For example, after this
check in I cannot edit a shell script and create a comment because I
cannot enter the "#" character.  When I try to type "#", the status bar
simply shows "Unknown Command".

I don't really understand the change, but if in the MainEditorKeyInput
function in file MainTextEditor.c I delete the "NoShiftState" check
from the first "else if" below:

+//
+// dispatch to different components' key handling function
+//
+if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey()) {
+  Status = EFI_SUCCESS;
+} else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) || 
((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <= 
SCAN_PAGE_DOWN {
+  Status = FileBufferHandleInput ();
+} else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) && 
(KeyData.Key.ScanCode <= SCAN_F12)) {
+  Status = MenuBarDispatchFunctionKey ();
+} else {
+  StatusBarSetStatusString (L"Unknown Command");
+  FileBufferMouseNeedRefresh = FALSE;  
+}

then I am able to enter "#" and other characters that I previously was
unable to enter.

Can you have a look at this?  I assume any shell binary built with
this change will have a similar issue.

Thanks,
Jim

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Monday, February 12, 2018 9:34 AM
To: edk2-devel@lists.01.org
Cc: Jaben Carsey; Felix
Subject: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=682

Edit and HexEdit commands assume that SimpleTxtIn translates
Ctrl+ key combinations into Unicode control characters
(0x1-0x1A).

Such translation does not seem to be required by the UEFI spec.
Shell should not rely on implementation specific behavior.
It should instead use SimpleTextInEx to read Ctrl+ key
combinations.

The patch changes edit and hexedit to only consumes SimpleTextInEx.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
Reported-by: Felix <fel...@mail.ru>
Cc: Felix <fel...@mail.ru>
Cc: Jaben Carsey <jaben.car...@intel.com>
---
 .../Edit/MainTextEditor.c  | 135 +-
 .../Edit/TextEditorTypes.h |  21 ++-
 .../UefiShellDebug1CommandsLib/EditInputBar.c  |  34 +++-
 .../UefiShellDebug1CommandsLib/EditInputBar.h  |   6 +-
 .../UefiShellDebug1CommandsLib/EditMenuBar.c   |  38 +++-
 .../UefiShellDebug1CommandsLib/EditMenuBar.h   |   6 +-
 .../HexEdit/HexEditorTypes.h   |  25 +--
 .../HexEdit/MainHexEditor.c| 205 +
 8 files changed, 309 insertions(+), 161 deletions(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
index 14f51dff19..a197f80a40 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
@@ -1,7 +1,7 @@
 /** @file
   Implements editor interface functions.
 
-  Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved. 
+  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved. 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -1362,7 +1362,9 @@ MainCommandDisplayHelp (
 {
   INT32   CurrentLine;
   CHAR16  *InfoString;
-  EFI_INPUT_KEY   Key;
+  EFI_KEY_DATAKeyData;
+  EFI_STATUS  Status;
+  UINTN   EventIndex;
   
   //
   // print helpInfo  
@@ -1371,14 +1373,39 @@ MainCommandDisplayHelp (
 InfoString = HiiGetString(gShellDebug1HiiHandle, 
MainMenuHelpInfo[CurrentLine], NULL);
 ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
   }
-  
+
   //
   // scan for ctrl+w
   //
-  do {
-gST->ConIn->ReadKeyStroke (gST->ConIn, );
-  } while(SCAN_CONTROL_W != Key.UnicodeChar); 
+  while (TRUE) {
+Status = gBS->WaitForEvent (1, >WaitForKeyEx, 
);
+if (EFI_ERROR (Status) || (EventIndex != 0)) {
+  continue;
+}
+Status = MainEditor.TextInputEx->ReadKeyStrokeEx (MainEditor.TextInputEx, 
);
+if (EFI_ERROR (Status)) {
+  continue;
+}
 
+if ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) {
+  //
+  // For consoles that

Re: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Ni, Ruiyu
> Sent: Monday, February 12, 2018 7:34 AM
> To: edk2-devel@lists.01.org
> Cc: Felix ; Carsey, Jaben 
> Subject: [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console
> Importance: High
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=682
> 
> Edit and HexEdit commands assume that SimpleTxtIn translates
> Ctrl+ key combinations into Unicode control characters
> (0x1-0x1A).
> 
> Such translation does not seem to be required by the UEFI spec.
> Shell should not rely on implementation specific behavior.
> It should instead use SimpleTextInEx to read Ctrl+ key
> combinations.
> 
> The patch changes edit and hexedit to only consumes SimpleTextInEx.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Reported-by: Felix 
> Cc: Felix 
> Cc: Jaben Carsey 
> ---
>  .../Edit/MainTextEditor.c  | 135 +-
>  .../Edit/TextEditorTypes.h |  21 ++-
>  .../UefiShellDebug1CommandsLib/EditInputBar.c  |  34 +++-
>  .../UefiShellDebug1CommandsLib/EditInputBar.h  |   6 +-
>  .../UefiShellDebug1CommandsLib/EditMenuBar.c   |  38 +++-
>  .../UefiShellDebug1CommandsLib/EditMenuBar.h   |   6 +-
>  .../HexEdit/HexEditorTypes.h   |  25 +--
>  .../HexEdit/MainHexEditor.c| 205 
> +
>  8 files changed, 309 insertions(+), 161 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> index 14f51dff19..a197f80a40 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> +++
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> @@ -1,7 +1,7 @@
>  /** @file
>Implements editor interface functions.
> 
> -  Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved. 
> +  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved. 
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution.  The full text of the license may be
> found at
> @@ -1362,7 +1362,9 @@ MainCommandDisplayHelp (
>  {
>INT32   CurrentLine;
>CHAR16  *InfoString;
> -  EFI_INPUT_KEY   Key;
> +  EFI_KEY_DATAKeyData;
> +  EFI_STATUS  Status;
> +  UINTN   EventIndex;
> 
>//
>// print helpInfo
> @@ -1371,14 +1373,39 @@ MainCommandDisplayHelp (
>  InfoString = HiiGetString(gShellDebug1HiiHandle,
> MainMenuHelpInfo[CurrentLine], NULL);
>  ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
>}
> -
> +
>//
>// scan for ctrl+w
>//
> -  do {
> -gST->ConIn->ReadKeyStroke (gST->ConIn, );
> -  } while(SCAN_CONTROL_W != Key.UnicodeChar);
> +  while (TRUE) {
> +Status = gBS->WaitForEvent (1, >WaitForKeyEx,
> );
> +if (EFI_ERROR (Status) || (EventIndex != 0)) {
> +  continue;
> +}
> +Status = MainEditor.TextInputEx->ReadKeyStrokeEx
> (MainEditor.TextInputEx, );
> +if (EFI_ERROR (Status)) {
> +  continue;
> +}
> 
> +if ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) {
> +  //
> +  // For consoles that don't support shift state reporting,
> +  // CTRL+W is translated to L'W' - L'A' + 1.
> +  //
> +  if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> +break;
> +  }
> +} else if (((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 shift state reporting,
> +  // make sure that only CONTROL shift key is pressed.
> +  //
> +  if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar ==
> 'W')) {
> +break;
> +  }
> +}
> +  }
>//
>// update screen with file buffer's info
>//
> @@ -1407,6 +1434,7 @@ EFI_EDITOR_GLOBAL_EDITOR  MainEditorConst =
> {
>  0
>},
>NULL,
> +  NULL,
>FALSE,
>NULL
>  };
> @@ -1452,6 +1480,19 @@ MainEditorInit (
>  &(MainEditor.ScreenSize.Row)
>  );
> 
> +  //
> +  // Find TextInEx in System Table ConsoleInHandle
> +  // Per UEFI Spec, TextInEx is required for a console capable platform.
> +  //
> +  Status = gBS->HandleProtocol (
> +  gST->ConsoleInHandle,
> +  ,
> +  (VOID**)
> +  );
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
>//
>// Find mouse in System Table ConsoleInHandle
>//
> @@ -1521,7 +1562,7 @@ MainEditorInit 

[edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console

[Ruiyu Ni]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=682

Edit and HexEdit commands assume that SimpleTxtIn translates
Ctrl+ key combinations into Unicode control characters
(0x1-0x1A).

Such translation does not seem to be required by the UEFI spec.
Shell should not rely on implementation specific behavior.
It should instead use SimpleTextInEx to read Ctrl+ key
combinations.

The patch changes edit and hexedit to only consumes SimpleTextInEx.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Reported-by: Felix 
Cc: Felix 
Cc: Jaben Carsey 
---
 .../Edit/MainTextEditor.c  | 135 +-
 .../Edit/TextEditorTypes.h |  21 ++-
 .../UefiShellDebug1CommandsLib/EditInputBar.c  |  34 +++-
 .../UefiShellDebug1CommandsLib/EditInputBar.h  |   6 +-
 .../UefiShellDebug1CommandsLib/EditMenuBar.c   |  38 +++-
 .../UefiShellDebug1CommandsLib/EditMenuBar.h   |   6 +-
 .../HexEdit/HexEditorTypes.h   |  25 +--
 .../HexEdit/MainHexEditor.c| 205 +
 8 files changed, 309 insertions(+), 161 deletions(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
index 14f51dff19..a197f80a40 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
@@ -1,7 +1,7 @@
 /** @file
   Implements editor interface functions.
 
-  Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved. 
+  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved. 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -1362,7 +1362,9 @@ MainCommandDisplayHelp (
 {
   INT32   CurrentLine;
   CHAR16  *InfoString;
-  EFI_INPUT_KEY   Key;
+  EFI_KEY_DATAKeyData;
+  EFI_STATUS  Status;
+  UINTN   EventIndex;
   
   //
   // print helpInfo  
@@ -1371,14 +1373,39 @@ MainCommandDisplayHelp (
 InfoString = HiiGetString(gShellDebug1HiiHandle, 
MainMenuHelpInfo[CurrentLine], NULL);
 ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
   }
-  
+
   //
   // scan for ctrl+w
   //
-  do {
-gST->ConIn->ReadKeyStroke (gST->ConIn, );
-  } while(SCAN_CONTROL_W != Key.UnicodeChar); 
+  while (TRUE) {
+Status = gBS->WaitForEvent (1, >WaitForKeyEx, 
);
+if (EFI_ERROR (Status) || (EventIndex != 0)) {
+  continue;
+}
+Status = MainEditor.TextInputEx->ReadKeyStrokeEx (MainEditor.TextInputEx, 
);
+if (EFI_ERROR (Status)) {
+  continue;
+}
 
+if ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) {
+  //
+  // For consoles that don't support shift state reporting,
+  // CTRL+W is translated to L'W' - L'A' + 1.
+  //
+  if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
+break;
+  }
+} else if (((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 shift state reporting,
+  // make sure that only CONTROL shift key is pressed.
+  //
+  if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 
'W')) {
+break;
+  }
+}
+  }
   //
   // update screen with file buffer's info
   //
@@ -1407,6 +1434,7 @@ EFI_EDITOR_GLOBAL_EDITOR  MainEditorConst = {
 0
   },
   NULL,
+  NULL,
   FALSE,
   NULL
 };
@@ -1452,6 +1480,19 @@ MainEditorInit (
 &(MainEditor.ScreenSize.Row)
 );
 
+  //
+  // Find TextInEx in System Table ConsoleInHandle
+  // Per UEFI Spec, TextInEx is required for a console capable platform.
+  //
+  Status = gBS->HandleProtocol (
+  gST->ConsoleInHandle,
+  ,
+  (VOID**)
+  );
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
   //
   // Find mouse in System Table ConsoleInHandle
   //
@@ -1521,7 +1562,7 @@ MainEditorInit (
 return EFI_LOAD_ERROR;
   }
 
-  InputBarInit ();
+  InputBarInit (MainEditor.TextInputEx);
 
   Status = FileBufferInit ();
   if (EFI_ERROR (Status)) {
@@ -1794,9 +1835,11 @@ MainEditorKeyInput (
   VOID
   )
 {
-  EFI_INPUT_KEY Key;
+  EFI_KEY_DATA  KeyData;
   EFI_STATUSStatus;
   EFI_SIMPLE_POINTER_STATE  MouseState;
+  UINTN EventIndex;
+  BOOLEAN   NoShiftState;
 
   do {
 
@@ -1831,46 +1874,52 @@ MainEditorKeyInput (
   }
 }
 
-Status = gST->ConIn->ReadKeyStroke (gST->ConIn,