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

When there's no profile or title, invent a title from the commandline #10998

Merged
1 commit merged into from
Aug 23, 2021

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Aug 20, 2021

This supports a future world where we give commandline-only invocations
their own tabs. It was easier to promote the commandline to a title at
the time of argument parsing, rather than later, but I am happy to
change this if anyone disagrees.

This supports a future world where we give commandline-only invocations
their own tabs. It was easier to promote the commandline to a title at
the time of argument parsing, rather than later, but I am happy to
change this if anyone disagrees.
@zadjii-msft
Copy link
Member

This might be #6776

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Honestly, the implementation is fine, I'm only blocking because maybe we want to save this for 1.12 with the other "breaking changes"

{
// If there's no profile, but there IS a command line, set the tab title to the first part of the command
// This will ensure that the tab we spawn has a name (since it didn't get one from its profile!)
args.TabTitle(winrt::to_hstring(til::at(_commandline, 0)));
Copy link
Member

Choose a reason for hiding this comment

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

one weird thing - this does cause the cmdpal to display the title like:

image

which seemed weird but also ¯\_(ツ)_/¯

Copy link
Member Author

Choose a reason for hiding this comment

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

Somewhat, but... it also suggests that you're allowed to change it (which is a somewhat nice side effect)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 23, 2021
@DHowett
Copy link
Member Author

DHowett commented Aug 23, 2021

I don't totally disagree, but 1.11 is our last preview release where we can test changes that we might want to promote to 1.10 before October. Since Def Term profile selection is built on making these changes, I'd be worried about holding them back any longer than we already have.

That includes the upcoming change to switch the profile here from defaultProfile to Base. We could go without this one, but it would make that switch weird.

Putting all our breaking changes into one release is a bit compelling, admittedly.

What do you think?

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 23, 2021
@zadjii-msft
Copy link
Member

That includes the upcoming change to switch the profile here from defaultProfile to Base. We could go without this one, but it would make that switch weird.

I didn't think we were doing that for 1.11. If we are, then we better get on it 😛

@DHowett
Copy link
Member Author

DHowett commented Aug 23, 2021

we better get on it 😛

That's the PR queued up to go up after the three active ones merge 😉

@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Aug 23, 2021
@DHowett
Copy link
Member Author

DHowett commented Aug 23, 2021

Thank you. 😄

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 23, 2021
@ghost
Copy link

ghost commented Aug 23, 2021

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.

@ghost ghost merged commit f681d3a into main Aug 23, 2021
@ghost ghost deleted the dev/duhowett/invent-title branch August 23, 2021 17:01
ghost pushed a commit that referenced this pull request Aug 24, 2021
It was insufficient to only promote commandline components to titles
during commandline parsing, because we also have a whole complement of
actions that contain NewTerminalArgs. The tests caught me out a little
too late (sorry!). I decided it was better move promotion down to
TerminalSettings.

Fixes #6776
Re-implements #10998
@ghost
Copy link

ghost commented Aug 31, 2021

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants