Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConsoleKeyInfo improvements. #107410

Closed
wants to merge 2 commits into from
Closed

ConsoleKeyInfo improvements. #107410

wants to merge 2 commits into from

Conversation

meokullu
Copy link

@meokullu meokullu commented Sep 5, 2024

  • New constructor is added. ConsoleKeyInfo(char keyChar, ConsoleKey key)
    New constructor:
 public ConsoleKeyInfo(char keyChar, ConsoleKey key)
 {
     // Limit ConsoleKey values to 0 to 255, but don't check whether the
     // key is a valid value in our ConsoleKey enum.  There are a few
     // values in that enum that we didn't define, and reserved keys
     // that might start showing up on keyboards in a few years.
     if (((int)key) < 0 || ((int)key) > 255)
     {
         throw new ArgumentOutOfRangeException(nameof(key), SR.ArgumentOutOfRange_ConsoleKey);
     }

     _keyChar = keyChar;
     _key = key;
     _mods = 0;
 }
  • On ConsoleKeyInfo(char keyChar, ConsoleyKey key, bool shift, bool alt, bool control) had a wrong conditional statements. Even though only one of Modifiers are used, it was checking them individually.

Old code:

 public ConsoleKeyInfo(char keyChar, ConsoleKey key, bool shift, bool alt, bool control)
 {
     // Limit ConsoleKey values to 0 to 255, but don't check whether the
     // key is a valid value in our ConsoleKey enum.  There are a few
     // values in that enum that we didn't define, and reserved keys
     // that might start showing up on keyboards in a few years.
     if (((int)key) < 0 || ((int)key) > 255)
     {
         throw new ArgumentOutOfRangeException(nameof(key), SR.ArgumentOutOfRange_ConsoleKey);
     }

     _keyChar = keyChar;
     _key = key;
     _mods = 0;
     if (shift)
         _mods |= ConsoleModifiers.Shift;
     if (alt)
         _mods |= ConsoleModifiers.Alt;
     if (control)
         _mods |= ConsoleModifiers.Control;
 }

New code:

 public ConsoleKeyInfo(char keyChar, ConsoleKey key, bool shift, bool alt, bool control)
 {
     // Limit ConsoleKey values to 0 to 255, but don't check whether the
     // key is a valid value in our ConsoleKey enum.  There are a few
     // values in that enum that we didn't define, and reserved keys
     // that might start showing up on keyboards in a few years.
     if (((int)key) < 0 || ((int)key) > 255)
     {
         throw new ArgumentOutOfRangeException(nameof(key), SR.ArgumentOutOfRange_ConsoleKey);
     }

     _keyChar = keyChar;
     _key = key;
     _mods = 0;
     if (shift)
         _mods |= ConsoleModifiers.Shift;
     else if (alt)
         _mods |= ConsoleModifiers.Alt;
     else if (control)
         _mods |= ConsoleModifiers.Control;
 }
  • Override toString() method is added.
 public override string ToString() => $"keyChar: {KeyChar} key: {Key} mods: {Enum.GetName(typeof(ConsoleModifiers), Modifiers)}";

… structure of _mods on ConsoleKeyInfo and adding override ToString() method for ConsoleKeyInfo.
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 5, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

@meokullu
Copy link
Author

meokullu commented Sep 5, 2024

@dotnet-policy-service agree

@huoyaoyuan
Copy link
Member

  • New constructor is added.

This should not be done in a pull request first. Instead, it must go through https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md. Since ConsoleKeyInfo is more created by the framework, improvement in constructor is not so valueable.

  • On ConsoleKeyInfo(char keyChar, ConsoleyKey key, bool shift, bool alt, bool control) had a wrong conditional statements. Even though only one of Modifiers are used, it was checking them individually.

This is incorrect. The |= operator uses every modifier present. If you specify shift: true, alt: false, control: true, you will get Shift|Control as the modifier.

  • Override toString() method is added.

This technically also falls into the new api category.

@jozkee jozkee closed this Sep 5, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Console community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants