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 a --elevate flag #12142

Closed
wants to merge 6 commits into from
Closed

Add a --elevate flag #12142

wants to merge 6 commits into from

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Jan 11, 2022

To match the arg in #12137.

--elevate will manually elevate the profile.

--no-elevate will override a elevate: true in the profile with false.

So now you can wt --elevate cmd and it'll do what you think

  • I work here
  • Tests added
  • need to update docs

    ```
    git diff dev/migrie/f/non-terminal-content-elevation-warning dev/migrie/f/632-on-warning-dialog > ..\632.patch
    git apply ..\632.patch --ignore-whitespace --reject
    ```
  To match the arg in #12137.

  `--elevate` will manually elevate the profile.

  `--no-elevate` will override a `elevate: true` in the profile with false.

  So now you can `wt --elevate cmd` and it'll do what you think

  * [x] I work here
  * [x] Tests added
  * [ ] need to update docs
@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-Commandline wt.exe's commandline arguments labels Jan 11, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.13 milestone Jan 11, 2022
@zadjii-msft zadjii-msft changed the base branch from dev/migrie/f/632-attempt-4 to main January 12, 2022 11:22
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Jan 12, 2022
@carlos-zamora
Copy link
Member

@zadjii-msft Curious, what happens when somebody puts --elevate and --no-elevate together?

@zadjii-msft
Copy link
Member Author

dont merge this quite yet

@zadjii-msft zadjii-msft marked this pull request as draft January 12, 2022 18:43
@zadjii-msft
Copy link
Member Author

void AppHost::_HandleCommandlineArgs()
{
    std::vector<winrt::hstring> args;
    _buildArgsFromCommandline(args);
    std::wstring cwd{ wil::GetCurrentDirectoryW<std::wstring>() };

    Remoting::CommandlineArgs eventArgs{ { args }, { cwd } };
>    _windowManager.ProposeCommandline(eventArgs);

    _shouldCreateWindow = _windowManager.ShouldCreateWindow();
    if (!_shouldCreateWindow)

If the Terminal isn't already running, and you have a saved layout, then this line right here crashes with a RPC_S_CALL_FAILED. Presumably the window manager is never set up, or something weird. I'll look into it.

@zadjii-msft zadjii-msft self-assigned this Jan 12, 2022
@zadjii-msft zadjii-msft removed the Needs-Second It's a PR that needs another sign-off label Jan 12, 2022
@zadjii-msft
Copy link
Member Author

Okay before I lose it

  • wt.exe A spawns, with --elevate
  • It determines that it should create a window, because it had a new-tab ation
  • It's the monarch, so it finds that it needs to spawn other processes to handle the PersistedWindowLayouts
  • wt B spawns
  • wt A spawns elevate-shim to elevate its profile
  • at this point, wt A dies. It didn't have any tabs, so it immediately exits.
  • wt B asks the WindowManager, who in turn asks the Monarch, if it should create a window. Problem is, A is still closing, or closed right in time for B to not become the monarch. So the _windowManager.ProposeCommandline call explodes (in the restored peasant)

@zadjii-msft
Copy link
Member Author

Ho boy, this just got interesting. Consider:

wtd --elevate ; --elevate

you'd assume that would spawn two tabs elevated (in the same window), even with glomming turned off. That's not what would happen. We'd spawn one elevate-shim per newTab action, resulting in two elevated windows.

May have to reconsider this entirely. Might have to become a root-level parameter, to affect the entire commandline. E.g. "run this commandline in and elevated wt"

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.

I'll block you just in case

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 12, 2022
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 12, 2022
@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Jan 13, 2022
@zadjii-msft
Copy link
Member Author

Ah butts okay. I definitely need to turn this into a top-level flag, not a new-tab level one. Plus, there's some other bugs that needs bits and pieces from this WIP branch, regardless of this feature getting added.

I'm going to just close this for now, and file proper issues for both these to keep me honest.

@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Jan 25, 2022
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 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
Development

Successfully merging this pull request may close these issues.

4 participants