Skip to content

Conversation

243083df
Copy link

@243083df 243083df commented Dec 2, 2021

PR Summary

toUnicode sticks with first encoutered locale. During some research i use toUnicodeEx and provide current used locale to it.
Resolve #2865

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests
  • Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
  • User-facing changes
    • Not Applicable
Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Dec 2, 2021

CLA assistant check
All CLA requirements met.

@@ -147,7 +157,8 @@ internal static void TryGetCharFromConsoleKey(ConsoleKeyInfo key, ref char resul
{
flags |= (1 << 2); /* If bit 2 is set, keyboard state is not changed (Windows 10, version 1607 and newer) */
}
int charCount = ToUnicode(virtualKey, scanCode, state, chars, chars.Length, flags);
uint layout = GetKeyboardLayout(GetWindowThreadProcessId(GetForegroundWindow(), 0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always felt like a bug in console applications. I believe the conhost process receives the keyboard layout change notifications, but the ToUnicode call happens in a different process that never sees layout change notifications. I may have even opened a bug on ToUnicode that might have been resolved as won't fix over concerns about breaking something.

This change may seem work well enough in practice, but there is a race condition:

  • Type a character
  • New popup window gets created by some background process
  • Call to GetForegroundWindow sees this new window instead of the console/vscode/etc. window.

I'm also not excited about seeing GetWindowThreadProcessId called on every key - it seems like it should be cached. Or maybe better - can we find the conhost/openconsole process instead? That process should see the correct layout, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lzybkr I create replacement for GetForegroundWindow. What do you think about it?

// aka GetForegroundWindow
static Process findParentWithWindowHandle()
        {
            var current = Process.GetCurrentProcess();
            while (current != null)
            {
                if (current.MainWindowHandle != IntPtr.Zero)
                {
                    break;
                }
                current = GetParentProcess(current.Handle);
            }
            return current;
        }

 static Process GetParentProcess(IntPtr handle)
        {
            ProcessInformation processInformation = new ProcessInformation();

            int returnLength;
            int status = NtQueryInformationProcess(handle, 0, ref processInformation, Marshal.SizeOf(processInformation), out returnLength);
            if (status != 0)
                throw new Win32Exception(status);

            try
            {
                return Process.GetProcessById(processInformation.InheritedFromUniqueProcessId.ToInt32());
            }
            catch (ArgumentException)
            {
                // not found
                return null;
            }
        }
        [StructLayout(LayoutKind.Sequential)]
        public struct ProcessInformation
        {
            internal IntPtr Reserved1;
            internal IntPtr PebBaseAddress;
            internal IntPtr Reserved2_0;
            internal IntPtr Reserved2_1;
            internal IntPtr UniqueProcessId;
            internal IntPtr InheritedFromUniqueProcessId;

        }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like it would work well in Windows Terminal, VSCode, and the ISE, but it won't work with conhost because the parent will be explorer. conhost.exe is a sibling process.

Copy link
Contributor

@lzybkr lzybkr Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you can use something like GetProcessHandle(GetConsoleWindow()) to get the correct conhost process - I expect this wouldn't work in Windows Terminal.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I research this theme deeper and find out that all approaches that find window that subsribed to locale change does not work in all situations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that replacing https://github.com/PowerShell/PSReadLine/pull/3078/files#diff-9ab1ecb1e5d0ebd879a483eada4f6fcf2b407c662752f52bea29e45fd143f1a5R242
to c == '\0' do not break test.
And so far i didn't find dead key what start with different from '\0' code. Can it help?

@243083df
Copy link
Author

This is all what can i do. Better solution immposible with my skills.

@lzybkr
Copy link
Contributor

lzybkr commented May 24, 2022

@DHowett - maybe you have some insights here?

@DHowett
Copy link
Contributor

DHowett commented May 24, 2022

Adding this to my list for tomorrow. My team is starting talent reviews, so I can't promise that I'll get to it timely.

@243083df
Copy link
Author

any update?

@243083df
Copy link
Author

bump

@243083df
Copy link
Author

How can i build custom verion of PSReadLine and install to PS?

@kun1ck
Copy link

kun1ck commented Sep 14, 2022

How can i build custom verion of PSReadLine and install to PS?

Hello, is there a working solution now?

@ForNeVeR
Copy link
Contributor

@lzybkr @DHowett, sorry, folks, but could someone of you please take a look at this? Or direct us to someone who can review this at the moment, please. We're looking for feedback on this PR.

@243083df
Copy link
Author

Hello, is there a working solution now?
This is fine to me, but i dont know how to install it to system

@kun1ck
Copy link

kun1ck commented Oct 22, 2022

Adding this to my list for tomorrow. My team is starting talent reviews, so I can't promise that I'll get to it timely.

Hello, how soon can you check it out?

@daxian-dbw
Copy link
Member

Superseded by #3786. See #3786 (comment) for the proposal to solve the two concerns raised in #3078 (comment).

@daxian-dbw daxian-dbw closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cyrillic breaks CTRL+ combos
7 participants