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

This looks great! Thanks!

carlos-zamora · 2020-12-07T17:52:59Z

src/cascadia/TerminalApp/CommandPalette.cpp

+                    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?

@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 · 2020-12-07T17:56:23Z

@msftbot make sure @zadjii-msft signs off on this

ghost · 2020-12-07T17:56:31Z

Hello @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 @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

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

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 😊

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.

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

zadjii-msft · 2020-12-11T14:09:51Z

src/cascadia/TerminalApp/CommandPalette.cpp

+            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

@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.

zadjii-msft · 2020-12-11T14:13:49Z

src/cascadia/TerminalApp/CommandPalette.cpp

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


@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)

zadjii-msft · 2020-12-11T14:15:52Z

src/cascadia/TerminalApp/CommandPalette.cpp

+                }
+                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

zadjii-msft

Yea this seems better to me, thanks!

DHowett · 2020-12-15T23:39:10Z

src/cascadia/TerminalApp/CommandPalette.xaml

+                        <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?

DHowett

Just a few Qs. This is utterly lovely tho.

DHowett · 2020-12-15T23:39:55Z

src/cascadia/TerminalApp/CommandPalette.cpp

+                {
+                    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?

@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

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

DHowett

Love it. Love it love it.

ghost · 2020-12-16T01:52:01Z

Hello @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.

DHowett · 2020-12-16T01:52:20Z

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

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 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

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

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

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 DHowett, carlos-zamora and zadjii-msft December 11, 2020 23:14

zadjii-msft approved these changes Dec 15, 2020

View reviewed changes

DHowett reviewed Dec 15, 2020

View reviewed changes

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 DHowett December 16, 2020 01:42

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

DHowett approved these changes Dec 16, 2020

View reviewed changes

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 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 commented Dec 7, 2020

Uh oh!

ghost commented Dec 7, 2020

Uh oh!

Don-Vito commented Dec 9, 2020

Uh oh!

zadjii-msft commented Dec 9, 2020

Uh oh!

Don-Vito commented Dec 9, 2020

Uh oh!

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!

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!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

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!

DHowett commented Dec 16, 2020

Uh oh!

ghost commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

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.