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

Introduce startupActions in settings #8770

Merged
3 commits merged into from
Jan 15, 2021
Merged

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Jan 12, 2021

Procedural solution for #756.

Introduces a startupActions global setting.

This setting is as string with the same format as actions in command line arguments.
It is used only if command line arguments were not provided
(aka running pure wt.exe).

The setting allows implicit new-tabs.
In the case of invalid syntax we show the warning dialog and ignore the setting.

The documentation PR is here: MicrosoftDocs/terminal#217

@zadjii-msft zadjii-msft self-assigned this Jan 15, 2021
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.

Okay I don't see anything wrong with this. Maybe add more comments? This is how I'd do it, so I'm gonna give you the ✔️

@@ -1105,6 +1135,7 @@ namespace winrt::TerminalApp::implementation
const auto result = _appArgs.ParseArgs(args);
if (result == 0)
{
_hasCommandLineStartupActions = args.size() > 1;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should leave a comment that this is explicitly > 1, so that wt by itself will not count as having a startup commandline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zadjii-msft - I will document this for sure! Thanks!

Actually, I had a question regarding the decision I took here - the startupActions will be run only if no arguments were provided at all (I mean the launchMode will also prevent the startupActions to run).

It felt simpler to understand and implement, but not sure if it was the right choice.

@zadjii-msft zadjii-msft removed their assignment Jan 15, 2021
@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Needs-Second It's a PR that needs another sign-off labels Jan 15, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Awesome! Created #8802 to track adding this to the Settings UI. Since the Settings UI is new, @zadjii-msft and I decided that there shouldn't be an expectation to update it. But if you're interested in doing so, let us know!

@Don-Vito
Copy link
Contributor Author

Awesome! Created #8802 to track adding this to the Settings UI. Since the Settings UI is new, @zadjii-msft and I decided that there shouldn't be an expectation to update it. But if you're interested in doing so, let us know!

Absolutely, this will be a great opportunity to learn! Can you please assign #8802 to me? 😊 I guess it is probably better to solve it in a separate PR.

@carlos-zamora
Copy link
Member

Awesome! Created #8802 to track adding this to the Settings UI. Since the Settings UI is new, @zadjii-msft and I decided that there shouldn't be an expectation to update it. But if you're interested in doing so, let us know!

Absolutely, this will be a great opportunity to learn! Can you please assign #8802 to me? 😊 I guess it is probably better to solve it in a separate PR.

Sure thing! Shouldn't be too difficult (famous last words). May just include a bit more back-and-forth on the design (i.e. placement of the setting, word-smithing, etc.). Fair warning! hahaha. I appreciate it though 😊

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 15, 2021
@ghost
Copy link

ghost commented Jan 15, 2021

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.

@ghost ghost merged commit 3c044f2 into microsoft:main Jan 15, 2021
mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
Procedural solution for microsoft#756.

Introduces a `startupActions` global setting. 

This setting is as string with the same format as actions in command line arguments.
It is used only if command line arguments were not provided
(aka running pure wt.exe).

The setting allows implicit new-tabs.
In the case of invalid syntax we show the warning dialog and ignore the setting.

The documentation PR is here: MicrosoftDocs/terminal#217
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-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met 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.

3 participants