-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
AzCon: improve input, usability, reliability (4 commits) #4756
Conversation
This commit replaces the AzureConnection's input handler with one that acts more like "getline()". Instead of the Read thread setting a state and WriteInput filling in the right member variable, the reader blocks on the user's input and receives it in an optional<string>. This moves the input number parsing and error case handling closer to the point where those inputs are used, as opposed to where they're collected. It also switches our input to be "line-based", which is a huge boon for typing tenant numbers >9. This fixes #3233. A simple line editor (supporting only backspace and CR) is included. It also enables echo on user input, and prints it in a nice pretty green color. It also enables input queueing: if the user types anything before the connection is established, it'll be sent once it is. Fixes #3233.
This commit colorizes parts of the AzCon's strings that include "user options" -- things the user can type -- in yellow. This is to help with accessibility. The implementation here is based on a discussion with the team. Alternative options for coloration were investigated, such as: * Embedding escape sequences in the resource file. This would have been confusing for translators. The RESW file format doesn't support  escapes, so we would need some magic post-processing. * Embedding "markup" in the resource file (like #{93m}, ...) This still would have been annoying for translators. We settled on an implementation that takes resource names, colorizes them, and string-formats them into other resources.
5bfff2b
to
daf32ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somehow got into a state where shell.azure.com returns an Internal Server Error, but Windows Terminal's Azure Cloud Shell works just fine. I consider that a win and MAJOR props to you hahaha
|
||
_currentInputMode = mode; | ||
|
||
_TerminalOutputHandlers(L"\x1b[92m"); // Make prompted user input green |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make 92 a static constexpr int
like the others above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's either this or we allocate a dynamically generated string just to insert the 92 into it because it needs to be string-formatted.
I guess this whole string could be a const.
return _currentInputMode != mode || _isStateAtOrBeyond(ConnectionState::Closing); | ||
}); | ||
|
||
_TerminalOutputHandlers(L"\x1b[m"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do? I don't speak VT too good yet 😋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme guess, stop coloring the output? Regardless, could you add a little comment here like you did above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. sure.
This pull request comprises four commits that improve the Azure connection.
Azure: rewrite user input handler
This commit replaces the AzureConnection's input handler with one that
acts more like "getline()". Instead of the Read thread setting a state
and WriteInput filling in the right member variable, the reader blocks
on the user's input and receives it in an optional.
This moves the input number parsing and error case handling closer to
the point where those inputs are used, as opposed to where they're
collected.
It also switches our input to be "line-based", which is a huge boon for
typing tenant numbers >9. This fixes #3233. A simple line editor
(supporting only backspace and CR) is included.
It also enables echo on user input, and prints it in a nice pretty green
color.
It also enables input queueing: if the user types anything before the
connection is established, it'll be sent once it is.
Fixes #3233.
Azure: display the user's options and additional information in color
This commit colorizes parts of the AzCon's strings that include "user
options" -- things the user can type -- in yellow. This is to help with
accessibility.
The implementation here is based on a discussion with the team.
Alternative options for coloration were investigated, such as:
This would have been confusing for translators.
The RESW file format doesn't support  escapes, so we would need
some magic post-processing.
This still would have been annoying for translators.
We settled on an implementation that takes resource names, colorizes
them, and string-formats them into other resources.
Azure: follow the user's shell choice from the online portal
Fixes #2266.
Azure: remove all credentials instead of just the first one
just a silly bug.