Add startup task, setting to launch application on login

[jelster]

Summary of the Pull Request

This PR adds a new boolean global setting, startOnUserLogin, along with associated AppLogic to request enabling or disabling of the StartupTask. Added UAP5 extensions to AppX manifests.

References

#2189

PR Checklist

• Closes Feature Request: Start teminal when user log-in #2189
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Feature Request: Start teminal when user log-in #2189

Detailed Description of the Pull Request / Additional comments

Please note, I'm a non-practicing C++ developer, there are a number of things I wasn't sure how to handle in the appropriate fashion, mostly around error handling and what probably looks like an incredibly naive (and messy) way to implement the async co_await behavior.

Error handling-wise, I found (don't ask me how!) that if you somehow mismatch the startup task's ID between the manifest and the call to StartupTask::GetAsync(hstring taskId), you'll get a very opaque WinRT exception that boils down to a generic invalid argument message. This isn't likely to happen in the wild, but worth mentioning...

I had enough trouble getting myself familiarized with the project, environment, and C++/WinRT in general didn't want to try to tackle adding tests for this quite yet since (as I mentioned) I don't really know what I'm doing. I'm happy to give it a try with perhaps a bit of assistance in getting started 😃

Further work in this area of the application outside of this immediate PR might need to include adding an additional setting to contain launch args that the startup task can pass to the app so that users can specify a non-default profile to launch on start, window position (e.g., #653).

Validation Steps Performed

✔️ Default settings:
Given the user does not have the startOnUserLogin setting in their profile.json,
When the default settings are opened (via alt+click on Settings),
Then the global settings should contain the "startOnUserLogin": false token

✔️ Applying setting on application launch
Given the startOnUserLogin is true and
the Windows Terminal startup task is disabled and
the application is not running
When the application is launched
Then the Windows Terminal entry in the user's Startup list should be enabled

✔️ Applying setting on settings change
Given the startOnUserLogin is true and
the Windows Terminal startup task is enabled and
the application is running
When the startOnUserLogin setting is changed to false and
the settings file is saved to disk
Then the Windows Terminal startup task entry should be disabled

✔️ Setting is ignored when user has manually disabled startup
Given the startOnUserLogin is true and
the application is not running and
the Windows Terminal startup task has been set to disabled via user action
When the application is launched
Then the startup task should remain disabled and
the application should not throw an exception

note: Task Manager does not seem to re-scan startup task states after launch; the Settings -> Apps -> Startup page also requires closing or moving away to refresh the status of entries

[DHowett-MSFT]

holy wow alright this'll take some time to digest. Thank you

[zadjii-msft]

holy crap you actually did it

I'm gonna throw this out here right now - the team is absolutely stoked that you were able to throw together a PR for this feature in just three days. That's really impressive.

I'm going to be the pre-emptive bearer of badish news - we're starting to make harder cuts about what is and isn't making it into the 1.0 release of the Terminal. As awesome as this feature is, I'm going to go with my gut here and guess that this isn't going to make that cut unfortunately. That being said, we're still going to review this now and get feedback to you. Once this gets approved, we'll probably just hold in until we make an official 1.0 release branch, and then we'll get it merged immediately after that.

[jelster]

holy wow alright this'll take some time to digest.

The excellent work you folks have put into the docs is the only thing that made it feasible for me to undertake. 👍 I'm happy to make any changes suggested - I know there's a lot that can be improved upon.

we're starting to make harder cuts about what is and isn't making it into the 1.0 release of the Terminal...this isn't going to make that cut unfortunately.

Honestly, I haven't been paying attention to the release schedule you guys have been so transparent about communicating, so only mildly disappointing 😆 - I'm happy you're all not laughing yourselves to the ground after looking at the products of my flailing around in VS! 😅

Joking aside, if it doesn't make 1.0, that'll just mean there's more time to add some more polish and refinement :)

Thanks for the consideration 😄

[jelster]

Question for @zadjii-msft -- do you typically prefer to keep PR's updated from the master branch periodically, once prior to review, not at all, something else? Wanted to ask before I pushed up a merge commit that brings in latest from master

[zadjii-msft]

In my experience, it's easier for me to do merges if I keep pulling master at regular intervals. Usually that keeps the merge conflicts as manageable as possible.

[jelster]

Looks like the unit test run is responsible for the timeout - ran for 46min, but seemed to be reporting results up to the point of cancellation.

Is this anything I can remedy, or is it OK to ignore it for now?

[DHowett-MSFT]

The x86 tests can be flaky. I'll re-schedule this one. 😄

[jelster]

@DHowett-MSFT -- I had no errors building locally with Invoke-OpenConsoleBuild /p:Configuration=Release /p:Platform=x64, and I can't see anything obvious in the diffs that point to a specific change that could cause the build failure:

\Microsoft.Windows.CppWinRT.2.0.200316.3\build\native\Microsoft.Windows.CppWinRT.targets(164,13): Error MSB4006: There is a circular dependency in the target dependency graph involving target "ComputeGetResolvedWinMD"

Is there anything you can see that I might be able to do to resolve this, or is it another one of those "flaky build" types of things?

[zadjii-msft]

@jelster That's unfortunately one of those "flaky build" issues. At least with the unit test failure, we know that's our fault, but we're still trying to track down whatever the heck is causing this error.

For the future us, the failing build was https://dev.azure.com/ms/Terminal/_build/results?buildId=74825&view=logs&jobId=fe5d217b-357f-5a83-11fa-ea15ba02c406&j=fe5d217b-357f-5a83-11fa-ea15ba02c406&t=1dea19ec-49ff-524b-2675-ea6f89a90475

I'll kick it again, and it'll probably just fix itself.

[jelster]

Hi @zadjii-msft -- congrats to you and the team for getting v1.0 released! 🥳 🎈

I know you folks have all been really busy, but I wanted to give a bit of a nudge to see if we can move this PR along a bit. What needs to happen for the review, and is there anything I might do to facilitate?

[carlos-zamora]

You now need to update the Preview appxmanifest too.

As for _ApplyStartupTaskStateChange(), I'm not too familiar with StartupTask. But if you leave the other StartupTaskState's empty, the app just doesn't launch on startup right? Won't cause any weird problems other than just ignoring your json setting when you try to set it?

[jelster]

But if you leave the other StartupTaskState's empty, the app just doesn't launch on startup right? Won't cause any weird problems other than just ignoring your json setting when you try to set it?

Fortunately, this is something I made sure to specifically check in my testing 😄. If the setting's value is blank or empty, the application will start just fine - it will just use the default of false as the materialized value (via LayerJson).

Similarly, if the user has disabled the startup task via the Settings App or Task Manager, setting the config value to true will have no visible effect . I agree, a separate issue makes sense to track the UX for this scenario

[carlos-zamora]

Just update the comment and use the GETSET thing then we're good to go. This is awesome! Thanks!

[zadjii-msft]

Okay this is beautiful, but I'm mostly blocking for the merge shrapnel and the co_await thing. Great work here!

[zadjii-msft]

You'll probably need to get a weak ref to this before the co_await, and make sure you can get a strong ref afterwards to make sure that this is still alive.

We do a lot of that in TerminalPage like so:

        auto weakThis{ get_weak() };
        co_await winrt::resume_foreground(Dispatcher());
        if (auto page{ weakThis.get() })
        {
            // Do something with this
        }

[jelster]

I'm happy to put that change in. Why is there the need to get the weak this ref? The other fire_and_forget routines in the same class don't follow that same pattern -- does it make sense to create an issue to refactor those to grab the weak refs as well?

ex in AppLogic:

    fire_and_forget AppLogic::_RefreshThemeRoutine()
    {
        co_await winrt::resume_foreground(_root->Dispatcher());

        // Refresh the UI theme
        _ApplyTheme(_settings->GlobalSettings().GetTheme());
    }

[zadjii-msft]

This particular one we're less worried about. AppLogic controls the lifetime of the entire application, so it's actually never going away, and we don't need to really worry about it. Though, you're right that it wouldn't hurt to do the right thing there as well.

[jelster]

Created #6295 to track it

[jelster]

Thanks for the feedback @zadjii-msft! I didn't have any issues with the requested changes, but I do want to understand the weak ref pattern you demonstrated a bit better. Is the capture of the weak ref just being explicit about what's happening, whereas the previous code does this implicitly?

[zadjii-msft]

Sorta, yea. Basically, the pattern above is used to say "I'm not sure that this will still be valid on the other side of the co_await. I'm going to get a weak_ref before the co_await that won't keep this alive. When this is method is resumed, I'll check that the weak_ref is still pointing to something valid. If it is, then we know that we can use this safely".

Otherwise, there's no good way of checking the value of this to make sure that it's a valid object on the other side of the co_await. The object might have been released, because this isn't an owning pointer.

It's a bit of a weird pattern that I'm still getting used to myself, so lemme know if that's not totally clear. There were a couple PRs a little while back that went through all of TerminalPage and tried to update it as best we could, but I'm not sure we got every case.

[zadjii-msft]

Thanks for putting this all together, and all your patience. This looks good to me 😄

[jelster]

Sorta, yea. Basically, the pattern above is used to say "I'm not sure that this will still be valid on the other side of the co_await. I'm going to get a weak_ref before the co_await that won't keep this alive. When this is method is resumed, I'll check that the weak_ref is still pointing to something valid. If it is, then we know that we can use this safely".

Otherwise, there's no good way of checking the value of this to make sure that it's a valid object on the other side of the co_await. The object might have been released, because this isn't an owning pointer.

It's a bit of a weird pattern that I'm still getting used to myself, so lemme know if that's not totally clear. There were a couple PRs a little while back that went through all of TerminalPage and tried to update it as best we could, but I'm not sure we got every case.

This makes complete sense. It also makes sense why it's not 100% needed for the AppLogic members, but at least by doing it the "right" way here the application will be just that much more robust 😃

Thanks for taking the time to review this PR as well as for providing the great explanations and feedback! I'm happy to have had the opportunity to contribute to something I use and ❤️ every day. The team is doing a lot of really exciting and great things - kudos and 🍻!

[ghost]

Hello @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]

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

Handy links:

• Release Notes
• Store Download