Skip to content

Add a move-focus subcommand - #8546

Merged
15 commits merged into
mainfrom
dev/migrie/f/6580-maybe-move-focus
Jan 11, 2021
Merged

15 commits merged into
mainfrom
dev/migrie/f/6580-maybe-move-focus

Conversation

@zadjii-msft

@zadjii-msft Mike Griese (zadjii-msft) commented Dec 10, 2020 •

Copy link
Copy Markdown
Member

Summary of the Pull Request

Adds support for the move-focus subcommand to wt.exe. This subcommand works exactly like moveFocus(up|down|left|right).

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Bear with me, I wrote this before paternity leave, so code might be a bit stale.

Oddly, after startup, this does not leave the focus on the pane you moved to. If you move-focus during startup, at the end of startup, we'll still focus a random pane. This is because the terminal still auto-focus a TermControl when it's done with layout. While we'll maintain the active control just fine during the startup, at the end of startup, all the controls will complete layout in a random order.

This is no different than the startup right now. wt sp ; sp ; sp will focus a random pane at the end. This is left for a future someone to fix

This is also subject to #2398 / #4692. Moving in a direction isn't totally reliable currently. focus-pane -t ID will certainly be more reliable, but this will work in the meantime?

Validation Steps Performed

Opened probably 100 terminals, confirmed that the layout was always correct. Final focused pane was random, but the layout was right.

@ghost ghost added Area-Commandline wt.exe's commandline arguments Issue-Question For questions or discussion Product-Terminal The new Windows Terminal. labels Dec 10, 2020
# Conflicts:
#	src/cascadia/TerminalApp/AppCommandlineArgs.h
#	src/cascadia/TerminalApp/Resources/en-US/Resources.resw

@Don-Vito Don-Vito left a comment •

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.

Besides the question inline - shouldn't we also update _resetStateToDefault

void AppCommandlineArgs::_buildMoveFocusParser()
{
_moveFocusCommand = _app.add_subcommand("move-focus", RS_A(L"CmdMoveFocusDesc"));
_moveFocusShort = _app.add_subcommand("mf", RS_A(L"CmdMFDesc"));

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.

Mike Griese (@zadjii-msft) - dumb question.. let's say we introduce "m" for minimized, and then I write -mf. What will the command line do: minimized focus or move focus? 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Presumably, wt -mf sp ; mf left will still work, because the --minimizeFocus arg will be an option, not a subcommand. I'd think that CLI11 should be able to differentiate between the -mf and mf args. If not, that would definitely be an upstream bug 😄

@zadjii-msft

Copy link
Copy Markdown
Member Author

shouldn't we also update _resetStateToDefault

Already done, on L532 😄

@Don-Vito

Copy link
Copy Markdown
Contributor

shouldn't we also update _resetStateToDefault

Already done, on L532 😄

Yeah. Weird - I checked few times, as I was surprised you will forget after doing so few days ago for focus tab. In any case it is there and looks lovely.

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
@zadjii-msft Mike Griese (zadjii-msft) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 11, 2021
@ghost

Copy link
Copy Markdown

Hello Mike Griese (@zadjii-msft)!

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.

@ghost
ghost merged commit 7235996 into main Jan 11, 2021
@ghost
ghost deleted the dev/migrie/f/6580-maybe-move-focus branch January 11, 2021 18:37
@ghost

Copy link
Copy Markdown

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

Handy links:

MPela (mpela81) pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
## Summary of the Pull Request

Adds support for the `move-focus` subcommand to `wt.exe`. This subcommand works _exactly_ like `moveFocus(up|down|left|right)`. 

## References
* Will surely conflict with microsoft#8183
* Is goodness even in the world where microsoft#5464 exists

## PR Checklist
* [x] Closes microsoft#6580 
* [x] I work here
* [x] Tests added/passed
* [x] Docs PR: MicrosoftDocs/terminal#209

## Detailed Description of the Pull Request / Additional comments

Bear with me, I wrote this before paternity leave, so code might be a bit stale.

Oddly, after startup, this _does not_ leave the focus on the pane you moved to. If you `move-focus` during startup, at the end of startup, we'll still focus a _random_ pane. This is because the terminal still auto-focus a TermControl when it's done with layout. While we'll maintain the active control just fine during the startup, at the end of startup, all the controls will complete layout in a random order. 

This is no different than the startup right now. `wt sp ; sp ; sp` will focus a random pane at the end. This is left for a future someone to fix

This is also subject to microsoft#2398 / microsoft#4692. Moving in a direction isn't _totally_ reliable currently. `focus-pane -t ID` will certainly be more reliable, but this will work in the meantime?

## Validation Steps Performed

Opened probably 100 terminals, confirmed that the layout was always correct. Final focused pane was random, but the layout was right.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Commandline wt.exe's commandline arguments AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Question For questions or discussion Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maybe add support for move-focus subcommand

4 participants