Skip to content

Commit

Permalink
Fix SUI race conditions when reloading settings
Browse files Browse the repository at this point in the history
  • Loading branch information
@lhecker
lhecker committed Jun 10, 2021
1 parent becc254 commit a193fae
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 123 deletions.
85 changes: 32 additions & 53 deletions src/cascadia/TerminalApp/AppLogic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ namespace winrt::TerminalApp::implementation
});
_root->Create();

_ApplyTheme(_settings.GlobalSettings().Theme());
_RefreshThemeRoutine();
_ApplyStartupTaskStateChange();

TraceLoggingWrite(
Expand Down Expand Up @@ -895,36 +895,25 @@ namespace winrt::TerminalApp::implementation
// this stops us from reloading too many times or too quickly.
fire_and_forget AppLogic::_DispatchReloadSettings()
{
static constexpr auto FileActivityQuiesceTime{ std::chrono::milliseconds(50) };
if (!_settingsReloadQueued.exchange(true))
if (_settingsReloadQueued.exchange(true))
{
co_await winrt::resume_after(FileActivityQuiesceTime);
_ReloadSettings();
_settingsReloadQueued.store(false);
co_return;
}
}

fire_and_forget AppLogic::_LoadErrorsDialogRoutine()
{
co_await winrt::resume_foreground(_root->Dispatcher());
auto weakSelf = get_weak();

const winrt::hstring titleKey = USES_RESOURCE(L"ReloadJsonParseErrorTitle");
const winrt::hstring textKey = USES_RESOURCE(L"ReloadJsonParseErrorText");
_ShowLoadErrorsDialog(titleKey, textKey, _settingsLoadedResult);
}

fire_and_forget AppLogic::_ShowLoadWarningsDialogRoutine()
{
co_await winrt::resume_after(std::chrono::milliseconds(100));
co_await winrt::resume_foreground(_root->Dispatcher());

_ShowLoadWarningsDialog();
if (auto self{ weakSelf.get() })
{
_ReloadSettings();
_settingsReloadQueued.store(false);
}
}

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

// Refresh the UI theme
_ApplyTheme(_settings.GlobalSettings().Theme());
}

Expand Down Expand Up @@ -959,39 +948,26 @@ namespace winrt::TerminalApp::implementation
return;
}

auto weakThis{ get_weak() };
co_await winrt::resume_foreground(_root->Dispatcher(), CoreDispatcherPriority::Normal);
if (auto page{ weakThis.get() })
{
StartupTaskState state;
bool tryEnableStartupTask = _settings.GlobalSettings().StartOnUserLogin();
StartupTask task = co_await StartupTask::GetAsync(StartupTaskName);
const auto tryEnableStartupTask = _settings.GlobalSettings().StartOnUserLogin();
const auto task = co_await StartupTask::GetAsync(StartupTaskName);

state = task.State();
switch (state)
{
case StartupTaskState::Disabled:
switch (task.State())
{
case StartupTaskState::Disabled:
if (tryEnableStartupTask)
{
if (tryEnableStartupTask)
{
co_await task.RequestEnableAsync();
}
break;
co_await task.RequestEnableAsync();
}
case StartupTaskState::DisabledByUser:
break;
case StartupTaskState::DisabledByUser:
// TODO: GH#6254: define UX for other StartupTaskStates
break;
case StartupTaskState::Enabled:
if (!tryEnableStartupTask)
{
// TODO: GH#6254: define UX for other StartupTaskStates
break;
}
case StartupTaskState::Enabled:
{
if (!tryEnableStartupTask)
{
task.Disable();
}
break;
}
task.Disable();
}
break;
}
}
CATCH_LOG();
Expand All @@ -1009,12 +985,15 @@ namespace winrt::TerminalApp::implementation

if (FAILED(_settingsLoadedResult))
{
_LoadErrorsDialogRoutine();
const winrt::hstring titleKey = USES_RESOURCE(L"ReloadJsonParseErrorTitle");
const winrt::hstring textKey = USES_RESOURCE(L"ReloadJsonParseErrorText");
_ShowLoadErrorsDialog(titleKey, textKey, _settingsLoadedResult);
return;
}
else if (_settingsLoadedResult == S_FALSE)

if (_settingsLoadedResult == S_FALSE)
{
_ShowLoadWarningsDialogRoutine();
_ShowLoadWarningsDialog();
}

// Here, we successfully reloaded the settings, and created a new
Expand Down
6 changes: 1 addition & 5 deletions src/cascadia/TerminalApp/AppLogic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,14 @@ namespace winrt::TerminalApp::implementation

::TerminalApp::AppCommandlineArgs _appArgs;
::TerminalApp::AppCommandlineArgs _settingsAppArgs;
int _ParseArgs(winrt::array_view<const hstring>& args);
static TerminalApp::FindTargetWindowResult _doFindTargetWindow(winrt::array_view<const hstring> args,
const Microsoft::Terminal::Settings::Model::WindowingMode& windowingBehavior);

void _ShowLoadErrorsDialog(const winrt::hstring& titleKey, const winrt::hstring& contentKey, HRESULT settingsLoadedResult);
void _ShowLoadWarningsDialog();
bool _IsKeyboardServiceEnabled();
void _ShowKeyboardServiceDisabledDialog();

fire_and_forget _LoadErrorsDialogRoutine();
fire_and_forget _ShowLoadWarningsDialogRoutine();
fire_and_forget _RefreshThemeRoutine();
void _RefreshThemeRoutine();
fire_and_forget _ApplyStartupTaskStateChange();

void _OnLoaded(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
Expand Down
48 changes: 18 additions & 30 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,31 +92,26 @@ namespace winrt::TerminalApp::implementation
}
}

winrt::fire_and_forget TerminalPage::SetSettings(CascadiaSettings settings, bool needRefreshUI)
void TerminalPage::SetSettings(CascadiaSettings settings, bool needRefreshUI)
{
_settings = settings;

auto weakThis{ get_weak() };
co_await winrt::resume_foreground(Dispatcher());
if (auto page{ weakThis.get() })
{
// Make sure to _UpdateCommandsForPalette before
// _RefreshUIForSettingsReload. _UpdateCommandsForPalette will make
// sure the KeyChordText of Commands is updated, which needs to
// happen before the Settings UI is reloaded and tries to re-read
// those values.
_UpdateCommandsForPalette();
CommandPalette().SetActionMap(_settings.ActionMap());
// Make sure to _UpdateCommandsForPalette before
// _RefreshUIForSettingsReload. _UpdateCommandsForPalette will make
// sure the KeyChordText of Commands is updated, which needs to
// happen before the Settings UI is reloaded and tries to re-read
// those values.
_UpdateCommandsForPalette();
CommandPalette().SetActionMap(_settings.ActionMap());

if (needRefreshUI)
{
_RefreshUIForSettingsReload();
}

// Upon settings update we reload the system settings for scrolling as well.
// TODO: consider reloading this value periodically.
_systemRowsToScroll = _ReadSystemRowsToScroll();
if (needRefreshUI)
{
_RefreshUIForSettingsReload();
}

// Upon settings update we reload the system settings for scrolling as well.
// TODO: consider reloading this value periodically.
_systemRowsToScroll = _ReadSystemRowsToScroll();
}

void TerminalPage::Create()
Expand Down Expand Up @@ -1830,7 +1825,7 @@ namespace winrt::TerminalApp::implementation
// This includes update the settings of all the tabs according
// to their profiles, update the title and icon of each tab, and
// finally create the tab flyout
winrt::fire_and_forget TerminalPage::_RefreshUIForSettingsReload()
void TerminalPage::_RefreshUIForSettingsReload()
{
// Re-wire the keybindings to their handlers, as we'll have created a
// new AppKeyBindings object.
Expand Down Expand Up @@ -1885,17 +1880,10 @@ namespace winrt::TerminalApp::implementation
tabImpl->SetActionMap(_settings.ActionMap());
}

auto weakThis{ get_weak() };

co_await winrt::resume_foreground(Dispatcher());

// repopulate the new tab button's flyout with entries for each
// profile, which might have changed
if (auto page{ weakThis.get() })
{
_UpdateTabWidthMode();
_CreateNewTabFlyout();
}
_UpdateTabWidthMode();
_CreateNewTabFlyout();

// Reload the current value of alwaysOnTop from the settings file. This
// will let the user hot-reload this setting, but any runtime changes to
Expand Down
6 changes: 2 additions & 4 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ namespace winrt::TerminalApp::implementation
// put it in our inheritance graph. https://github.com/microsoft/microsoft-ui-xaml/issues/3331
STDMETHODIMP Initialize(HWND hwnd);

winrt::fire_and_forget SetSettings(Microsoft::Terminal::Settings::Model::CascadiaSettings settings, bool needRefreshUI);
void SetSettings(Microsoft::Terminal::Settings::Model::CascadiaSettings settings, bool needRefreshUI);

void Create();

Expand All @@ -67,8 +67,6 @@ namespace winrt::TerminalApp::implementation
winrt::hstring ApplicationDisplayName();
winrt::hstring ApplicationVersion();

winrt::hstring ThirdPartyNoticesLink();

winrt::fire_and_forget CloseWindow();

void ToggleFocusMode();
Expand Down Expand Up @@ -296,7 +294,7 @@ namespace winrt::TerminalApp::implementation
winrt::Microsoft::Terminal::Control::TermControl _InitControl(const winrt::Microsoft::Terminal::Settings::Model::TerminalSettingsCreateResult& settings,
const winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection& connection);

winrt::fire_and_forget _RefreshUIForSettingsReload();
void _RefreshUIForSettingsReload();

void _SetNonClientAreaColors(const Windows::UI::Color& selectedTabColor);
void _ClearNonClientAreaColors();
Expand Down
67 changes: 37 additions & 30 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// - settings - the new settings source
// Return value:
// - <none>
fire_and_forget MainPage::UpdateSettings(Model::CascadiaSettings settings)
void MainPage::UpdateSettings(const Model::CascadiaSettings& settings)
{
_settingsSource = settings;
_settingsClone = settings.Copy();

co_await winrt::resume_foreground(Dispatcher());

// Deduce information about the currently selected item
IInspectable selectedItemTag;
auto menuItems{ SettingsNav().MenuItems() };
Expand All @@ -83,34 +81,43 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
}

// remove all profile-related NavViewItems by populating a std::vector
// with the ones we want to keep.
// NOTE: menuItems.Remove() causes an out-of-bounds crash. Using ReplaceAll()
// gets around this crash.
std::vector<IInspectable> menuItemsSTL;
for (const auto& item : menuItems)
{
if (const auto& navViewItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
if (const auto& tag{ navViewItem.Tag() })
{
if (tag.try_as<Editor::ProfileViewModel>())
// We'll remove a bunch of items and iterate over it twice.
// --> Copy it into an STL vector to simplify our code and reduce COM overhead.
std::vector<IInspectable> menuItemsSTL(menuItems.Size(), nullptr);
menuItems.GetMany(0, menuItemsSTL);

// We want to refresh the list of profiles in the NavigationView.
// In order to add profiles we can use _InitializeProfilesList();
// But before we can do that we have to remove existing profiles first of course.
// This "erase-remove" idiom will achieve just that.
menuItemsSTL.erase(
std::remove_if(
menuItemsSTL.begin(),
menuItemsSTL.end(),
[](const auto& item) -> bool {
if (const auto& navViewItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
// don't add NavViewItem pointing to a Profile
continue;
}
else if (const auto& stringTag{ tag.try_as<hstring>() })
{
if (stringTag == addProfileTag)
if (const auto& tag{ navViewItem.Tag() })
{
// don't add the "Add Profile" item
continue;
if (tag.try_as<Editor::ProfileViewModel>())
{
// don't add NavViewItem pointing to a Profile
return true;
}
if (const auto& stringTag{ tag.try_as<hstring>() })
{
if (stringTag == addProfileTag)
{
// don't add the "Add Profile" item
return true;
}
}
}
}
}
}
menuItemsSTL.emplace_back(item);
}
return false;
}),
menuItemsSTL.end());

menuItems.ReplaceAll(menuItemsSTL);

// Repopulate profile-related menu items
Expand All @@ -123,7 +130,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// refresh the current page using the SelectedItem data we collected before the refresh
if (selectedItemTag)
{
for (const auto& item : menuItems)
for (const auto& item : menuItemsSTL)
{
if (const auto& menuItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
Expand All @@ -138,7 +145,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// found the one that was selected before the refresh
SettingsNav().SelectedItem(item);
_Navigate(*stringTag);
co_return;
return;
}
}
}
Expand All @@ -151,7 +158,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// found the one that was selected before the refresh
SettingsNav().SelectedItem(item);
_Navigate(*profileTag);
co_return;
return;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsEditor/MainPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
MainPage() = delete;
MainPage(const Model::CascadiaSettings& settings);

fire_and_forget UpdateSettings(Model::CascadiaSettings settings);
void UpdateSettings(const Model::CascadiaSettings& settings);

void OpenJsonKeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& args);
void OpenJsonTapped(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::TappedRoutedEventArgs const& args);
Expand Down

0 comments on commit a193fae

Please sign in to comment.