[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
microsoft · Sep 18, 2020 · 3aa7a52 · 3aa7a52
1 parent 2bc3480
commit 3aa7a52
Show file tree

Hide file tree

Showing 9 changed files with 148 additions and 81 deletions.
diff --git a/src/modules/fancyzones/dll/dllmain.cpp b/src/modules/fancyzones/dll/dllmain.cpp
@@ -71,7 +71,7 @@ class FancyZonesModule : public PowertoyModuleIface
         {
             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 @@ class FancyZonesModule : public PowertoyModuleIface
     // 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
@@ -45,14 +45,16 @@ 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]() {
             PostMessageW(m_window, WM_PRIV_LOCATIONCHANGE, NULL, NULL);
         })
     {
         m_settings->SetCallback(this);
+
+        this->disableModuleCallback = std::move(disableModuleCallback);
     }
 
     // IFancyZones
@@ -240,6 +242,9 @@ struct FancyZones : public winrt::implements<FancyZones, IFancyZones, IFancyZone
     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 @@ struct FancyZones : public winrt::implements<FancyZones, IFancyZones, IFancyZone
     };
 };
 
+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);
 }
diff --git a/src/modules/fancyzones/lib/FancyZones.h b/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;
diff --git 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
@@ -205,15 +205,27 @@
     <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>
   </data>
   <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>
diff --git a/src/modules/fancyzones/lib/Settings.cpp b/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.
-    PowerToysSettings::CustomActionObject action_object =
-        PowerToysSettings::CustomActionObject::from_json_string(action);
+    try
+    {
+        // 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)
+        if (m_callback && action_object.get_name() == NonLocalizable::ToggleEditorActionID)
+        {
+            m_callback->ToggleEditor();
+        }
+    }
+    catch (...)
     {
-        m_callback->ToggleEditor();
+        // 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::ZoneColorID))
+        {
+            m_settings.zoneColor = std::move(*val);
+        }
 
-    if (auto val = values.get_string_value(NonLocalizable::ZoneHighlightColorID))
-    {
-        m_settings.zoneHighlightColor = std::move(*val);
-    }
+        if (auto val = values.get_string_value(NonLocalizable::ZoneBorderColorID))
+        {
+            m_settings.zoneBorderColor = 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::ZoneHighlightColorID))
+        {
+            m_settings.zoneHighlightColor = std::move(*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 (const auto val = values.get_json(NonLocalizable::EditorHotkeyID))
         {
-            view.remove_prefix(1);
+            m_settings.editorHotkey = PowerToysSettings::HotkeyObject::from_json(*val);
         }
-        while (!view.empty())
+
+        if (auto val = values.get_string_value(NonLocalizable::ExcludedAppsID))
         {
-            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.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))
+        if (auto val = values.get_int_value(NonLocalizable::ZoneHighlightOpacityID))
+        {
+            m_settings.zoneHighlightOpacity = *val;
+        }
+    }
+    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.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();
+        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);
+    }
 }
-CATCH_LOG();
 
 winrt::com_ptr<IFancyZonesSettings> MakeFancyZonesSettings(HINSTANCE hinstance, PCWSTR name) noexcept
 {