[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

@@ -71,7 +71,7 @@ public:
         {
             InitializeWinhookEventIds();
             Trace::FancyZones::EnableFancyZones(true);
-            m_app = MakeFancyZones(reinterpret_cast<HINSTANCE>(&__ImageBase), m_settings);
+            m_app = MakeFancyZones(reinterpret_cast<HINSTANCE>(&__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

src/modules/fancyzones/lib/FancyZones.cpp

@@ -45,7 +45,7 @@ namespace NonLocalizable
 struct FancyZones : public winrt::implements<FancyZones, IFancyZones, IFancyZonesCallback, IZoneWindowHost>
 {
 public:
-    FancyZones(HINSTANCE hinstance, const winrt::com_ptr<IFancyZonesSettings>& settings) noexcept :
+    FancyZones(HINSTANCE hinstance, const winrt::com_ptr<IFancyZonesSettings>& settings, std::function<void()> 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<void()> 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<void()> 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<std::pair<HMONITOR, RECT>> FancyZones::GetRawMonitorData() noexcept
     return monitorInfo;
 }
 
-winrt::com_ptr<IFancyZones> MakeFancyZones(HINSTANCE hinstance, const winrt::com_ptr<IFancyZonesSettings>& settings) noexcept
+winrt::com_ptr<IFancyZones> MakeFancyZones(HINSTANCE hinstance,
+                                           const winrt::com_ptr<IFancyZonesSettings>& settings,
+                                           std::function<void()> disableCallback) noexcept
 {
     if (!settings)
     {
         return nullptr;
     }
 
-    return winrt::make_self<FancyZones>(hinstance, settings);
+    return winrt::make_self<FancyZones>(hinstance, settings, disableCallback);
 }

src/modules/fancyzones/lib/FancyZones.h

@@ -2,6 +2,8 @@
 
 #include <common/WinHookEvent.h>
 
+#include <functional>
+
 interface IZoneWindow;
 interface IFancyZonesSettings;
 interface IZoneSet;
@@ -102,4 +104,4 @@ interface __declspec(uuid("{5C8D99D6-34B2-4F4A-A8E5-7483F6869775}")) IZoneWindow
     () = 0;
 };
 
-winrt::com_ptr<IFancyZones> MakeFancyZones(HINSTANCE hinstance, const winrt::com_ptr<IFancyZonesSettings>& settings) noexcept;
+winrt::com_ptr<IFancyZones> MakeFancyZones(HINSTANCE hinstance, const winrt::com_ptr<IFancyZonesSettings>& settings, std::function<void()> disableCallback) noexcept;

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);
 

src/modules/fancyzones/lib/Resources.resx

@@ -205,10 +205,10 @@
     <value>Don't show again</value>
   </data>
   <data name="Keyboard_Listener_Error" xml:space="preserve">
-    <value>Cannot install keyboard listener.</value>
+    <value>Failed to install keyboard listener.</value>
   </data>
   <data name="Window_Event_Listener_Error" xml:space="preserve">
-    <value>Cannot install Windows event listener.</value>
+    <value>Failed to install Windows event listener.</value>
   </data>
   <data name="Powertoys_FancyZones" xml:space="preserve">
     <value>PowerToys - FancyZones</value>
@@ -216,4 +216,16 @@
   <data name="Span_Across_Zones_Warning" xml:space="preserve">
     <value>The 'Allow zones to span across monitors' option requires all monitors to have the same DPI scaling to work properly.</value>
   </data>
+  <data name="FancyZones_Data_Error" xml:space="preserve">
+    <value>FancyZones persisted data path not found. Please report the bug to</value>
+  </data>
+  <data name="FancyZones_Editor_Launch_Error" xml:space="preserve">
+    <value>The FancyZones editor failed to start. Please report the bug to</value>
+  </data>
+  <data name="FancyZones_Settings_Load_Error" xml:space="preserve">
+    <value>Failed to load the FancyZones settings. Default settings will be used.</value>
+  </data>
+  <data name="FancyZones_Settings_Save_Error" xml:space="preserve">
+    <value>Failed to save the FancyZones settings. Please retry again later, if the problem persists report the bug to</value>
+  </data>
 </root>

src/modules/fancyzones/lib/Settings.cpp

@@ -1,9 +1,12 @@
 #include "pch.h"
 #include <common/settings_objects.h>
+#include <common/common.h>
 #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<FancyZonesSettings, IFancyZonesSettings>
@@ -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.
+    try
-    PowerToysSettings::CustomActionObject action_object =
-        PowerToysSettings::CustomActionObject::from_json_string(action);
-
-    if (m_callback && action_object.get_name() == NonLocalizable::ToggleEditorActionID)
     {
-        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 ?
+    try
-        PowerToysSettings::PowerToyValues::load_from_settings_file(m_moduleName) :
-        PowerToysSettings::PowerToyValues::from_json_string(config);
-
-    for (auto const& setting : m_configBools)
     {
-        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))
+        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'))
         {
-            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.zoneBorderColor = std::move(*val);
-            m_settings.excludedAppsArray.emplace_back(view.substr(0, pos));
+        }
-            view.remove_prefix(pos);
+
+        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;
         }
     }
-
+    catch (...)
-    if (auto val = values.get_int_value(NonLocalizable::ZoneHighlightOpacityID))
     {
-        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);
+    try
-
-    for (auto const& setting : m_configBools)
     {
-        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<IFancyZonesSettings> MakeFancyZonesSettings(HINSTANCE hinstance, PCWSTR name) noexcept
 {

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<IZoneWindowHost>();
@@ -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<IFancyZonesCallback>();

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

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);
 }