Skip to content

Teach command palette to fill in selected commandline upon right arrow - #11069

Merged
2 commits merged into
microsoft:mainfrom
Don-Vito:11049-cmdpal-commandline-right
Aug 30, 2021
Merged

Teach command palette to fill in selected commandline upon right arrow#11069
2 commits merged into
microsoft:mainfrom
Don-Vito:11049-cmdpal-commandline-right

Conversation

@Don-Vito

@Don-Vito Don-Vito commented Aug 27, 2021

Copy link
Copy Markdown
Contributor

Closes #11049

@ghost ghost added Area-CmdPal Command Palette issues and features Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 27, 2021
zadjii-msft
Mike Griese (zadjii-msft) approved these changes Aug 30, 2021
View reviewed changes

@zadjii-msft Mike Griese (zadjii-msft) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay checked out the branch - I love it. This is exactly what I wanted ☺️ Granted, I should make sure the rest of the team is on board with this UX pattern, but I love it

@zadjii-msft Mike Griese (zadjii-msft) mentioned this pull request Aug 30, 2021
Merged
@DHowett

Dustin L. Howett (DHowett) commented Aug 30, 2021

Copy link
Copy Markdown
Member

Can one-a-ya share a GIF? 😄

@zadjii-msft

Mike Griese (zadjii-msft) commented Aug 30, 2021

Copy link
Copy Markdown
Member

gh-11069

@zadjii-msft Mike Griese (zadjii-msft) added the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 30, 2021
@DHowett

Dustin L. Howett (DHowett) commented Aug 30, 2021

Copy link
Copy Markdown
Member

/me smashes that merge button

DHowett
Dustin L. Howett (DHowett) reviewed Aug 30, 2021
View reviewed changes
Comment thread src/cascadia/TerminalApp/CommandPalette.cpp
_searchBox().Select(_searchBox().Text().size(), 0);
_searchBox().Focus(FocusState::Programmatic);
_filteredActionsView().SelectedIndex(-1);
e.Handled(true);

@DHowett Dustin L. Howett (DHowett) Aug 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same question as Schuyler Rosefield (@Rosefield), actually -- this doesn't break left/right in the general case, right? For editing the command once you pop it in the box?

@Rosefield Schuyler Rosefield (Rosefield) Aug 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the change as is is fine since it only handles the right array key if you have something selected, and it deselects the option after you hit right arrow the first time.

@DHowett

Dustin L. Howett (DHowett) commented Aug 30, 2021

Copy link
Copy Markdown
Member

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 30, 2021
@ghost

Deleted user (ghost) commented Aug 30, 2021

Copy link
Copy Markdown

Hello Dustin L. Howett (@DHowett)!

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 won't merge this pull request until after the UTC date Mon, 30 Aug 2021 18:35:04 GMT, which is in 1 minute

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

@ghost
ghost merged commit 871b8de into microsoft:main Aug 30, 2021
@zadjii-msft Mike Griese (zadjii-msft) removed the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 30, 2021
ghost pushed a commit that referenced this pull request Aug 31, 2021
@zadjii-msft
Fix a crash when there aren't any recentCommands yet (#11082)
717ea85 
The first time you open commandline mode, `recentCommands` doesn't exist yet. However, we immediately try to read the `Size()` in a couple places. This'll A/V and we'll crash 😨 

The fix is easy - don't try and read the size of the non-existent `recentCommands`

Found this while playing with #11069
Regressed in #11030 
Didn't bother filing an issue for it when I have the fix in hand
@ghost

Deleted user (ghost) commented Oct 20, 2021

Copy link
Copy Markdown

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

Handy links:

@ghost ghost mentioned this pull request Oct 20, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DHowett Dustin L. Howett (DHowett) DHowett approved these changes
@zadjii-msft Mike Griese (zadjii-msft) zadjii-msft approved these changes
+1 more reviewer
@Rosefield Schuyler Rosefield (Rosefield) Rosefield left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-CmdPal Command Palette issues and features AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pressing the right arrow in the commandline mode of the cmdpal should fill in the selected command

4 participants

@Don-Vito @DHowett @zadjii-msft @Rosefield