Skip to content
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

Add --pos and --size cmdline args #13730

Merged

Conversation

ianjoneill
Copy link
Contributor

@ianjoneill ianjoneill commented Aug 12, 2022

Adds --pos, and --size commandline arguments to wt.

Closes #4620

@ghost ghost added Area-Commandline wt.exe's commandline arguments Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Aug 12, 2022
@ianjoneill
Copy link
Contributor Author

I have written some commandline parsing tests, but didn't push them as TAEF is erroring out for some reason when running the localTerminalApp test suite:

Summary of Errors Outside of Tests:
    Error: TAEF: [HRESULT: 0x80070005] A failure occurred while preparing to run tests in 'TerminalApp.LocalTests.dll'. (Failed to load "F:\windows-terminal\bin\x64\Debug\TestHostApp\TerminalApp.LocalTests.dll" or one of its dependencies.)

It's done this to me in the past - when I was back on Windows 10. Upgrading to Windows 11 fixed it for a bit, then it started erroring again.

@DHowett
Copy link
Member

DHowett commented Aug 12, 2022

I'm (vaguely) worried about these, even though we have an open feature request for them. They specify what happens to a new WT window, but they don't control windowing semantics. So, if your settings are such that all invocations launch in the same window, you'll never see the impact of these args unless you add -w -1 to force the creation of a new window.

In addition, should we consider something more "all-in-one" like xterm's -geometry (which takes an x11 geometry spec which looks like 100x300+30+10 or something?

AKA: It's three arguments, should it be one? Or maybe two? We perhaps incorrectly made it three different settings... but we don't need to repeat that mistake.

Thoughts? I know I'm just brain-dumping right now.

@ianjoneill
Copy link
Contributor Author

ianjoneill commented Aug 12, 2022

What you are suggesting certainly sounds more powerful. Especially if you allow it to change the location of an existing window. You could potentially open yourself up to malicious programs arbitrarily changing the position of your terminal, but I would imagine that's a low risk - it sounds more annoying than anything else.

Unless I'm reading it wrong, X's geometry spec seems a bit rigid compared to what terminal currently allows - i.e. you have to provide the complete specification or nothing. Maybe two arguments would be better - a position and a size? e.g. --position ,200 --size 10, would give you something that's 200 pixels from the top of the screen and 10 columns wide.

@serd2011
Copy link
Contributor

I don't think that changing location of an existing window is natural. Are there any applications where this is implemented?
And I think it's reasonable to expect user to add -w -1 if he set a setting to launch all invocations in the same window.

@WSLUser
Copy link
Contributor

WSLUser commented Aug 15, 2022

Adding initialRows and initialCols as a single argument makes more sense to me when used this way. You're typically changing both anyways and if you only enter one of them, the other can just remain default. And now that we have semicolons working better, that can be used as the delimiter to determine which is which (rows first; cols) and yes still need -w -1 when manipulating the existing window. All these window positioning arguments can easily be scripted together with the other arguments to get the perfect custom setup so it's not a huge deal.

@ianjoneill
Copy link
Contributor Author

@DHowett it's been a couple of weeks since I opened this - have you/the team had any thoughts on what you'd like the command line args to be?

@lhecker lhecker added the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 9, 2022
@zadjii-msft
Copy link
Member

quick 5pm note:

  • --initialPosition x,y: needs both, makes sense
  • --size c,r: needs both, accepts size in chars
    • (in the future, we can add --size {w}px,{h}px, where a literal px would disambiguate between px and chars. And it could be --size {w}px,r

@DHowett
Copy link
Member

DHowett commented Sep 12, 2022

  • --initialPosition x,y: needs both, makes sense

I'd go further and call it --pos; thoughts Mike?

Sorry for the delay @ianjoneill!

@zadjii-msft
Copy link
Member

I'd go further and call it --pos; thoughts Mike?

sure

@ianjoneill ianjoneill changed the title Add initialPosition, initialRows and initialCols cmdline args Add --pos, and --size cmdline args Sep 19, 2022
@ianjoneill
Copy link
Contributor Author

I've updated the cmdline args as per your suggestions.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

This is simple and I'm cool with it. I don't love our ProposedSize implementation, but that's not on you at all. Sheesh, getting data from one end of the app to the other is a bear, innit?

Thanks for doing this & putting up with the delay!

@DHowett DHowett added Needs-Second It's a PR that needs another sign-off and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Sep 27, 2022
@ianjoneill
Copy link
Contributor Author

We're now in the slightly weird situation where this is documented, but not merged!

https://learn.microsoft.com/en-us/windows/terminal/command-line-arguments?tabs=windows

@DHowett
Copy link
Member

DHowett commented Oct 7, 2022

paging @PankajBhojwani into the OR

@DHowett DHowett changed the title Add --pos, and --size cmdline args Add --pos and --size cmdline args Oct 7, 2022
@carlos-zamora
Copy link
Member

carlos-zamora commented Oct 7, 2022

Since this has two approvals, merging.

@msftbot merge this in 5 minutes

EDIT: ok, I'll just add the automerge label then.

EDIT2: huh, the bot acknowledged the automerge label then didn't merge it!

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 7, 2022
@ghost
Copy link

ghost commented Oct 7, 2022

Hello @carlos-zamora!

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.

@carlos-zamora carlos-zamora merged commit 43dbbd5 into microsoft:main Oct 7, 2022
ghost pushed a commit that referenced this pull request Oct 12, 2022
Since #13730 merged, when we parse LaunchPosition we treat the
coordinates as `int32_t`. This PR updates the actual `LaunchPosition`
struct to also use `int32_t` for consistency.

## Validation Steps Performed
Terminal still builds and runs
@ghost
Copy link

ghost commented Jan 24, 2023

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

Handy links:

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-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. 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.

initialPosition (and others) should be available as commandline args
8 participants