Make ConsoleHost honor NoEcho on Unix platforms - #3801
Conversation
|
I should add that I haven't been able to figure out a way to actually test this other than manually. I investigated how the corefx guys test Console.ReadKey, and they do it manually. That is, their automation calls a test which then requires that a user type keys which are then validated by the user. |
There was a problem hiding this comment.
key = Console.ReadKey(options.HasFlag(NoEcho))?
There was a problem hiding this comment.
fixed
There was a problem hiding this comment.
Actually, we should avoid HasFlag, because it's much, much more expensive than a simple bitwise test. HasFlag calls into a native implementation -- see the implementation here: https://github.com/dotnet/coreclr/blob/e67851210d1c03d730a3bc97a87e8a6713bbf772/src/vm/reflectioninvocation.cpp#L3699
61603c5 to
c950d8d
Compare
There was a problem hiding this comment.
Could you please remove the extra empty line?
|
Title updated -- it's recommended to briefly describe what is fixed in the PR title, and put "Fix #" in the PR's body. See the |
There was a problem hiding this comment.
Please avoid the use of the HasFlag api - it is much, much slower than the simple bit test - see the implementation here. The expensive part is mostly boxing, but you can see it does other work before the simple bitwise test.
There was a problem hiding this comment.
it would be better to get uniform input - one person's opinion vs another. My original code was requested to change to this
There was a problem hiding this comment.
Sorry, I don't see that feedback, so I can't comment on that.
It's not an opinion that the api is significantly slower, e.g. see here - that's the second hit for me on this search
My preference is to avoid the api completely because:
- We do have places where the api would make PowerShell measurably slower
- People working in our code base should be comfortable reading and writing code that is equivalent to this api
- People tend to copy the style, so avoiding the api helps avoid usage creeping into places that matter.
Again, this is just feedback - I'm not blocking the PR.
There was a problem hiding this comment.
i agree completely, there's even an open issue in corefx (https://github.com/dotnet/corefx/issues/15453). My original code did not use hasflag, which I will put back.
This does not provide support for IncludeKeyDown or IncludeKeyUp, but does not echo the character when provided. In order to support IncludeKeyDown and IncludeKeyUp a more substantial rewrite will be needed as Console.ReadKey doesn't support this behavior. This applies to both OSX and Linux.
c950d8d to
10625c3
Compare
|
I think all the feedback has been addressed Dongbo Wang (@daxian-dbw) ? |
Fix #3183
this also affects Linux as well.
This does not provide support for IncludeKeyDown or IncludeKeyUp, but does not
echo the character when provided. In order to support IncludeKeyDown and IncludeKeyUp a
more substantial rewrite will be needed as Console.ReadKey doesn't support this behavior.