feat(settings_revert): use full extended topology as a fallback (#133)

Author: FrogTheFrog

.github/workflows/ci.yml

@@ -182,7 +182,7 @@ jobs:
             # Doxygen from Ubuntu is too old, need Doxygen >= 1.10
             DOCS=OFF
           else
-            DOCS=ON
+            DOCS=OFF
           fi
 
           cmake \

src/common/include/display_device/settings_manager_interface.h

@@ -26,6 +26,20 @@ namespace display_device {
       PersistenceSaveFailed,  ///< Persistence save failed
     };
 
+    /**
+     * @brief Outcome values when trying to revert settings.
+     */
+    enum class RevertResult {
+      Ok,  ///< Settings were reverted successfully
+      ApiTemporarilyUnavailable,  ///< API is temporarily unavailable
+      TopologyIsInvalid,  ///< Topology is invalid
+      SwitchingTopologyFailed,  ///< Switching topology has failed
+      RevertingPrimaryDeviceFailed,  ///< Reverting primary device failed
+      RevertingDisplayModesFailed,  ///< Reverting display modes failed
+      RevertingHdrStatesFailed,  ///< Reverting HDR states failed
+      PersistenceSaveFailed,  ///< Persistence save failed
+    };
+
     /**
      * @brief Default virtual destructor.
      */
@@ -79,7 +93,7 @@ namespace display_device {
      * const auto result = iface->revertSettings();
      * @examples_end
      */
-    [[nodiscard]] virtual bool
+    [[nodiscard]] virtual RevertResult
     revertSettings() = 0;
 
     /**
@@ -94,11 +108,11 @@ namespace display_device {
      * auto result = iface->applySettings(config);
      * if (result == ApplyResult::Ok) {
      *   // Wait for some time
-     *   if (!iface->revertSettings()) {
+     *   if (iface->revertSettings() != RevertResult::Ok) {
      *     // Wait for user input
      *     const bool user_wants_reset { true };
      *     if (user_wants_reset) {
-     *       result = iface->resetPersistence();
+     *       iface->resetPersistence();
      *     }
      *   }
      * }

src/windows/include/display_device/windows/settings_manager.h

@@ -45,7 +45,7 @@ namespace display_device {
     applySettings(const SingleDisplayConfiguration &config) override;
 
     /** For details @see SettingsManagerInterface::revertSettings */
-    [[nodiscard]] bool
+    [[nodiscard]] RevertResult
     revertSettings() override;
 
     /** For details @see SettingsManagerInterface::resetPersistence */
@@ -107,11 +107,11 @@ namespace display_device {
      * @param current_topology Topology before this method is called.
      * @param system_settings_touched Indicates whether a "write" operation could have been performed on the OS.
      * @param switched_topology [Optional] Indicates whether the current topology was switched to revert settings.
-     * @returns True on success, false otherwise.
+     * @returns Result enum indicating success or failure.
      * @warning The method assumes that the caller will ensure restoring the topology
      *          in case of a failure!
      */
-    [[nodiscard]] bool
+    [[nodiscard]] RevertResult
     revertModifiedSettings(const ActiveTopology &current_topology, bool &system_settings_touched, bool *switched_topology = nullptr);
 
     std::shared_ptr<WinDisplayDeviceInterface> m_dd_api;

src/windows/include/display_device/windows/settings_utils.h

@@ -17,7 +17,7 @@
  */
 namespace display_device::win_utils {
   /**
-   * @brief Get all the device its in the topology.
+   * @brief Get all the device ids in the topology.
    * @param topology Topology to be "flattened".
    * @return Device ids found in the topology.
    * @examples
@@ -29,18 +29,16 @@ namespace display_device::win_utils {
   flattenTopology(const ActiveTopology &topology);
 
   /**
-   * @brief Remove all unavailable devices from the topology.
+   * @brief Create extended topology from all the available devices.
    * @param win_dd Interface for interacting with the OS.
-   * @param topology Topology to be stripped.
-   * @return Topology with only available devices.
+   * @return Extended topology with all the available devices.
    * @examples
    * const WinDisplayDeviceInterface* iface = getIface(...);
-   * const ActiveTopology topology { { "DeviceId1", "DeviceId2" }, { "DeviceId3" } };
-   * const auto stripped_topology { stripTopologyOfUnavailableDevices(*iface, topology) };
+   * const auto extended_topology { stripTopologyOfUnavailableDevices(*iface) };
    * @examples_end
    */
   ActiveTopology
-  stripTopologyOfUnavailableDevices(WinDisplayDeviceInterface &win_dd, const ActiveTopology &topology);
+  createFullExtendedTopology(WinDisplayDeviceInterface &win_dd);
 
   /**
    * @brief Get one primary device from the provided topology.
@@ -86,7 +84,7 @@ namespace display_device::win_utils {
 
   /**
    * @brief Compute new topology from arbitrary data.
-   * @param device_prep Specify how to to compute the new topology.
+   * @param device_prep Specify how to compute the new topology.
    * @param configuring_primary_devices Specify whether the `device_to_configure` was unspecified (primary device was selected).
    * @param device_to_configure Main device to be configured.
    * @param additional_devices_to_configure Additional devices that belong to the same group as `device_to_configure`.

src/windows/settings_manager_apply.cpp

@@ -172,7 +172,7 @@ namespace display_device {
     if (change_is_needed) {
       if (cached_state && !m_dd_api->isTopologyTheSame(cached_state->m_modified.m_topology, new_topology)) {
         DD_LOG(warning) << "To apply new display device settings, previous modifications must be undone! Trying to undo them now.";
-        if (!revertModifiedSettings(topology_before_changes, system_settings_touched)) {
+        if (revertModifiedSettings(topology_before_changes, system_settings_touched) != RevertResult::Ok) {
           DD_LOG(error) << "Failed to apply new configuration, because the previous settings could not be reverted!";
           return std::nullopt;
         }

src/windows/settings_manager_general.cpp

@@ -49,7 +49,7 @@ namespace display_device {
   bool
   SettingsManager::resetPersistence() {
     // Trying to revert one more time in case we succeed.
-    if (revertSettings()) {
+    if (revertSettings() == RevertResult::Ok) {
       return true;
     }
 

src/windows/settings_manager_revert.cpp

@@ -1,6 +1,6 @@
 /**
  * @file src/windows/settings_manager_revert.cpp
- * @brief Definitions for the methods for reverting settings in SettingsManager..
+ * @brief Definitions for the methods for reverting settings in SettingsManager.
  */
 // class header include
 #include "display_device/windows/settings_manager.h"
@@ -23,25 +23,25 @@ namespace display_device {
     }
   }  // namespace
 
-  bool
+  SettingsManager::RevertResult
   SettingsManager::revertSettings() {
     const auto &cached_state { m_persistence_state->getState() };
     if (!cached_state) {
-      return true;
+      return RevertResult::Ok;
     }
 
     const auto api_access { m_dd_api->isApiAccessAvailable() };
     DD_LOG(info) << "Trying to revert applied display device settings. API is available: " << toJson(api_access);
 
     if (!api_access) {
-      return false;
+      return RevertResult::ApiTemporarilyUnavailable;
     }
 
     const auto current_topology { m_dd_api->getCurrentTopology() };
     if (!m_dd_api->isTopologyValid(current_topology)) {
       DD_LOG(error) << "Retrieved current topology is invalid:\n"
                     << toJson(current_topology);
-      return false;
+      return RevertResult::TopologyIsInvalid;
     }
 
     bool system_settings_touched { false };
@@ -50,8 +50,8 @@ namespace display_device {
         win_utils::blankHdrStates(*m_dd_api, m_workarounds.m_hdr_blank_delay);
       }
     } };
-    boost::scope::scope_exit topology_prep_guard { [this, &cached_state, &current_topology, &system_settings_touched]() {
-      auto topology_to_restore { win_utils::stripTopologyOfUnavailableDevices(*m_dd_api, cached_state->m_initial.m_topology) };
+    boost::scope::scope_exit topology_prep_guard { [this, &current_topology, &system_settings_touched]() {
+      auto topology_to_restore { win_utils::createFullExtendedTopology(*m_dd_api) };
       if (!m_dd_api->isTopologyValid(topology_to_restore)) {
         topology_to_restore = current_topology;
       }
@@ -66,15 +66,15 @@ namespace display_device {
 
     // We can revert the modified setting independently before playing around with initial topology.
     bool switched_to_modified_topology { false };
-    if (!revertModifiedSettings(current_topology, system_settings_touched, &switched_to_modified_topology)) {
+    if (const auto result = revertModifiedSettings(current_topology, system_settings_touched, &switched_to_modified_topology); result != RevertResult::Ok) {
       // Error already logged
-      return false;
+      return result;
     }
 
     if (!m_dd_api->isTopologyValid(cached_state->m_initial.m_topology)) {
       DD_LOG(error) << "Trying to revert to an invalid initial topology:\n"
                     << toJson(cached_state->m_initial.m_topology);
-      return false;
+      return RevertResult::TopologyIsInvalid;
     }
 
     const bool is_topology_the_same { m_dd_api->isTopologyTheSame(current_topology, cached_state->m_initial.m_topology) };
@@ -83,12 +83,12 @@ namespace display_device {
     if (need_to_switch_topology && !m_dd_api->setTopology(cached_state->m_initial.m_topology)) {
       DD_LOG(error) << "Failed to change topology to:\n"
                     << toJson(cached_state->m_initial.m_topology);
-      return false;
+      return RevertResult::SwitchingTopologyFailed;
     }
 
     if (!m_persistence_state->persistState(std::nullopt)) {
       DD_LOG(error) << "Failed to save reverted settings! Undoing initial topology changes...";
-      return false;
+      return RevertResult::PersistenceSaveFailed;
     }
 
     if (m_audio_context_api->isCaptured()) {
@@ -97,28 +97,28 @@ namespace display_device {
 
     // Disable guards
     topology_prep_guard.set_active(false);
-    return true;
+    return RevertResult::Ok;
   }
 
-  bool
+  SettingsManager::RevertResult
   SettingsManager::revertModifiedSettings(const ActiveTopology &current_topology, bool &system_settings_touched, bool *switched_topology) {
     const auto &cached_state { m_persistence_state->getState() };
     if (!cached_state || !cached_state->m_modified.hasModifications()) {
-      return true;
+      return RevertResult::Ok;
     }
 
     if (!m_dd_api->isTopologyValid(cached_state->m_modified.m_topology)) {
       DD_LOG(error) << "Trying to revert modified settings using invalid topology:\n"
                     << toJson(cached_state->m_modified.m_topology);
-      return false;
+      return RevertResult::TopologyIsInvalid;
     }
 
     const bool is_topology_the_same { m_dd_api->isTopologyTheSame(current_topology, cached_state->m_modified.m_topology) };
     system_settings_touched = !is_topology_the_same;
     if (!is_topology_the_same && !m_dd_api->setTopology(cached_state->m_modified.m_topology)) {
       DD_LOG(error) << "Failed to change topology to:\n"
                     << toJson(cached_state->m_modified.m_topology);
-      return false;
+      return RevertResult::SwitchingTopologyFailed;
     }
     if (switched_topology) {
       *switched_topology = !is_topology_the_same;
@@ -135,7 +135,7 @@ namespace display_device {
                      << toJson(cached_state->m_modified.m_original_hdr_states);
         if (!m_dd_api->setHdrStates(cached_state->m_modified.m_original_hdr_states)) {
           // Error already logged
-          return false;
+          return RevertResult::RevertingHdrStatesFailed;
         }
 
         hdr_guard_fn = win_utils::hdrStateGuardFn(*m_dd_api, current_states);
@@ -152,7 +152,7 @@ namespace display_device {
         if (!m_dd_api->setDisplayModes(cached_state->m_modified.m_original_modes)) {
           system_settings_touched = true;
           // Error already logged
-          return false;
+          return RevertResult::RevertingDisplayModesFailed;
         }
 
         // It is possible that the display modes will not actually change even though the "current != new" condition is true.
@@ -175,7 +175,7 @@ namespace display_device {
         DD_LOG(info) << "Trying to change back the original primary device to: " << toJson(cached_state->m_modified.m_original_primary_device);
         if (!m_dd_api->setAsPrimary(cached_state->m_modified.m_original_primary_device)) {
           // Error already logged
-          return false;
+          return RevertResult::RevertingPrimaryDeviceFailed;
         }
 
         primary_guard_fn = win_utils::primaryGuardFn(*m_dd_api, current_primary_device);
@@ -186,13 +186,13 @@ namespace display_device {
     cleared_data.m_modified = { cleared_data.m_modified.m_topology };
     if (!m_persistence_state->persistState(cleared_data)) {
       DD_LOG(error) << "Failed to save reverted settings! Undoing changes to modified topology...";
-      return false;
+      return RevertResult::PersistenceSaveFailed;
     }
 
     // Disable guards
     hdr_guard.set_active(false);
     mode_guard.set_active(false);
     primary_guard.set_active(false);
-    return true;
+    return RevertResult::Ok;
   }
 }  // namespace display_device

src/windows/settings_utils.cpp

@@ -147,14 +147,19 @@ namespace display_device::win_utils {
   }
 
   ActiveTopology
-  stripTopologyOfUnavailableDevices(WinDisplayDeviceInterface &win_dd, const ActiveTopology &topology) {
+  createFullExtendedTopology(WinDisplayDeviceInterface &win_dd) {
     const auto devices { win_dd.enumAvailableDevices() };
     if (devices.empty()) {
-      DD_LOG(error) << "Failed to enumerate available devices for stripping topology!";
+      DD_LOG(error) << "Failed to enumerate available devices for full extended topology!";
       return {};
     }
 
-    return stripTopology(topology, devices);
+    ActiveTopology topology;
+    for (const auto &device : devices) {
+      topology.push_back({ device.m_device_id });
+    }
+
+    return topology;
   }
 
   std::string

src/windows/win_api_layer.cpp

@@ -612,12 +612,12 @@ namespace display_device {
       return TRUE; }, reinterpret_cast<LPARAM>(&enum_data));
 
     if (!enum_data.m_width) {
-      DD_LOG(error) << "Failed to get monitor info for " << display_name << "!";
+      DD_LOG(debug) << "Failed to get monitor info for " << display_name << "!";
       return std::nullopt;
     }
 
     if (*enum_data.m_width * source_mode.width == 0) {
-      DD_LOG(error) << "Cannot get display scale for " << display_name << " from a width of 0!";
+      DD_LOG(debug) << "Cannot get display scale for " << display_name << " from a width of 0!";
       return std::nullopt;
     }