From 3aa7a52c21cb5a468d51b9e07c3f9694e64d6005 Mon Sep 17 00:00:00 2001 From: vldmr11080 <57061786+vldmr11080@users.noreply.github.com> Date: Fri, 18 Sep 2020 15:18:01 +0200 Subject: [PATCH] [FancyZones] Initial improvements in FancyZones exception handling (#6063) * Initial improvements in FancyZones exception handling * Add callback * Disable FancyZones if error durign loading data occurrs * Remove logs * Add resource strings * Add 1sec retry when failure during initialization occurs * Rephrase error descriptions * Error handling during loading of module in runner * Pass error handling on the runner * Remove unneeded string * Remove unnedeed changes --- src/modules/fancyzones/dll/dllmain.cpp | 4 +- src/modules/fancyzones/lib/FancyZones.cpp | 15 +- src/modules/fancyzones/lib/FancyZones.h | 4 +- src/modules/fancyzones/lib/FancyZonesData.cpp | 1 + src/modules/fancyzones/lib/Resources.resx | 16 +- src/modules/fancyzones/lib/Settings.cpp | 178 +++++++++++------- .../tests/UnitTests/FancyZones.Spec.cpp | 12 +- src/runner/main.cpp | 7 + src/runner/powertoy_module.cpp | 2 +- 9 files changed, 153 insertions(+), 86 deletions(-) diff --git a/src/modules/fancyzones/dll/dllmain.cpp b/src/modules/fancyzones/dll/dllmain.cpp index 4824a90d68..d988a9796c 100644 --- a/src/modules/fancyzones/dll/dllmain.cpp +++ b/src/modules/fancyzones/dll/dllmain.cpp @@ -71,7 +71,7 @@ public: { InitializeWinhookEventIds(); Trace::FancyZones::EnableFancyZones(true); - m_app = MakeFancyZones(reinterpret_cast(&__ImageBase), m_settings); + m_app = MakeFancyZones(reinterpret_cast(&__ImageBase), m_settings, std::bind(&FancyZonesModule::disable, this)); #if defined(DISABLE_LOWLEVEL_HOOKS_WHEN_DEBUGGED) const bool hook_disabled = IsDebuggerPresent(); #else @@ -129,7 +129,7 @@ public: // Returns if the powertoy is enabled virtual bool is_enabled() override { - return (m_app != nullptr); + return m_app != nullptr; } // Destroy the powertoy and free memory diff --git a/src/modules/fancyzones/lib/FancyZones.cpp b/src/modules/fancyzones/lib/FancyZones.cpp index 9e70fccb1f..1f221f1cda 100644 --- a/src/modules/fancyzones/lib/FancyZones.cpp +++ b/src/modules/fancyzones/lib/FancyZones.cpp @@ -45,7 +45,7 @@ namespace NonLocalizable struct FancyZones : public winrt::implements { public: - FancyZones(HINSTANCE hinstance, const winrt::com_ptr& settings) noexcept : + FancyZones(HINSTANCE hinstance, const winrt::com_ptr& settings, std::function disableModuleCallback) noexcept : m_hinstance(hinstance), m_settings(settings), m_windowMoveHandler(settings, [this]() { @@ -53,6 +53,8 @@ public: }) { m_settings->SetCallback(this); + + this->disableModuleCallback = std::move(disableModuleCallback); } // IFancyZones @@ -240,6 +242,9 @@ private: OnThreadExecutor m_dpiUnawareThread; OnThreadExecutor m_virtualDesktopTrackerThread; + // If non-recoverable error occurs, trigger disabling of entire FancyZones. + static std::function disableModuleCallback; + static UINT WM_PRIV_VD_INIT; // Scheduled when FancyZones is initialized static UINT WM_PRIV_VD_SWITCH; // Scheduled when virtual desktop switch occurs static UINT WM_PRIV_VD_UPDATE; // Scheduled on virtual desktops update (creation/deletion) @@ -255,6 +260,8 @@ private: }; }; +std::function FancyZones::disableModuleCallback = {}; + UINT FancyZones::WM_PRIV_VD_INIT = RegisterWindowMessage(L"{469818a8-00fa-4069-b867-a1da484fcd9a}"); UINT FancyZones::WM_PRIV_VD_SWITCH = RegisterWindowMessage(L"{128c2cb0-6bdf-493e-abbe-f8705e04aa95}"); UINT FancyZones::WM_PRIV_VD_UPDATE = RegisterWindowMessage(L"{b8b72b46-f42f-4c26-9e20-29336cf2f22e}"); @@ -1385,12 +1392,14 @@ std::vector> FancyZones::GetRawMonitorData() noexcept return monitorInfo; } -winrt::com_ptr MakeFancyZones(HINSTANCE hinstance, const winrt::com_ptr& settings) noexcept +winrt::com_ptr MakeFancyZones(HINSTANCE hinstance, + const winrt::com_ptr& settings, + std::function disableCallback) noexcept { if (!settings) { return nullptr; } - return winrt::make_self(hinstance, settings); + return winrt::make_self(hinstance, settings, disableCallback); } diff --git a/src/modules/fancyzones/lib/FancyZones.h b/src/modules/fancyzones/lib/FancyZones.h index 5d6e455b2b..ea31a67148 100644 --- a/src/modules/fancyzones/lib/FancyZones.h +++ b/src/modules/fancyzones/lib/FancyZones.h @@ -2,6 +2,8 @@ #include +#include + interface IZoneWindow; interface IFancyZonesSettings; interface IZoneSet; @@ -102,4 +104,4 @@ interface __declspec(uuid("{5C8D99D6-34B2-4F4A-A8E5-7483F6869775}")) IZoneWindow () = 0; }; -winrt::com_ptr MakeFancyZones(HINSTANCE hinstance, const winrt::com_ptr& settings) noexcept; +winrt::com_ptr MakeFancyZones(HINSTANCE hinstance, const winrt::com_ptr& settings, std::function disableCallback) noexcept; diff --git a/src/modules/fancyzones/lib/FancyZonesData.cpp b/src/modules/fancyzones/lib/FancyZonesData.cpp index e95a70e7f8..a2a7e7f19b 100644 --- a/src/modules/fancyzones/lib/FancyZonesData.cpp +++ b/src/modules/fancyzones/lib/FancyZonesData.cpp @@ -148,6 +148,7 @@ FancyZonesData& FancyZonesDataInstance() FancyZonesData::FancyZonesData() { std::wstring saveFolderPath = PTSettingsHelper::get_module_save_folder_location(NonLocalizable::FancyZonesStr); + zonesSettingsFileName = saveFolderPath + L"\\" + std::wstring(NonLocalizable::FancyZonesDataFile); appZoneHistoryFileName = saveFolderPath + L"\\" + std::wstring(NonLocalizable::FancyZonesAppZoneHistoryFile); diff --git a/src/modules/fancyzones/lib/Resources.resx b/src/modules/fancyzones/lib/Resources.resx index 696299ccb0..415d7cf880 100644 --- a/src/modules/fancyzones/lib/Resources.resx +++ b/src/modules/fancyzones/lib/Resources.resx @@ -205,10 +205,10 @@ Don't show again - Cannot install keyboard listener. + Failed to install keyboard listener. - Cannot install Windows event listener. + Failed to install Windows event listener. PowerToys - FancyZones @@ -216,4 +216,16 @@ The 'Allow zones to span across monitors' option requires all monitors to have the same DPI scaling to work properly. + + FancyZones persisted data path not found. Please report the bug to + + + The FancyZones editor failed to start. Please report the bug to + + + Failed to load the FancyZones settings. Default settings will be used. + + + Failed to save the FancyZones settings. Please retry again later, if the problem persists report the bug to + \ No newline at end of file diff --git a/src/modules/fancyzones/lib/Settings.cpp b/src/modules/fancyzones/lib/Settings.cpp index 660e2fe03e..bca90ba4f2 100644 --- a/src/modules/fancyzones/lib/Settings.cpp +++ b/src/modules/fancyzones/lib/Settings.cpp @@ -1,9 +1,12 @@ #include "pch.h" #include +#include #include "lib/Settings.h" #include "lib/FancyZones.h" #include "trace.h" +extern "C" IMAGE_DOS_HEADER __ImageBase; + // Non-Localizable strings namespace NonLocalizable { @@ -34,6 +37,7 @@ namespace NonLocalizable const wchar_t IconKeyID[] = L"pt-fancy-zones"; const wchar_t OverviewURL[] = L"https://aka.ms/PowerToysOverview_FancyZones"; const wchar_t VideoURL[] = L"https://youtu.be/rTtGzZYAXgY"; + const wchar_t PowerToysIssuesURL[] = L"https://aka.ms/powerToysReportBug"; } struct FancyZonesSettings : winrt::implements @@ -126,7 +130,7 @@ IFACEMETHODIMP_(bool) FancyZonesSettings::GetConfig(_Out_ PWSTR buffer, _Out_ in return settings.serialize_to_buffer(buffer, buffer_size); } -IFACEMETHODIMP_(void) FancyZonesSettings::SetConfig(PCWSTR serializedPowerToysSettingsJson) noexcept try +IFACEMETHODIMP_(void) FancyZonesSettings::SetConfig(PCWSTR serializedPowerToysSettingsJson) noexcept { LoadSettings(serializedPowerToysSettingsJson, false /*fromFile*/); SaveSettings(); @@ -136,104 +140,136 @@ IFACEMETHODIMP_(void) FancyZonesSettings::SetConfig(PCWSTR serializedPowerToysSe } Trace::SettingsChanged(m_settings); } -CATCH_LOG(); -IFACEMETHODIMP_(void) FancyZonesSettings::CallCustomAction(PCWSTR action) noexcept try +IFACEMETHODIMP_(void) FancyZonesSettings::CallCustomAction(PCWSTR action) noexcept { - // Parse the action values, including name. - PowerToysSettings::CustomActionObject action_object = - PowerToysSettings::CustomActionObject::from_json_string(action); - - if (m_callback && action_object.get_name() == NonLocalizable::ToggleEditorActionID) + try { - m_callback->ToggleEditor(); + // Parse the action values, including name. + PowerToysSettings::CustomActionObject action_object = + PowerToysSettings::CustomActionObject::from_json_string(action); + + if (m_callback && action_object.get_name() == NonLocalizable::ToggleEditorActionID) + { + m_callback->ToggleEditor(); + } + } + catch (...) + { + // Currently only custom action comming from main PowerToys window is request to launch editor. + // If new custom action is added, error message need to be modified. + std::wstring errorMessage = GET_RESOURCE_STRING(IDS_FANCYZONES_EDITOR_LAUNCH_ERROR) + L" " + NonLocalizable::PowerToysIssuesURL; + MessageBox(NULL, + errorMessage.c_str(), + GET_RESOURCE_STRING(IDS_POWERTOYS_FANCYZONES).c_str(), + MB_OK); } } -CATCH_LOG(); -void FancyZonesSettings::LoadSettings(PCWSTR config, bool fromFile) noexcept try +void FancyZonesSettings::LoadSettings(PCWSTR config, bool fromFile) noexcept { - PowerToysSettings::PowerToyValues values = fromFile ? - PowerToysSettings::PowerToyValues::load_from_settings_file(m_moduleName) : - PowerToysSettings::PowerToyValues::from_json_string(config); - - for (auto const& setting : m_configBools) + try { - if (const auto val = values.get_bool_value(setting.name)) + PowerToysSettings::PowerToyValues values = fromFile ? + PowerToysSettings::PowerToyValues::load_from_settings_file(m_moduleName) : + PowerToysSettings::PowerToyValues::from_json_string(config); + + for (auto const& setting : m_configBools) { - *setting.value = *val; + if (const auto val = values.get_bool_value(setting.name)) + { + *setting.value = *val; + } } - } - if (auto val = values.get_string_value(NonLocalizable::ZoneColorID)) - { - m_settings.zoneColor = std::move(*val); - } - - if (auto val = values.get_string_value(NonLocalizable::ZoneBorderColorID)) - { - m_settings.zoneBorderColor = std::move(*val); - } - - if (auto val = values.get_string_value(NonLocalizable::ZoneHighlightColorID)) - { - m_settings.zoneHighlightColor = std::move(*val); - } - - if (const auto val = values.get_json(NonLocalizable::EditorHotkeyID)) - { - m_settings.editorHotkey = PowerToysSettings::HotkeyObject::from_json(*val); - } - - if (auto val = values.get_string_value(NonLocalizable::ExcludedAppsID)) - { - m_settings.excludedApps = std::move(*val); - m_settings.excludedAppsArray.clear(); - auto excludedUppercase = m_settings.excludedApps; - CharUpperBuffW(excludedUppercase.data(), (DWORD)excludedUppercase.length()); - std::wstring_view view(excludedUppercase); - while (view.starts_with('\n') || view.starts_with('\r')) + if (auto val = values.get_string_value(NonLocalizable::ZoneColorID)) { - view.remove_prefix(1); + m_settings.zoneColor = std::move(*val); } - while (!view.empty()) + + if (auto val = values.get_string_value(NonLocalizable::ZoneBorderColorID)) { - auto pos = (std::min)(view.find_first_of(L"\r\n"), view.length()); - m_settings.excludedAppsArray.emplace_back(view.substr(0, pos)); - view.remove_prefix(pos); + m_settings.zoneBorderColor = std::move(*val); + } + + if (auto val = values.get_string_value(NonLocalizable::ZoneHighlightColorID)) + { + m_settings.zoneHighlightColor = std::move(*val); + } + + if (const auto val = values.get_json(NonLocalizable::EditorHotkeyID)) + { + m_settings.editorHotkey = PowerToysSettings::HotkeyObject::from_json(*val); + } + + if (auto val = values.get_string_value(NonLocalizable::ExcludedAppsID)) + { + m_settings.excludedApps = std::move(*val); + m_settings.excludedAppsArray.clear(); + auto excludedUppercase = m_settings.excludedApps; + CharUpperBuffW(excludedUppercase.data(), (DWORD)excludedUppercase.length()); + std::wstring_view view(excludedUppercase); while (view.starts_with('\n') || view.starts_with('\r')) { view.remove_prefix(1); } + while (!view.empty()) + { + auto pos = (std::min)(view.find_first_of(L"\r\n"), view.length()); + m_settings.excludedAppsArray.emplace_back(view.substr(0, pos)); + view.remove_prefix(pos); + while (view.starts_with('\n') || view.starts_with('\r')) + { + view.remove_prefix(1); + } + } + } + + if (auto val = values.get_int_value(NonLocalizable::ZoneHighlightOpacityID)) + { + m_settings.zoneHighlightOpacity = *val; } } - - if (auto val = values.get_int_value(NonLocalizable::ZoneHighlightOpacityID)) + catch (...) { - m_settings.zoneHighlightOpacity = *val; + // Failure to load settings does not break FancyZones functionality. Display error message and continue with default settings. + MessageBox(NULL, + GET_RESOURCE_STRING(IDS_FANCYZONES_SETTINGS_LOAD_ERROR).c_str(), + GET_RESOURCE_STRING(IDS_POWERTOYS_FANCYZONES).c_str(), + MB_OK); } } -CATCH_LOG(); -void FancyZonesSettings::SaveSettings() noexcept try +void FancyZonesSettings::SaveSettings() noexcept { - PowerToysSettings::PowerToyValues values(m_moduleName); - - for (auto const& setting : m_configBools) + try { - values.add_property(setting.name, *setting.value); + PowerToysSettings::PowerToyValues values(m_moduleName); + + for (auto const& setting : m_configBools) + { + values.add_property(setting.name, *setting.value); + } + + values.add_property(NonLocalizable::ZoneColorID, m_settings.zoneColor); + values.add_property(NonLocalizable::ZoneBorderColorID, m_settings.zoneBorderColor); + values.add_property(NonLocalizable::ZoneHighlightColorID, m_settings.zoneHighlightColor); + values.add_property(NonLocalizable::ZoneHighlightOpacityID, m_settings.zoneHighlightOpacity); + values.add_property(NonLocalizable::EditorHotkeyID, m_settings.editorHotkey.get_json()); + values.add_property(NonLocalizable::ExcludedAppsID, m_settings.excludedApps); + + values.save_to_settings_file(); + } + catch (...) + { + // Failure to save settings does not break FancyZones functionality. Display error message and continue with currently cached settings. + std::wstring errorMessage = GET_RESOURCE_STRING(IDS_FANCYZONES_SETTINGS_LOAD_ERROR) + L" " + NonLocalizable::PowerToysIssuesURL; + MessageBox(NULL, + errorMessage.c_str(), + GET_RESOURCE_STRING(IDS_POWERTOYS_FANCYZONES).c_str(), + MB_OK); } - - values.add_property(NonLocalizable::ZoneColorID, m_settings.zoneColor); - values.add_property(NonLocalizable::ZoneBorderColorID, m_settings.zoneBorderColor); - values.add_property(NonLocalizable::ZoneHighlightColorID, m_settings.zoneHighlightColor); - values.add_property(NonLocalizable::ZoneHighlightOpacityID, m_settings.zoneHighlightOpacity); - values.add_property(NonLocalizable::EditorHotkeyID, m_settings.editorHotkey.get_json()); - values.add_property(NonLocalizable::ExcludedAppsID, m_settings.excludedApps); - - values.save_to_settings_file(); } -CATCH_LOG(); winrt::com_ptr MakeFancyZonesSettings(HINSTANCE hinstance, PCWSTR name) noexcept { diff --git a/src/modules/fancyzones/tests/UnitTests/FancyZones.Spec.cpp b/src/modules/fancyzones/tests/UnitTests/FancyZones.Spec.cpp index 6627e5d9a1..ce5df79dbc 100644 --- a/src/modules/fancyzones/tests/UnitTests/FancyZones.Spec.cpp +++ b/src/modules/fancyzones/tests/UnitTests/FancyZones.Spec.cpp @@ -32,24 +32,24 @@ namespace FancyZonesUnitTests TEST_METHOD (Create) { - auto actual = MakeFancyZones(m_hInst, m_settings); + auto actual = MakeFancyZones(m_hInst, m_settings, nullptr); Assert::IsNotNull(actual.get()); } TEST_METHOD (CreateWithEmptyHinstance) { - auto actual = MakeFancyZones({}, m_settings); + auto actual = MakeFancyZones({}, m_settings, nullptr); Assert::IsNotNull(actual.get()); } TEST_METHOD (CreateWithNullHinstance) { - auto actual = MakeFancyZones(nullptr, m_settings); + auto actual = MakeFancyZones(nullptr, m_settings, nullptr); Assert::IsNotNull(actual.get()); } TEST_METHOD (CreateWithNullSettings) { - auto actual = MakeFancyZones(m_hInst, nullptr); + auto actual = MakeFancyZones(m_hInst, nullptr, nullptr); Assert::IsNull(actual.get()); } }; @@ -95,7 +95,7 @@ namespace FancyZonesUnitTests m_settings = MakeFancyZonesSettings(m_hInst, m_settingsLocation.c_str()); Assert::IsTrue(m_settings != nullptr); - auto fancyZones = MakeFancyZones(m_hInst, m_settings); + auto fancyZones = MakeFancyZones(m_hInst, m_settings, nullptr); Assert::IsTrue(fancyZones != nullptr); m_zoneWindowHost = fancyZones.as(); @@ -331,7 +331,7 @@ namespace FancyZonesUnitTests m_settings = MakeFancyZonesSettings(m_hInst, m_settingsLocation.c_str()); Assert::IsTrue(m_settings != nullptr); - auto fancyZones = MakeFancyZones(m_hInst, m_settings); + auto fancyZones = MakeFancyZones(m_hInst, m_settings, nullptr); Assert::IsTrue(fancyZones != nullptr); m_fzCallback = fancyZones.as(); diff --git a/src/runner/main.cpp b/src/runner/main.cpp index a66adf3d4a..9b1fe1fb44 100644 --- a/src/runner/main.cpp +++ b/src/runner/main.cpp @@ -41,6 +41,8 @@ namespace localized_strings const wchar_t DOWNLOAD_UPDATE_ERROR[] = L"Couldn't download PowerToys update! Please report the issue on Github."; const wchar_t OLDER_MSIX_UNINSTALLED[] = L"An older MSIX version of PowerToys was uninstalled."; const wchar_t PT_UPDATE_MESSAGE_BOX_TEXT[] = L"PowerToys was updated successfully!"; + const wchar_t POWER_TOYS[] = L"PowerToys"; + const wchar_t POWER_TOYS_MODULE_LOAD_FAIL[] = L"Failed to load "; // Module name will be appended on this message and it is not localized. } namespace @@ -136,6 +138,11 @@ int runner(bool isProcessElevated) } catch (...) { + std::wstring errorMessage = std::wstring(localized_strings::POWER_TOYS_MODULE_LOAD_FAIL) + moduleSubdir.data(); + MessageBox(NULL, + errorMessage.c_str(), + localized_strings::POWER_TOYS, + MB_OK | MB_ICONERROR); } } // Start initial powertoys diff --git a/src/runner/powertoy_module.cpp b/src/runner/powertoy_module.cpp index 2b81f405c9..23853cda07 100644 --- a/src/runner/powertoy_module.cpp +++ b/src/runner/powertoy_module.cpp @@ -20,7 +20,7 @@ PowertoyModule load_powertoy(const std::wstring_view filename) if (!module) { FreeLibrary(handle); - winrt::throw_last_error(); + winrt::throw_hresult(winrt::hresult(E_POINTER)); } return PowertoyModule(module, handle); }