Introduce parsed command line text to command palette by Don-Vito · Pull Request #8515 · microsoft/terminal

Don-Vito · 2020-12-07T14:46:40Z

This commit introduces another optional text block in palette that will
be shown in the command line mode (above the history). This text block
will either contain a list of parsed command lines or a description why
the parsing failed

Closes #8344
Closes #7284

Don-Vito · 2020-12-07T14:50:25Z

github-actions · 2020-12-07T15:34:22Z

New misspellings found, please review:

pased

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"AAAAA Bopomofo CParams CSV GENERATEPROJECTPRIFILE hhhh Horiz IHosted Inlines MAKEINTRESOURCEA mousemode renamer
 Reserialize rgus SGRXY tcon UDK UDKs Unfocus xe xlang "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/e0739c2b718f82a8e9ef6a89f9144df9da135ce9.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"csv horiz inlines pased Renamer reserialize udk unfocus "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/e0739c2b718f82a8e9ef6a89f9144df9da135ce9.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spell-check/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

Carlos Zamora (carlos-zamora)

This looks great! Thanks!

Carlos Zamora (carlos-zamora) · 2020-12-07T17:52:59Z

+                    const auto& commands = appArgs.GetStartupActions();
+                    if (commands.size() > 0)
+                    {
+                        std::wstring commandDescription{ RS_(L"CommandPalette_ParsedCommandLine") + L":" };


(asking the team) Not too familiar with how localization works, but should we move the : to the resw?

Dustin L. Howett (@DHowett) would know for sure, but my guess would be yes, we need to move it to the resw. I'm thinking the resource would need to be

Executing command line will invoke the following commands:{0}

and then we fmt::format that with the list of generated names (joined with \n\t)

I do agree that : should be there. My idea was not to include {0} so if we decide to put them in different Runs (e.g. make this caption red or green) we won't need new translation. WDYT?

Yeah, I actually agree with that. We may want the actions to each be a separate run of text for whatever reason.

Carlos Zamora (carlos-zamora) · 2020-12-07T17:56:23Z

@msftbot make sure Mike Griese (@zadjii-msft) signs off on this

Deleted user (ghost) · 2020-12-07T17:56:31Z

Hello Carlos Zamora (@carlos-zamora)!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by Mike Griese (@zadjii-msft)

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Don-Vito · 2020-12-09T20:35:04Z

Mike Griese (@zadjii-msft) - do you like it? Do we need someone from the product / accessibility to review it?

Mike Griese (zadjii-msft) · 2020-12-09T20:45:06Z

I don't like it.

I love it.

I just need to find some time to get back to reviewing it. Don't worry, it's the next highest thing on my todo after getting the SUI reviewed.

Don-Vito · 2020-12-09T20:49:20Z

I don't like it.

I love it.

I just need to find some time to get back to reviewing it. Don't worry, it's the next highest thing on my todo after getting the SUI reviewed.

Thanks! Not urgent (it is quite standalone). Was asking since I solved it quite quick and dirty, so was wondering if improvements are required 😊

Dustin L. Howett (DHowett) · 2020-12-09T20:58:13Z

Will this conflict with your command palette model changes?

Don-Vito · 2020-12-09T21:00:09Z

Will this conflict with your command palette model changes?

A bit. But I am not worried 😊
I mean I can adjust it quite quickly if required 😊

Don-Vito · 2020-12-10T06:26:54Z

So it appears not to conflict with the model change at all. Noice.

Mike Griese (zadjii-msft)

Okay the resources thing is such a nit, but it probably does need to be changed. The rest of this looks great. I'm really glad we got both the error text and the parsed commands into the text block. It's so crisp

Mike Griese (zadjii-msft) · 2020-12-11T14:09:51Z

+            if (!commandLine.empty())
+            {
+                ExecuteCommandlineArgs args{ commandLine };
+                ::TerminalApp::AppCommandlineArgs appArgs;


Part of me wonders if the CommandPalette should just keep a single AppCommandlineArgs instance as a member, and then reset it and re-use it for parsing each time.

That being said, this solution certainly does work

Mike Griese (@zadjii-msft) - I see your point. I allowed this to myself, since today we do allocate an instance upon every execution. However, now we will actually allocate an instance on every character in the search - so it makes sense to cache it. I will see if there is a good way to reset the instance - since the internal reset we have there now is partial IIRC.

Mike Griese (zadjii-msft) · 2020-12-11T14:13:49Z

+                    const auto& commands = appArgs.GetStartupActions();
+                    if (commands.size() > 0)
+                    {
+                        std::wstring commandDescription{ RS_(L"CommandPalette_ParsedCommandLine") + L":" };


Dustin L. Howett (@DHowett) would know for sure, but my guess would be yes, we need to move it to the resw. I'm thinking the resource would need to be

Executing command line will invoke the following commands:{0}

and then we fmt::format that with the list of generated names (joined with \n\t)

Mike Griese (zadjii-msft) · 2020-12-11T14:15:52Z

+                }
+                else
+                {
+                    ParsedCommandLineText(RS_(L"CommandPalette_FailedParsingCommandLine") + L":\n\t" + til::u8u16(appArgs.GetExitMessage()));


Same down here, we should use fmt::format to add the error text to the resource here.

NoticeFontNotFound in the TerminalControl project is a good example

Same as above

…-predict

Mike Griese (zadjii-msft)

Yea this seems better to me, thanks!

Dustin L. Howett (DHowett) · 2020-12-15T23:39:10Z

+                        <Setter Property="Background" Value="{ThemeResource SystemAltMediumLowColor}" />
+                        <Setter Property="BorderBrush" Value="{ThemeResource SystemControlForegroundBaseMediumBrush}" />
+                    </Style>
+		    <Style x:Key="ParsedCommandLineTextBlockStyle" TargetType="TextBlock">


spacing looks strange around here.. Sup?

Dustin L. Howett (DHowett)

Just a few Qs. This is utterly lovely tho.

Dustin L. Howett (DHowett) · 2020-12-15T23:39:55Z

+                {
+                    ParsedCommandLineText(RS_(L"CommandPalette_FailedParsingCommandLine") + L"\n\t" + til::u8u16(_appArgs.GetExitMessage()));
+                }
+                _noMatchesText().Visibility(Visibility::Visible);


Curious -- we're showing the no matches string all the time now? What's this look like?

Dustin L. Howett (@DHowett) - I am afraid this might be a leftover.. 🤦 which is hidden by the ParsedCommandLineText... 🤔
Fixing it immediately, after I am finalizing the live search for initial review. Hopefully today (it is already 2am for me 😄)

Yep. It is a leftover. Thanks!

Don-Vito · 2020-12-16T01:42:07Z

Dustin L. Howett (@DHowett) - fixed the comments + introduced a vertical scroll bar for the parsed message window in the case the message is too long. Please review :)

Dustin L. Howett (DHowett)

Love it. Love it love it.

Deleted user (ghost) · 2020-12-16T01:52:01Z

Hello Dustin L. Howett (@DHowett)!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Dustin L. Howett (DHowett) · 2020-12-16T01:52:20Z

(Thanks for staying up late to work on our project 😄)

Deleted user (ghost) · 2021-01-28T18:05:46Z

🎉Windows Terminal Preview v1.6.10272.0 has been released which incorporates this pull request.:tada:

Handy links:

This commit introduces another optional text block in palette that will be shown in the command line mode (above the history). This text block will either contain a list of parsed command lines or a description why the parsing failed Closes microsoft#8344 Closes microsoft#7284

Don-Vito added 2 commits December 7, 2020 15:13

Introduce parsed command line text to command palette

7d6c61a

Introduce support for command line parsing failure message

e0739c2

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Dec 7, 2020

Don-Vito changed the title ~~8344 cmdpal command predict~~ Introduce parsed command line text to command palette Dec 7, 2020

Carlos Zamora (carlos-zamora) approved these changes Dec 7, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 7, 2020

Mike Griese (zadjii-msft) self-assigned this Dec 7, 2020

Fix typo in resource comment

dc33d61

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 7, 2020

Mike Griese (zadjii-msft) requested changes Dec 11, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 11, 2020

Mike Griese (zadjii-msft) removed their assignment Dec 11, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 11, 2020

Don-Vito added 2 commits December 12, 2020 00:12

Merge remote-tracking branch 'upstream/main' into 8344-cmdpal-command…

4ceeadf

…-predict

Introduce command line args as a member of CmdPal to reduce allocations

a39dc0d

Don-Vito requested review from Dustin L. Howett (DHowett), Carlos Zamora (carlos-zamora) and Mike Griese (zadjii-msft) December 11, 2020 23:14

Mike Griese (zadjii-msft) approved these changes Dec 15, 2020

View reviewed changes

Dustin L. Howett (DHowett) reviewed Dec 15, 2020

View reviewed changes

Dustin L. Howett (DHowett) requested changes Dec 15, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 15, 2020

Don-Vito added 3 commits December 16, 2020 03:23

Fix spacing in CmdPal xaml

39553a3

Cleanup leftover setting noMatchText visible in command line mode

75fffb3

Introduce vertical scroll for parsed command text

dc24592

Don-Vito requested a review from Dustin L. Howett (DHowett) December 16, 2020 01:42

ghost added the Area-CmdPal Command Palette issues and features label Dec 16, 2020

Dustin L. Howett (DHowett) approved these changes Dec 16, 2020

View reviewed changes

Dustin L. Howett (DHowett) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 16, 2020

ghost merged commit 22d43a4 into microsoft:main Dec 16, 2020

This was referenced Jan 28, 2021

Provide realtime commandline parsing feedback in the commandline mode of the command palette #7284

Closed

Preview what actions will be run in commandline mode of the CmdPal #8344

Closed

This pull request was closed.

Conversation

Don-Vito commented Dec 7, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Don-Vito commented Dec 7, 2020

Uh oh!

github-actions Bot commented Dec 7, 2020

New misspellings found, please review:

⚠️ Reviewers

Uh oh!

Carlos Zamora (carlos-zamora) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carlos Zamora (carlos-zamora) commented Dec 7, 2020

Uh oh!

Deleted user (ghost) commented Dec 7, 2020

Uh oh!

Don-Vito commented Dec 9, 2020

Uh oh!

Mike Griese (zadjii-msft) commented Dec 9, 2020

Uh oh!

Don-Vito commented Dec 9, 2020

Uh oh!

Dustin L. Howett (DHowett) commented Dec 9, 2020

Uh oh!

Don-Vito commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Don-Vito commented Dec 10, 2020

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dustin L. Howett (DHowett) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Don-Vito commented Dec 16, 2020

Uh oh!

Dustin L. Howett (DHowett) left a comment

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Dec 16, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Dustin L. Howett (DHowett) commented Dec 16, 2020

Uh oh!

Deleted user (ghost) commented Jan 28, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Don-Vito commented Dec 7, 2020 •

edited by DHowett

Loading

Don-Vito commented Dec 9, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.