ReadKey has the KeyChar set to 0 if a modifier is held

gratejames commented 1 year ago

Area of Cosmos - What area of Cosmos are we dealing with?

ConsoleKeyInfo/Console.ReadKey()

Expected Behaviour - What do you think that should happen?

Console.ReadKey().KeyChar should be useable even if a modifier is held

Actual Behaviour - What unexpectedly happens?

ConsoleKeyInfo cki = Console.ReadKey(true);
bool ctrl = (cki.Modifiers & ConsoleModifiers.Control) != 0;
if (ctrl) {
    Console.WriteLine("S'" + cki.KeyChar.ToString() + "',I'" + ((int)cki.KeyChar).ToString() + "'");
}

When control is pressed, the first write produces a space, the second a 0

Reproduction - How did you get this error to appear?

The above code snippet alone within beforeRun() is a minimum working example (In a while true for testing)

Version - Were you using the User Kit or Dev Kit? And what User Kit version or Dev Kit commit (Cosmos, IL2CPU, X#)?

Newest UserKit afaik, can't find the version number

jvyden commented 1 year ago

Seems like a flaw with how keyboard layouts are implemented in COSMOS. An individual KeyMapping has individual characters for each combination of modifiers:

        /// <summary>
        /// Initializes a new instance of the <see cref="KeyMapping"/> class.
        /// </summary>
        /// <param name="scanCode">The physical scan code of the key.</param>
        /// <param name="normal">The text character value of the key with no modifiers being active.</param>
        /// <param name="shift">The text character value of the key with the Shift modifier being active.</param>
        /// <param name="num">The text character value of the key with the Num Lock modifier being active.</param>
        /// <param name="caps">The text character value of the key with the Caps Lock modifier being active.</param>
        /// <param name="shiftCapsLock">The text character value of the key with the Shift and Caps Lock modifiers being active.</param>
        /// <param name="shiftNumLock">The text character value of the key with the Shift and Num Lock modifiers being active</param>
        /// <param name="ctrlAlt">The text character value of the key with the Control and Alt modifiers being active.</param>
        /// <param name="ctrlAltShift">The text character value of the key with the Control, Alt, and Shift modifiers being active</param>
        /// <param name="ctrl">The text character value of the key with the Control modifier being active.</param>
        /// <param name="shiftCtrl">The text character value of the key with the Shift and Control modifiers being active.</param>
        /// <param name="key">The virtual key that the physical key-press maps to.</param>
        /// <param name="numKey">The virtual key that the physical key-press maps to when the Num Lock modifier is active..</param>
        public KeyMapping(byte scanCode, char normal, char shift, char num, char caps, char shiftCapsLock, char shiftNumLock, char ctrlAlt, char ctrlAltShift, char ctrl, char shiftCtrl, ConsoleKeyEx key, ConsoleKeyEx numKey)
        {
            ScanCode = scanCode;
            Value = normal;
            Shift = shift;
            NumLock = num;
            CapsLock = caps;
            ShiftCapsLock = shiftCapsLock;
            ShiftNumLock = shiftNumLock;
            Key = key;
            NumLockKey = key;
            ControlAlt = ctrlAlt;
            Control = ctrl;
            ControlAltShift = ctrlAltShift;
            ControlShift = shiftCtrl;
            NumLockKey = numKey;
        }

For example, the US standard keyboard layout uses an overload of the constructor that specifies \0 for all keys holding control:

        public KeyMapping(byte scanCode, char normal, char shift, char numLock, char capsLock, char shiftCapsLock, char shiftNumLock, ConsoleKeyEx key)
            : this(scanCode, normal, shift, numLock, capsLock, shiftCapsLock, shiftNumLock, ctrlAlt: '\0', ctrlAltShift: '\0', ctrl: '\0', shiftCtrl: '\0', key)
        {
        }

So, it depends on whatever adds the keyboard layout to specify the correct char value for ctrl. I could change the constructors to simply specify the same key when ctrl is pressed, but it would definitely be a breaking change I feel.

Not sure why mappings were done this way, but I'd like to hear a maintainer's opinion on this approach.

quajak commented 1 year ago

Looking at https://github.com/CosmosOS/Cosmos/blob/a6f72dba387b3bdd0cef769b148cc2bd801c82e8/source/Cosmos.System2/Keyboard/KeyboardManager.cs#L89 and https://github.com/CosmosOS/Cosmos/blob/a6f72dba387b3bdd0cef769b148cc2bd801c82e8/source/Cosmos.System2/Keyboard/KeyboardManager.cs#L180 I don't think the modifier keys should ever show up when using ReadKey and that is the actual issue here. I guess we also have the question what key should be returned when ctrl+space is pressed for example, since that usually does not correspond to any actual char value.

Regarding why the mappings were made that way, I do not have any idea.

CosmosOS / Cosmos