From a46fac25d3264fdb29dd2b70ec33f3b5e4f3540f Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Thu, 20 Feb 2025 16:53:33 -0600 Subject: [PATCH] Remove `startOnUserLogin` from the settings; use OS APIs only (#18530) Before we had a Settings UI, we added support for a setting called `startOnUserLogin`. It was a boolean, and on startup we would try to yeet the value of that setting into the Windows API responsible for registering us as a startup task. Unfortunately, we failed to take into account a few things. - Startup tasks can be independently controlled by the user in Windows Settings or by an enterprise using enterprise policy - This control is not limited to *disabling* the task; it also supports enabling it! Users could enable our startup task outside the settings file and we would never know it. We would load up, see that `startOnUserLogin` was `false`, and go disable the task again. :facepalm: Conversely, if the user disables our task outside the app _we can never enable it from inside the app._ If an enterprise has configured it either direction, we can't change it either. The best way forward is to remove it from our settings model and only ever interact with the Windows API. This pull request replaces `startOnUserLogin` with a rich settings experience that will reflect the current and final state of the task as configured through Windows. Terminal will enable it if it can and display a message if it can't. My first attempt at this PR (which you can read in the commit history) made us try harder to sync the state between the settings model and the OS; we would propagate the disabled state back to the user setting when the task was disabled in the OS or if we failed to enable it when the user asked for it. That was fragile and didn't support reporting the state in the settings UI, and it seems like it would be confusing for a setting to silently turn itself back off anyway... Closes #12564 --- doc/cascadia/profiles.schema.json | 5 - src/cascadia/TerminalApp/AppLogic.cpp | 38 -------- src/cascadia/TerminalApp/AppLogic.h | 1 - .../TerminalSettingsEditor/Launch.cpp | 3 + .../TerminalSettingsEditor/Launch.xaml | 7 +- .../LaunchViewModel.cpp | 84 +++++++++++++++++ .../TerminalSettingsEditor/LaunchViewModel.h | 11 ++- .../LaunchViewModel.idl | 8 +- .../Resources/en-US/Resources.resw | 8 ++ .../SettingContainer.cpp | 93 +++++++++++-------- .../TerminalSettingsEditor/SettingContainer.h | 2 + .../SettingContainerStyle.xaml | 1 + .../GlobalAppSettings.idl | 1 - .../TerminalSettingsModel/MTSMSettings.h | 1 - .../TerminalSettingsModel/defaults.json | 1 - .../SerializationTests.cpp | 1 - 16 files changed, 170 insertions(+), 95 deletions(-) diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index 315d103c53..8726d8303c 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -2480,11 +2480,6 @@ "minimum": 1, "type": "integer" }, - "startOnUserLogin": { - "default": false, - "description": "When set to true, this enables the launch of Terminal at startup. Setting this to false will disable the startup task entry. If the Terminal startup task entry is disabled either by org policy or by user action this setting will have no effect.", - "type": "boolean" - }, "firstWindowPreference": { "default": "defaultProfile", "description": "Defines what behavior the terminal takes when it starts. \"defaultProfile\" will have the terminal launch with one tab of the default profile, and \"persistedWindowLayout\" will cause the terminal to save its layout on close and reload it on open.", diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index c1c56ca9af..a022be1e6a 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -81,8 +81,6 @@ static winrt::hstring _GetErrorText(SettingsLoadErrors error) return _GetMessageText(static_cast(error), settingsLoadErrorsLabels); } -static constexpr std::wstring_view StartupTaskName = L"StartTerminalOnLoginTask"; - namespace winrt::TerminalApp::implementation { // Function Description: @@ -353,40 +351,6 @@ namespace winrt::TerminalApp::implementation } CATCH_LOG() - safe_void_coroutine AppLogic::_ApplyStartupTaskStateChange() - try - { - // First, make sure we're running in a packaged context. This method - // won't work, and will crash mysteriously if we're running unpackaged. - if (!IsPackaged()) - { - co_return; - } - - const auto tryEnableStartupTask = _settings.GlobalSettings().StartOnUserLogin(); - const auto task = co_await StartupTask::GetAsync(StartupTaskName); - - switch (task.State()) - { - case StartupTaskState::Disabled: - if (tryEnableStartupTask) - { - co_await task.RequestEnableAsync(); - } - break; - case StartupTaskState::DisabledByUser: - // TODO: GH#6254: define UX for other StartupTaskStates - break; - case StartupTaskState::Enabled: - if (!tryEnableStartupTask) - { - task.Disable(); - } - break; - } - } - CATCH_LOG(); - // Method Description: // - Reloads the settings from the settings.json file. // - When this is called the first time, this initializes our settings. See @@ -446,7 +410,6 @@ namespace winrt::TerminalApp::implementation // TerminalSettings object. _ApplyLanguageSettingChange(); - _ApplyStartupTaskStateChange(); _ProcessLazySettingsChanges(); auto warnings{ winrt::multi_threaded_vector() }; @@ -473,7 +436,6 @@ namespace winrt::TerminalApp::implementation // Both LoadSettings and ReloadSettings are supposed to call this function, // but LoadSettings skips it, so that the UI starts up faster. // Now that the UI is present we can do them with a less significant UX impact. - _ApplyStartupTaskStateChange(); _ProcessLazySettingsChanges(); FILETIME creationTime, exitTime, kernelTime, userTime, now; diff --git a/src/cascadia/TerminalApp/AppLogic.h b/src/cascadia/TerminalApp/AppLogic.h index 8960a476f2..0b76939992 100644 --- a/src/cascadia/TerminalApp/AppLogic.h +++ b/src/cascadia/TerminalApp/AppLogic.h @@ -76,7 +76,6 @@ namespace winrt::TerminalApp::implementation TerminalApp::ContentManager _contentManager{ winrt::make() }; void _ApplyLanguageSettingChange() noexcept; - safe_void_coroutine _ApplyStartupTaskStateChange(); [[nodiscard]] HRESULT _TryLoadSettings() noexcept; void _ProcessLazySettingsChanges(); diff --git a/src/cascadia/TerminalSettingsEditor/Launch.cpp b/src/cascadia/TerminalSettingsEditor/Launch.cpp index 15ed5bee12..286ed7c814 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.cpp +++ b/src/cascadia/TerminalSettingsEditor/Launch.cpp @@ -5,6 +5,7 @@ #include "Launch.h" #include "Launch.g.cpp" #include "EnumEntry.h" +#include "LaunchViewModel.h" #include @@ -40,5 +41,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void Launch::OnNavigatedTo(const NavigationEventArgs& e) { _ViewModel = e.Parameter().as(); + auto innerViewModel{ winrt::get_self(_ViewModel) }; + /* coroutine dispatch */ innerViewModel->PrepareStartOnUserLoginSettings(); } } diff --git a/src/cascadia/TerminalSettingsEditor/Launch.xaml b/src/cascadia/TerminalSettingsEditor/Launch.xaml index 67e79ba43e..8aeca2f92b 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.xaml +++ b/src/cascadia/TerminalSettingsEditor/Launch.xaml @@ -163,8 +163,11 @@ - - + diff --git a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp index 7b37863ef3..440f43211a 100644 --- a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp @@ -14,6 +14,8 @@ using namespace winrt::Windows::Foundation; using namespace winrt::Microsoft::Terminal::Settings::Model; using namespace winrt::Windows::UI::Xaml::Data; +static constexpr std::wstring_view StartupTaskName = L"StartTerminalOnLoginTask"; + namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { // For ComboBox an empty SelectedItem string denotes no selection. @@ -365,4 +367,86 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { return _Settings.DefaultTerminals(); } + + bool LaunchViewModel::StartOnUserLoginAvailable() + { + return IsPackaged(); + } + + safe_void_coroutine LaunchViewModel::PrepareStartOnUserLoginSettings() + { + if (!StartOnUserLoginAvailable()) + { + co_return; + } + + auto strongThis{ get_strong() }; + auto task{ co_await winrt::Windows::ApplicationModel::StartupTask::GetAsync(StartupTaskName) }; + _startOnUserLoginTask = std::move(task); + _NotifyChanges(L"StartOnUserLoginConfigurable", L"StartOnUserLoginStatefulHelpText", L"StartOnUserLogin"); + } + + bool LaunchViewModel::StartOnUserLoginConfigurable() + { + if (!_startOnUserLoginTask) + { + return false; + } + namespace WAM = winrt::Windows::ApplicationModel; + const auto state{ _startOnUserLoginTask.State() }; + // Terminal cannot change the state of the login task if it is any of the "ByUser" or "ByPolicy" states. + return state == WAM::StartupTaskState::Disabled || state == WAM::StartupTaskState::Enabled; + } + + winrt::hstring LaunchViewModel::StartOnUserLoginStatefulHelpText() + { + if (_startOnUserLoginTask) + { + namespace WAM = winrt::Windows::ApplicationModel; + switch (_startOnUserLoginTask.State()) + { + case WAM::StartupTaskState::EnabledByPolicy: + case WAM::StartupTaskState::DisabledByPolicy: + return winrt::hstring{ L"\uE72E " } /*lock icon*/ + RS_(L"Globals_StartOnUserLogin_UnavailableByPolicy"); + case WAM::StartupTaskState::DisabledByUser: + return RS_(L"Globals_StartOnUserLogin_DisabledByUser"); + case WAM::StartupTaskState::Enabled: + case WAM::StartupTaskState::Disabled: + default: + break; // fall through to the common case (no task, not configured, etc.) + } + } + return RS_(L"Globals_StartOnUserLogin/HelpText"); + } + + bool LaunchViewModel::StartOnUserLogin() + { + if (!_startOnUserLoginTask) + { + return false; + } + namespace WAM = winrt::Windows::ApplicationModel; + const auto state{ _startOnUserLoginTask.State() }; + return state == WAM::StartupTaskState::Enabled || state == WAM::StartupTaskState::EnabledByPolicy; + } + + safe_void_coroutine LaunchViewModel::StartOnUserLogin(bool enable) + { + if (!_startOnUserLoginTask) + { + co_return; + } + + auto strongThis{ get_strong() }; + if (enable) + { + co_await _startOnUserLoginTask.RequestEnableAsync(); + } + else + { + _startOnUserLoginTask.Disable(); + } + // Any of these could have changed in response to an attempt to enable (e.g. it was disabled in task manager since our last check) + _NotifyChanges(L"StartOnUserLoginConfigurable", L"StartOnUserLoginStatefulHelpText", L"StartOnUserLogin"); + } } diff --git a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.h b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.h index bd6df9299d..d96046524f 100644 --- a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.h +++ b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.h @@ -6,6 +6,7 @@ #include "LaunchViewModel.g.h" #include "ViewModelHelpers.h" #include "Utils.h" +#include namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { @@ -51,10 +52,16 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation GETSET_BINDABLE_ENUM_SETTING(WindowingBehavior, Model::WindowingMode, _Settings.GlobalSettings().WindowingBehavior); PERMANENT_OBSERVABLE_PROJECTED_SETTING(_Settings.GlobalSettings(), CenterOnLaunch); - PERMANENT_OBSERVABLE_PROJECTED_SETTING(_Settings.GlobalSettings(), StartOnUserLogin); PERMANENT_OBSERVABLE_PROJECTED_SETTING(_Settings.GlobalSettings(), InitialRows); PERMANENT_OBSERVABLE_PROJECTED_SETTING(_Settings.GlobalSettings(), InitialCols); + bool StartOnUserLoginAvailable(); + safe_void_coroutine PrepareStartOnUserLoginSettings(); + bool StartOnUserLoginConfigurable(); + winrt::hstring StartOnUserLoginStatefulHelpText(); + bool StartOnUserLogin(); + safe_void_coroutine StartOnUserLogin(bool enable); + private: Model::CascadiaSettings _Settings; winrt::Windows::Foundation::Collections::IObservableVector _languageList; @@ -63,6 +70,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation winrt::Windows::Foundation::Collections::IObservableVector _LaunchModeList; winrt::Windows::Foundation::Collections::IMap _LaunchModeMap; + + winrt::Windows::ApplicationModel::StartupTask _startOnUserLoginTask{ nullptr }; }; }; diff --git a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.idl b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.idl index ffe2b6b482..3781c24b87 100644 --- a/src/cascadia/TerminalSettingsEditor/LaunchViewModel.idl +++ b/src/cascadia/TerminalSettingsEditor/LaunchViewModel.idl @@ -9,8 +9,6 @@ namespace Microsoft.Terminal.Settings.Editor { runtimeclass LaunchViewModel : Windows.UI.Xaml.Data.INotifyPropertyChanged { - LaunchViewModel(Microsoft.Terminal.Settings.Model.CascadiaSettings settings); - static String LanguageDisplayConverter(String tag); Boolean LanguageSelectorAvailable { get; }; Windows.Foundation.Collections.IObservableVector LanguageList { get; }; @@ -41,8 +39,12 @@ namespace Microsoft.Terminal.Settings.Editor IObservableVector WindowingBehaviorList { get; }; PERMANENT_OBSERVABLE_PROJECTED_SETTING(Boolean, CenterOnLaunch); - PERMANENT_OBSERVABLE_PROJECTED_SETTING(Boolean, StartOnUserLogin); PERMANENT_OBSERVABLE_PROJECTED_SETTING(Int32, InitialRows); PERMANENT_OBSERVABLE_PROJECTED_SETTING(Int32, InitialCols); + + Boolean StartOnUserLogin { get; set; }; + Boolean StartOnUserLoginAvailable { get; }; + Boolean StartOnUserLoginConfigurable { get; }; + String StartOnUserLoginStatefulHelpText { get; }; } } diff --git a/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw b/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw index 227c93e9b5..5d98ce728e 100644 --- a/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw @@ -2332,4 +2332,12 @@ None Text displayed when the tab title is not defined. + + Automatic startup has been disabled in the Startup Apps section of Windows settings. + {Locked="Windows"}This is displayed in concordance with Globals_StartOnUserLogin if the user has disabled the setting outside of the application. + + + This option is managed by enterprise policy and cannot be changed here. + This is displayed in concordance with Globals_StartOnUserLogin if the enterprise administrator has taken control of this setting. + \ No newline at end of file diff --git a/src/cascadia/TerminalSettingsEditor/SettingContainer.cpp b/src/cascadia/TerminalSettingsEditor/SettingContainer.cpp index d197475e02..2ab1614123 100644 --- a/src/cascadia/TerminalSettingsEditor/SettingContainer.cpp +++ b/src/cascadia/TerminalSettingsEditor/SettingContainer.cpp @@ -46,7 +46,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation L"HelpText", xaml_typename(), xaml_typename(), - PropertyMetadata{ box_value(L"") }); + PropertyMetadata{ box_value(L""), PropertyChangedCallback{ &SettingContainer::_OnHelpTextChanged } }); } if (!_FontIconGlyphProperty) { @@ -126,48 +126,15 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation get_self(obj)->_UpdateOverrideSystem(); } - void SettingContainer::OnApplyTemplate() + void SettingContainer::_OnHelpTextChanged(const DependencyObject& d, const DependencyPropertyChangedEventArgs& /*args*/) { - if (const auto& child{ GetTemplateChild(L"ResetButton") }) - { - if (const auto& button{ child.try_as() }) - { - // Apply click handler for the reset button. - // When clicked, we dispatch the bound ClearSettingValue event, - // resulting in inheriting the setting value from the parent. - button.Click([=](auto&&, auto&&) { - ClearSettingValue.raise(*this, nullptr); - - // move the focus to the child control - if (const auto& content{ Content() }) - { - if (const auto& control{ content.try_as() }) - { - control.Focus(FocusState::Programmatic); - return; - } - else if (const auto& panel{ content.try_as() }) - { - for (const auto& panelChild : panel.Children()) - { - if (const auto& panelControl{ panelChild.try_as() }) - { - panelControl.Focus(FocusState::Programmatic); - return; - } - } - } - // if we get here, we didn't find something to reasonably focus to. - } - }); - - // apply name (automation property) - Automation::AutomationProperties::SetName(child, RS_(L"SettingContainer_OverrideMessageBaseLayer")); - } - } - - _UpdateOverrideSystem(); + // update visibility for override message and reset button + const auto& obj{ d.try_as() }; + get_self(obj)->_UpdateHelpText(); + } + void SettingContainer::_UpdateHelpText() + { // Get the correct base to apply automation properties to std::vector base; base.reserve(2); @@ -215,6 +182,50 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation } } + void SettingContainer::OnApplyTemplate() + { + if (const auto& child{ GetTemplateChild(L"ResetButton") }) + { + if (const auto& button{ child.try_as() }) + { + // Apply click handler for the reset button. + // When clicked, we dispatch the bound ClearSettingValue event, + // resulting in inheriting the setting value from the parent. + button.Click([=](auto&&, auto&&) { + ClearSettingValue.raise(*this, nullptr); + + // move the focus to the child control + if (const auto& content{ Content() }) + { + if (const auto& control{ content.try_as() }) + { + control.Focus(FocusState::Programmatic); + return; + } + else if (const auto& panel{ content.try_as() }) + { + for (const auto& panelChild : panel.Children()) + { + if (const auto& panelControl{ panelChild.try_as() }) + { + panelControl.Focus(FocusState::Programmatic); + return; + } + } + } + // if we get here, we didn't find something to reasonably focus to. + } + }); + + // apply name (automation property) + Automation::AutomationProperties::SetName(child, RS_(L"SettingContainer_OverrideMessageBaseLayer")); + } + } + + _UpdateOverrideSystem(); + _UpdateHelpText(); + } + void SettingContainer::SetExpanded(bool expanded) { if (const auto& child{ GetTemplateChild(L"Expander") }) diff --git a/src/cascadia/TerminalSettingsEditor/SettingContainer.h b/src/cascadia/TerminalSettingsEditor/SettingContainer.h index 862d3e8660..9c77a838e0 100644 --- a/src/cascadia/TerminalSettingsEditor/SettingContainer.h +++ b/src/cascadia/TerminalSettingsEditor/SettingContainer.h @@ -47,9 +47,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation static void _InitializeProperties(); static void _OnCurrentValueChanged(const Windows::UI::Xaml::DependencyObject& d, const Windows::UI::Xaml::DependencyPropertyChangedEventArgs& e); static void _OnHasSettingValueChanged(const Windows::UI::Xaml::DependencyObject& d, const Windows::UI::Xaml::DependencyPropertyChangedEventArgs& e); + static void _OnHelpTextChanged(const Windows::UI::Xaml::DependencyObject& d, const Windows::UI::Xaml::DependencyPropertyChangedEventArgs& e); static hstring _GenerateOverrideMessage(const IInspectable& settingOrigin); hstring _GenerateAccessibleName(); void _UpdateOverrideSystem(); + void _UpdateHelpText(); void _UpdateCurrentValueAutoProp(); }; } diff --git a/src/cascadia/TerminalSettingsEditor/SettingContainerStyle.xaml b/src/cascadia/TerminalSettingsEditor/SettingContainerStyle.xaml index 5ef4acd978..c4d3c7a580 100644 --- a/src/cascadia/TerminalSettingsEditor/SettingContainerStyle.xaml +++ b/src/cascadia/TerminalSettingsEditor/SettingContainerStyle.xaml @@ -176,6 +176,7 @@ + diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl index cee08a2656..9bf5267733 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl @@ -84,7 +84,6 @@ namespace Microsoft.Terminal.Settings.Model INHERITABLE_SETTING(Microsoft.Terminal.Control.TextMeasurement, TextMeasurement); INHERITABLE_SETTING(Boolean, UseBackgroundImageForWindow); INHERITABLE_SETTING(Boolean, DebugFeaturesEnabled); - INHERITABLE_SETTING(Boolean, StartOnUserLogin); INHERITABLE_SETTING(Boolean, AlwaysOnTop); INHERITABLE_SETTING(Boolean, AutoHideWindow); INHERITABLE_SETTING(TabSwitcherMode, TabSwitcherMode); diff --git a/src/cascadia/TerminalSettingsModel/MTSMSettings.h b/src/cascadia/TerminalSettingsModel/MTSMSettings.h index 070792d149..eb195f2695 100644 --- a/src/cascadia/TerminalSettingsModel/MTSMSettings.h +++ b/src/cascadia/TerminalSettingsModel/MTSMSettings.h @@ -51,7 +51,6 @@ Author(s): X(Model::LaunchMode, LaunchMode, "launchMode", LaunchMode::DefaultMode) \ X(bool, SnapToGridOnResize, "snapToGridOnResize", true) \ X(bool, DebugFeaturesEnabled, "debugFeatures", debugFeaturesDefault) \ - X(bool, StartOnUserLogin, "startOnUserLogin", false) \ X(bool, AlwaysOnTop, "alwaysOnTop", false) \ X(bool, AutoHideWindow, "autoHideWindow", false) \ X(Model::TabSwitcherMode, TabSwitcherMode, "tabSwitcherMode", Model::TabSwitcherMode::InOrder) \ diff --git a/src/cascadia/TerminalSettingsModel/defaults.json b/src/cascadia/TerminalSettingsModel/defaults.json index 4de4d8788f..d8840bb0cd 100644 --- a/src/cascadia/TerminalSettingsModel/defaults.json +++ b/src/cascadia/TerminalSettingsModel/defaults.json @@ -25,7 +25,6 @@ // Miscellaneous "confirmCloseAllTabs": true, - "startOnUserLogin": false, "theme": "dark", "snapToGridOnResize": true, "disableAnimations": false, diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index a6feec86cd..d3d6586479 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -117,7 +117,6 @@ namespace SettingsModelUnitTests "tabWidthMode": "equal", "tabSwitcherMode": "mru", - "startOnUserLogin": false, "theme": "system", "snapToGridOnResize": true, "disableAnimations": false,