From d702204c07a4fe1dc610f848fe125db8cc9e465e Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Sat, 5 Sep 2026 19:26:37 -0500 Subject: [PATCH 01/12] fix(windows): recover displays after undocking during a session --- src/common/include/display_device/types.h | 1 + src/common/json_serializer.cpp | 19 +- .../display_device/windows/settings_manager.h | 7 + .../windows/win_display_device.h | 4 +- src/windows/settings_manager_revert.cpp | 79 ++++++- src/windows/win_display_device_general.cpp | 11 +- src/windows/win_display_device_modes.cpp | 10 +- src/windows/win_display_device_primary.cpp | 2 +- src/windows/win_display_device_topology.cpp | 18 +- tests/unit/general/test_json_converter.cpp | 20 +- .../windows/test_settings_manager_revert.cpp | 1 + .../windows/test_settings_manager_undock.cpp | 203 ++++++++++++++++++ .../test_win_display_device_general.cpp | 9 +- .../windows/test_win_display_device_modes.cpp | 124 +++++++++++ .../test_win_display_device_primary.cpp | 32 +++ .../test_win_display_device_topology.cpp | 81 +++++++ 16 files changed, 597 insertions(+), 24 deletions(-) create mode 100644 tests/unit/windows/test_settings_manager_undock.cpp diff --git a/src/common/include/display_device/types.h b/src/common/include/display_device/types.h index dd49d4a0..0d411e30 100644 --- a/src/common/include/display_device/types.h +++ b/src/common/include/display_device/types.h @@ -195,6 +195,7 @@ namespace display_device { std::string m_friendly_name {}; ///< A human-readable name for the device. std::optional m_edid {}; ///< Some basic parsed EDID data. std::optional m_info {}; ///< Additional information about an active display device. + bool m_is_internal {}; ///< Positively identified built-in panel; false also covers unknown connectors. /** * @brief Comparator for strict equality. diff --git a/src/common/json_serializer.cpp b/src/common/json_serializer.cpp index ff56800a..877ddf96 100644 --- a/src/common/json_serializer.cpp +++ b/src/common/json_serializer.cpp @@ -20,6 +20,23 @@ namespace display_device { DD_JSON_DEFINE_SERIALIZE_STRUCT(Point, x, y) DD_JSON_DEFINE_SERIALIZE_STRUCT(EdidData, manufacturer_id, product_code, serial_number) DD_JSON_DEFINE_SERIALIZE_STRUCT(EnumeratedDevice::Info, resolution, resolution_scale, refresh_rate, primary, origin_point, hdr_state) - DD_JSON_DEFINE_SERIALIZE_STRUCT(EnumeratedDevice, device_id, display_name, friendly_name, edid, info) + + void to_json(nlohmann::json &nlohmann_json_j, const EnumeratedDevice &nlohmann_json_t) { + DD_JSON_TO(device_id) + DD_JSON_TO(display_name) + DD_JSON_TO(friendly_name) + DD_JSON_TO(edid) + DD_JSON_TO(info) + DD_JSON_TO(is_internal) + } + + void from_json(const nlohmann::json &nlohmann_json_j, EnumeratedDevice &nlohmann_json_t) { + DD_JSON_FROM(device_id) + DD_JSON_FROM(display_name) + DD_JSON_FROM(friendly_name) + DD_JSON_FROM(edid) + DD_JSON_FROM(info) + nlohmann_json_t.m_is_internal = nlohmann_json_j.value("is_internal", false); + } DD_JSON_DEFINE_SERIALIZE_STRUCT(SingleDisplayConfiguration, device_id, device_prep, resolution, refresh_rate, hdr_state) } // namespace display_device diff --git a/src/windows/include/display_device/windows/settings_manager.h b/src/windows/include/display_device/windows/settings_manager.h index 3c6971cd..11cee6da 100644 --- a/src/windows/include/display_device/windows/settings_manager.h +++ b/src/windows/include/display_device/windows/settings_manager.h @@ -148,6 +148,13 @@ namespace display_device { */ [[nodiscard]] RevertResult revertModifiedPrimaryDevice(const SingleDisplayConfigState::Modified &modified_state, DdGuardFn &guard_fn, bool &system_settings_touched); + /** + * @brief Recover to surviving original displays or a built-in panel after an undock. + * Only devices activated by this session may be removed. The original recovery + * record remains intact on failure; a visible replacement is verified first. + */ + [[nodiscard]] bool recoverMissingTopology(const ActiveTopology ¤t_topology); + private: std::shared_ptr m_dd_api; std::shared_ptr m_audio_context_api; diff --git a/src/windows/include/display_device/windows/win_display_device.h b/src/windows/include/display_device/windows/win_display_device.h index b12e76b7..7d37f06a 100644 --- a/src/windows/include/display_device/windows/win_display_device.h +++ b/src/windows/include/display_device/windows/win_display_device.h @@ -19,9 +19,10 @@ namespace display_device { public: /** * Default constructor for the class. + * @param save_to_database Whether CCD changes update Windows' saved display layout. * @param w_api A pointer to the Windows API layer. Will throw on nullptr! */ - explicit WinDisplayDevice(std::shared_ptr w_api); + explicit WinDisplayDevice(std::shared_ptr w_api, bool save_to_database = true); /** * @copydoc WinDisplayDeviceInterface::isApiAccessAvailable @@ -90,5 +91,6 @@ namespace display_device { private: std::shared_ptr m_w_api; + bool m_save_to_database; }; } // namespace display_device diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index e9acea0e..8d0ef499 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -6,6 +6,7 @@ #include "display_device/windows/settings_manager.h" // system includes +#include #include // local includes @@ -82,7 +83,9 @@ 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 RevertResult::SwitchingTopologyFailed; + if (!recoverMissingTopology(current_topology)) { + return RevertResult::SwitchingTopologyFailed; + } } if (!m_persistence_state->persistState(std::nullopt)) { @@ -99,6 +102,80 @@ namespace display_device { return RevertResult::Ok; } + bool SettingsManager::recoverMissingTopology(const ActiveTopology ¤t_topology) { + const auto &state {*m_persistence_state->getState()}; + const auto initial {win_utils::flattenTopology(state.m_initial.m_topology)}; + const auto modified {win_utils::flattenTopology(state.m_modified.m_topology)}; + const auto devices {m_dd_api->enumAvailableDevices()}; + StringSet available; + for (const auto &device : devices) { + available.insert(device.m_device_id); + } + if (available.empty() || std::ranges::all_of(initial, [&](const auto &id) { + return available.contains(id); + })) { + // An API failure with unchanged hardware must not discard the original state. + return false; + } + + StringSet activated; + std::ranges::set_difference(modified, initial, std::inserter(activated, activated.end())); + ActiveTopology target; + StringSet included; + const auto append = [&](const ActiveTopology &topology) { + for (const auto &group : topology) { + std::vector remaining; + for (const auto &id : group) { + if (available.contains(id) && !activated.contains(id) && included.insert(id).second) { + remaining.push_back(id); + } + } + if (!remaining.empty()) { + target.push_back(std::move(remaining)); + } + } + }; + append(state.m_initial.m_topology); + if (target.empty()) { + // An arbitrary virtual/unknown output is not proof of a usable laptop screen. + for (const auto &device : devices) { + if (device.m_is_internal && !activated.contains(device.m_device_id)) { + append({{device.m_device_id}}); + break; + } + } + } + if (target.empty()) { + return false; // Closed lid and no original display: retain pending recovery. + } + const auto replacements {win_utils::flattenTopology(target)}; + append(current_topology); // Preserve unrelated displays activated by the user. + + // First activate the replacement without switching off the current output. + ActiveTopology staged {current_topology}; + auto staged_ids {win_utils::flattenTopology(staged)}; + for (const auto &id : included) { + if (staged_ids.insert(id).second) { + staged.push_back({id}); + } + } + if (!m_dd_api->setTopology(staged)) { + return false; + } + const auto active {win_utils::flattenTopology(m_dd_api->getCurrentTopology())}; + if (!std::ranges::all_of(replacements, [&](const auto &id) { + return active.contains(id); + })) { + return false; + } + if (!m_dd_api->setTopology(target) || !m_dd_api->isTopologyTheSame(m_dd_api->getCurrentTopology(), target)) { + return false; + } + DD_LOG(info) << "Recovered display topology after original devices disappeared:\n" + << toJson(target); + return true; + } + SettingsManager::RevertResult SettingsManager::revertModifiedHdrStates(const SingleDisplayConfigState::Modified &modified_state, DdGuardFn &guard_fn, bool &system_settings_touched) { using enum RevertResult; diff --git a/src/windows/win_display_device_general.cpp b/src/windows/win_display_device_general.cpp index e75ad59a..afa99843 100644 --- a/src/windows/win_display_device_general.cpp +++ b/src/windows/win_display_device_general.cpp @@ -13,8 +13,9 @@ #include "display_device/windows/win_api_utils.h" namespace display_device { - WinDisplayDevice::WinDisplayDevice(std::shared_ptr w_api): - m_w_api {std::move(w_api)} { + WinDisplayDevice::WinDisplayDevice(std::shared_ptr w_api, bool save_to_database): + m_w_api {std::move(w_api)}, + m_save_to_database {save_to_database} { if (!m_w_api) { throw std::invalid_argument {"Nullptr provided for WinApiLayerInterface in WinDisplayDevice!"}; } @@ -53,6 +54,8 @@ namespace display_device { const auto source_mode {is_active ? win_utils::getSourceMode(win_utils::getSourceIndex(best_path, display_data->m_modes), display_data->m_modes) : nullptr}; const auto display_name {is_active ? m_w_api->getDisplayName(best_path) : std::string {}}; // Inactive devices can have multiple display names, so it's just meaningless use any const auto edid {EdidData::parse(m_w_api->getEdid(best_path))}; + const auto technology {best_path.targetInfo.outputTechnology}; + const bool is_internal {technology == DISPLAYCONFIG_OUTPUT_TECHNOLOGY_INTERNAL || technology == DISPLAYCONFIG_OUTPUT_TECHNOLOGY_LVDS || technology == DISPLAYCONFIG_OUTPUT_TECHNOLOGY_DISPLAYPORT_EMBEDDED || technology == DISPLAYCONFIG_OUTPUT_TECHNOLOGY_UDI_EMBEDDED}; if (is_active && !source_mode) { DD_LOG(warning) << "Device " << device_id << " is missing source mode!"; @@ -70,10 +73,10 @@ namespace display_device { }; // Keep braced aggregate construction; emplace_back(args...) relies on parenthesized aggregate init, which older libc++ rejects. - available_devices.push_back(EnumeratedDevice {device_id, display_name, friendly_name, edid, info}); // NOSONAR(cpp:S6003): Direct emplace_back args fail on older toolchains + available_devices.push_back(EnumeratedDevice {device_id, display_name, friendly_name, edid, info, is_internal}); // NOSONAR(cpp:S6003): Direct emplace_back args fail on older toolchains } else { // Keep braced aggregate construction; emplace_back(args...) relies on parenthesized aggregate init, which older libc++ rejects. - available_devices.push_back(EnumeratedDevice {device_id, display_name, friendly_name, edid, std::nullopt}); // NOSONAR(cpp:S6003): Direct emplace_back args fail on older toolchains + available_devices.push_back(EnumeratedDevice {device_id, display_name, friendly_name, edid, std::nullopt, is_internal}); // NOSONAR(cpp:S6003): Direct emplace_back args fail on older toolchains } } diff --git a/src/windows/win_display_device_modes.cpp b/src/windows/win_display_device_modes.cpp index 9c71f583..ec9a7ad7 100644 --- a/src/windows/win_display_device_modes.cpp +++ b/src/windows/win_display_device_modes.cpp @@ -26,7 +26,7 @@ namespace display_device { /** * @see set_display_modes for a description as this was split off to reduce cognitive complexity. */ - bool doSetModes(WinApiLayerInterface &w_api, const DeviceDisplayModeMap &modes, const Strategy strategy) { + bool doSetModes(WinApiLayerInterface &w_api, const DeviceDisplayModeMap &modes, const Strategy strategy, const bool save_to_database) { auto display_data {w_api.queryDisplayConfig(QueryType::Active)}; if (!display_data) { // Error already logged @@ -85,7 +85,7 @@ namespace display_device { return true; } - UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_SAVE_TO_DATABASE | SDC_VIRTUAL_MODE_AWARE}; + UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | (save_to_database ? SDC_SAVE_TO_DATABASE : 0u) | SDC_VIRTUAL_MODE_AWARE}; if (strategy == Strategy::Relaxed) { // It's probably best for Windows to select the "best" display settings for us. However, in case we // have custom resolution set in nvidia control panel for example, this flag will prevent successfully applying @@ -182,7 +182,7 @@ namespace display_device { return false; } - if (!doSetModes(*m_w_api, modes, Strategy::Relaxed)) { + if (!doSetModes(*m_w_api, modes, Strategy::Relaxed, m_save_to_database)) { // Error already logged return false; } @@ -219,7 +219,7 @@ namespace display_device { // resolution to be selected, we actually need to omit SDC_ALLOW_CHANGES // flag. DD_LOG(info) << "Failed to change display modes using Windows recommended modes, trying to set modes more strictly!"; - if (doSetModes(*m_w_api, modes, Strategy::Strict)) { + if (doSetModes(*m_w_api, modes, Strategy::Strict, m_save_to_database)) { current_modes = getCurrentDisplayModes(device_ids); if (!current_modes.empty() && all_modes_match(current_modes)) { return true; @@ -227,7 +227,7 @@ namespace display_device { } } - const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_SAVE_TO_DATABASE | SDC_VIRTUAL_MODE_AWARE}; + const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | (m_save_to_database ? SDC_SAVE_TO_DATABASE : 0u) | SDC_VIRTUAL_MODE_AWARE}; static_cast(m_w_api->setDisplayConfig(original_data->m_paths, original_data->m_modes, flags)); // Return value does not matter as we are trying out best to undo DD_LOG(error) << "Failed to set display mode(-s) completely!"; return false; diff --git a/src/windows/win_display_device_primary.cpp b/src/windows/win_display_device_primary.cpp index 92ee9649..78bf874a 100644 --- a/src/windows/win_display_device_primary.cpp +++ b/src/windows/win_display_device_primary.cpp @@ -97,7 +97,7 @@ namespace display_device { modified_modes.insert(*source_index); } - const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_SAVE_TO_DATABASE | SDC_VIRTUAL_MODE_AWARE}; + const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | (m_save_to_database ? SDC_SAVE_TO_DATABASE : 0u) | SDC_VIRTUAL_MODE_AWARE}; if (const LONG result {m_w_api->setDisplayConfig(display_data->m_paths, display_data->m_modes, flags)}; result != ERROR_SUCCESS) { DD_LOG(error) << m_w_api->getErrorString(result) << " failed to set primary mode for " << device_id << "!"; return false; diff --git a/src/windows/win_display_device_topology.cpp b/src/windows/win_display_device_topology.cpp index 9d938f24..3b69caca 100644 --- a/src/windows/win_display_device_topology.cpp +++ b/src/windows/win_display_device_topology.cpp @@ -19,7 +19,7 @@ namespace display_device { /** * @see set_topology for a description as this was split off to reduce cognitive complexity. */ - bool doSetTopology(WinApiLayerInterface &w_api, const ActiveTopology &new_topology, const PathAndModeData &display_data) { + bool doSetTopology(WinApiLayerInterface &w_api, const ActiveTopology &new_topology, const PathAndModeData &display_data, const bool save_to_database) { const auto path_data {win_utils::collectSourceDataForMatchingPaths(w_api, display_data.m_paths)}; if (path_data.empty()) { // Error already logged @@ -32,6 +32,18 @@ namespace display_device { return false; } + if (!save_to_database) { + // TOPOLOGY_SUPPLIED also changes the remembered topology. Session-only + // layouts must use the explicitly temporary supplied-config API path. + const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + const LONG result {w_api.setDisplayConfig(paths, {}, flags)}; + if (result != ERROR_SUCCESS) { + DD_LOG(error) << w_api.getErrorString(result) << " failed to set temporary topology!"; + return false; + } + return true; + } + UINT32 flags {SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE}; if (LONG result {w_api.setDisplayConfig(paths, {}, flags)}; result == ERROR_GEN_FAILURE) { DD_LOG(warning) << w_api.getErrorString(result) << " failed to change topology using the topology from Windows DB! Asking Windows to create the topology."; @@ -158,7 +170,7 @@ namespace display_device { return false; } - if (doSetTopology(*m_w_api, new_topology, *original_data)) { + if (doSetTopology(*m_w_api, new_topology, *original_data, m_save_to_database)) { if (const auto updated_topology {getCurrentTopology()}; isTopologyValid(updated_topology)) { if (isTopologyTheSame(new_topology, updated_topology)) { return true; @@ -194,7 +206,7 @@ namespace display_device { } // Revert back to the original topology - const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_SAVE_TO_DATABASE | SDC_VIRTUAL_MODE_AWARE}; + const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | (m_save_to_database ? SDC_SAVE_TO_DATABASE : 0u) | SDC_VIRTUAL_MODE_AWARE}; static_cast(m_w_api->setDisplayConfig(original_data->m_paths, original_data->m_modes, flags)); // Return value does not matter as we are trying out best to undo } diff --git a/tests/unit/general/test_json_converter.cpp b/tests/unit/general/test_json_converter.cpp index 8c7d8dcf..48615664 100644 --- a/tests/unit/general/test_json_converter.cpp +++ b/tests/unit/general/test_json_converter.cpp @@ -58,9 +58,9 @@ TEST_F_S(EnumeratedDevice) { const auto item_1 {makeEnumeratedDeviceOne()}; const auto item_2 {makeEnumeratedDeviceTwo()}; - executeTestCase(display_device::EnumeratedDevice {}, R"({"device_id":"","display_name":"","edid":null,"friendly_name":"","info":null})"); - executeTestCase(item_1, R"({"device_id":"ID_1","display_name":"NAME_2","edid":null,"friendly_name":"FU_NAME_3","info":{"hdr_state":"Enabled","origin_point":{"x":1,"y":2},"primary":false,"refresh_rate":{"type":"double","value":119.9554},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"rational","value":{"denominator":100,"numerator":175}}}})"); - executeTestCase(item_2, R"({"device_id":"ID_2","display_name":"NAME_2","edid":{"manufacturer_id":"","product_code":"","serial_number":0},"friendly_name":"FU_NAME_2","info":{"hdr_state":"Disabled","origin_point":{"x":0,"y":0},"primary":true,"refresh_rate":{"type":"rational","value":{"denominator":10000,"numerator":1199554}},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"double","value":1.75}}})"); + executeTestCase(display_device::EnumeratedDevice {}, R"({"device_id":"","display_name":"","edid":null,"friendly_name":"","info":null,"is_internal":false})"); + executeTestCase(item_1, R"({"device_id":"ID_1","display_name":"NAME_2","edid":null,"friendly_name":"FU_NAME_3","info":{"hdr_state":"Enabled","origin_point":{"x":1,"y":2},"primary":false,"refresh_rate":{"type":"double","value":119.9554},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"rational","value":{"denominator":100,"numerator":175}}},"is_internal":false})"); + executeTestCase(item_2, R"({"device_id":"ID_2","display_name":"NAME_2","edid":{"manufacturer_id":"","product_code":"","serial_number":0},"friendly_name":"FU_NAME_2","info":{"hdr_state":"Disabled","origin_point":{"x":0,"y":0},"primary":true,"refresh_rate":{"type":"rational","value":{"denominator":10000,"numerator":1199554}},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"double","value":1.75}},"is_internal":false})"); executeInvalidJsonTestCase(); executeFromJsonFailureTestCase(R"({})"); executeFromJsonFailureTestCase(R"({"device_id":"","display_name":"","edid":null,"friendly_name":"","info":{"hdr_state":null,"origin_point":{"x":0,"y":0},"primary":false,"refresh_rate":{"type":"unknown","value":0},"resolution":{"height":0,"width":0},"resolution_scale":{"type":"double","value":1.0}}})"); @@ -73,9 +73,9 @@ TEST_F_S(EnumeratedDeviceList) { display_device::EnumeratedDevice item_3 {}; executeTestCase(display_device::EnumeratedDeviceList {}, R"([])"); - executeTestCase(display_device::EnumeratedDeviceList {item_1, item_2, item_3}, R"([{"device_id":"ID_1","display_name":"NAME_2","edid":null,"friendly_name":"FU_NAME_3","info":{"hdr_state":"Enabled","origin_point":{"x":1,"y":2},"primary":false,"refresh_rate":{"type":"double","value":119.9554},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"rational","value":{"denominator":100,"numerator":175}}}},)" - R"({"device_id":"ID_2","display_name":"NAME_2","edid":{"manufacturer_id":"","product_code":"","serial_number":0},"friendly_name":"FU_NAME_2","info":{"hdr_state":"Disabled","origin_point":{"x":0,"y":0},"primary":true,"refresh_rate":{"type":"rational","value":{"denominator":10000,"numerator":1199554}},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"double","value":1.75}}},)" - R"({"device_id":"","display_name":"","edid":null,"friendly_name":"","info":null}])"); + executeTestCase(display_device::EnumeratedDeviceList {item_1, item_2, item_3}, R"([{"device_id":"ID_1","display_name":"NAME_2","edid":null,"friendly_name":"FU_NAME_3","info":{"hdr_state":"Enabled","origin_point":{"x":1,"y":2},"primary":false,"refresh_rate":{"type":"double","value":119.9554},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"rational","value":{"denominator":100,"numerator":175}}},"is_internal":false},)" + R"({"device_id":"ID_2","display_name":"NAME_2","edid":{"manufacturer_id":"","product_code":"","serial_number":0},"friendly_name":"FU_NAME_2","info":{"hdr_state":"Disabled","origin_point":{"x":0,"y":0},"primary":true,"refresh_rate":{"type":"rational","value":{"denominator":10000,"numerator":1199554}},"resolution":{"height":1080,"width":1920},"resolution_scale":{"type":"double","value":1.75}},"is_internal":false},)" + R"({"device_id":"","display_name":"","edid":null,"friendly_name":"","info":null,"is_internal":false}])"); executeInvalidJsonTestCase(); executeFromJsonFailureTestCase(R"([{}])"); executeToJsonFailureTestCase(display_device::EnumeratedDeviceList {display_device::EnumeratedDevice {.m_device_id = "ID\xC2"}}); @@ -125,3 +125,11 @@ TEST_F_S(Bool) { executeInvalidJsonTestCase(); executeFromJsonFailureTestCase(R"("not-bool")"); } + +TEST_F_S(EnumeratedDeviceInternalPanelMetadata) { + display_device::EnumeratedDevice device {.m_is_internal = true}; + EXPECT_TRUE(display_device::fromJson(R"({"device_id":"","display_name":"","edid":null,"friendly_name":"","info":null})", device)); + EXPECT_FALSE(device.m_is_internal); + device.m_is_internal = true; + executeTestCase(device, R"({"device_id":"","display_name":"","edid":null,"friendly_name":"","info":null,"is_internal":true})"); +} diff --git a/tests/unit/windows/test_settings_manager_revert.cpp b/tests/unit/windows/test_settings_manager_revert.cpp index 50e9cb18..095cf03e 100644 --- a/tests/unit/windows/test_settings_manager_revert.cpp +++ b/tests/unit/windows/test_settings_manager_revert.cpp @@ -529,6 +529,7 @@ TEST_F_S_MOCKED(FailedToSetInitialTopology) { .WillOnce(Return(false)) .RetiresOnSaturation(); + EXPECT_CALL(*m_dd_api, enumAvailableDevices()).WillOnce(Return(CURRENT_DEVICES)); expectedDefaultTopologyGuardCall(sequence); expectedHdrWorkaroundCalls(sequence); diff --git a/tests/unit/windows/test_settings_manager_undock.cpp b/tests/unit/windows/test_settings_manager_undock.cpp new file mode 100644 index 00000000..1e867a56 --- /dev/null +++ b/tests/unit/windows/test_settings_manager_undock.cpp @@ -0,0 +1,203 @@ +#include "display_device/windows/settings_manager.h" +#include "display_device/windows/settings_utils.h" +#include "fixtures/fixtures.h" +#include "fixtures/mock_audio_context.h" +#include "fixtures/mock_settings_persistence.h" +#include "utils/helpers.h" +#include "utils/mock_win_display_device.h" + +#include + +namespace { + using namespace display_device; + using ::testing::_; + using ::testing::NiceMock; + using ::testing::Return; + + class UndockRecovery: public BaseTest { + protected: + ActiveTopology active {{"stream"}}; + EnumeratedDeviceList devices {{.m_device_id = "stream"}}; + SingleDisplayConfigState state {{{{"dock"}}, {"dock"}}, {{{"stream"}}, {}, {}, {}}}; + std::vector writes; + bool fail_activation {}; + bool lie_about_activation {}; + bool fail_cleanup {}; + bool fail_clear {}; + bool lie_about_cleanup {}; + int clears {}; + std::shared_ptr> api = std::make_shared>(); + std::shared_ptr> persistence = std::make_shared>(); + std::shared_ptr> audio = std::make_shared>(); + std::unique_ptr manager; + + void init() { + ON_CALL(*api, isApiAccessAvailable()).WillByDefault(Return(true)); + ON_CALL(*api, enumAvailableDevices()).WillByDefault([&] { + return devices; + }); + ON_CALL(*api, getCurrentTopology()).WillByDefault([&] { + return active; + }); + ON_CALL(*api, isTopologyValid(_)).WillByDefault([](const auto &t) { + return !t.empty(); + }); + ON_CALL(*api, isTopologyTheSame(_, _)).WillByDefault([](const auto &a, const auto &b) { + return win_utils::flattenTopology(a) == win_utils::flattenTopology(b); + }); + ON_CALL(*api, setTopology(_)).WillByDefault([&](const ActiveTopology &target) { + writes.push_back(target); + const auto ids = win_utils::flattenTopology(target); + if (ids.empty()) { + ADD_FAILURE() << "Attempted to blank every output"; + return false; + } + for (const auto &id : ids) { + if (std::ranges::none_of(devices, [&](const auto &d) { + return d.m_device_id == id; + })) { + return false; + } + } + if (fail_activation && ids.contains("panel")) { + return false; + } + if (fail_cleanup && !ids.contains("stream")) { + return false; + } + if (!lie_about_activation && !(lie_about_cleanup && !ids.contains("stream"))) { + active = target; + } + return true; + }); + ON_CALL(*persistence, load()).WillByDefault([&] { + return serializeState(state); + }); + ON_CALL(*persistence, store(_)).WillByDefault(Return(true)); + ON_CALL(*persistence, clear()).WillByDefault([&] { + if (fail_clear) { + return false; + } + ++clears; + return true; + }); + manager = std::make_unique(api, audio, std::make_unique(persistence), WinWorkarounds {}); + } + + void openLid() { + devices.push_back({.m_device_id = "panel", .m_is_internal = true}); + } + }; +} // namespace + +TEST_F(UndockRecovery, ClosedLidRetainsRecoveryUntilPanelAppears) { + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + EXPECT_EQ(clears, 0); + EXPECT_EQ(active, (ActiveTopology {{"stream"}})); + openLid(); + writes.clear(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"panel"}})); + ASSERT_GE(writes.size(), 3u); + EXPECT_EQ(win_utils::flattenTopology(writes[writes.size() - 2]), (StringSet {"panel", "stream"})); + EXPECT_EQ(clears, 1); + writes.clear(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_TRUE(writes.empty()); +} + +TEST_F(UndockRecovery, RecoverySurvivesControllerRestart) { + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + manager.reset(); + openLid(); + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"panel"}})); +} + +TEST_F(UndockRecovery, RedockRestoresOriginalOutput) { + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + devices.push_back({.m_device_id = "dock"}); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"dock"}})); + EXPECT_EQ(clears, 1); +} + +TEST_F(UndockRecovery, PreservesUnrelatedActiveOutput) { + openLid(); + devices.push_back({.m_device_id = "other"}); + active.push_back({"other"}); + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(win_utils::flattenTopology(active), (StringSet {"panel", "other"})); +} + +TEST_F(UndockRecovery, RestoresSurvivingOriginalDisplay) { + state.m_initial.m_topology.push_back({"survivor"}); + devices.push_back({.m_device_id = "survivor"}); + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"survivor"}})); +} + +TEST_F(UndockRecovery, DoesNotMistakeUnknownOutputForLaptopPanel) { + devices.push_back({.m_device_id = "unknown"}); + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + EXPECT_EQ(clears, 0); +} + +TEST_F(UndockRecovery, ActivationFailureDoesNotRemoveStreamingOutput) { + openLid(); + fail_activation = true; + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + EXPECT_EQ(clears, 0); + EXPECT_TRUE(win_utils::flattenTopology(active).contains("stream")); +} + +TEST_F(UndockRecovery, ActivationRequiresReadback) { + openLid(); + lie_about_activation = true; + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + EXPECT_EQ(clears, 0); + for (const auto &target : writes) { + if (!win_utils::flattenTopology(target).contains("dock")) { + EXPECT_TRUE(win_utils::flattenTopology(target).contains("stream")); + } + } +} + +TEST_F(UndockRecovery, CleanupFailureRetainsPersistence) { + openLid(); + fail_cleanup = true; + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + EXPECT_EQ(clears, 0); + EXPECT_TRUE(win_utils::flattenTopology(active).contains("panel")); +} + +TEST_F(UndockRecovery, FailedPersistenceCanRetry) { + openLid(); + fail_clear = true; + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::PersistenceSaveFailed); + EXPECT_EQ(clears, 0); + fail_clear = false; + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"panel"}})); + EXPECT_EQ(clears, 1); +} + +TEST_F(UndockRecovery, CleanupRequiresReadback) { + openLid(); + lie_about_cleanup = true; + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + EXPECT_EQ(clears, 0); + EXPECT_TRUE(win_utils::flattenTopology(active).contains("stream")); +} diff --git a/tests/unit/windows/test_win_display_device_general.cpp b/tests/unit/windows/test_win_display_device_general.cpp index 3adfdc2e..50275a63 100644 --- a/tests/unit/windows/test_win_display_device_general.cpp +++ b/tests/unit/windows/test_win_display_device_general.cpp @@ -151,6 +151,9 @@ TEST_F_S_MOCKED(EnumAvailableDevices) { const auto pam_active_and_inactive {[]() { auto pam {ut_consts::PAM_3_ACTIVE}; pam->m_paths.at(0).targetInfo.refreshRate.Denominator = 0; + pam->m_paths.at(0).targetInfo.outputTechnology = DISPLAYCONFIG_OUTPUT_TECHNOLOGY_DISPLAYPORT_EMBEDDED; + pam->m_paths.at(1).targetInfo.outputTechnology = DISPLAYCONFIG_OUTPUT_TECHNOLOGY_INDIRECT_WIRED; + pam->m_paths.at(2).targetInfo.outputTechnology = DISPLAYCONFIG_OUTPUT_TECHNOLOGY_LVDS; pam->m_paths.at(2).flags &= ~DISPLAYCONFIG_PATH_ACTIVE; return pam; }()}; @@ -179,7 +182,8 @@ TEST_F_S_MOCKED(EnumAvailableDevices) { true, {0, 0}, std::nullopt - }}, + }, + true}, {"DeviceId2", "DisplayName2", "FriendlyName2", @@ -196,7 +200,8 @@ TEST_F_S_MOCKED(EnumAvailableDevices) { "", "FriendlyName3", std::nullopt, - std::nullopt} + std::nullopt, + true} }; EXPECT_EQ(m_win_dd.enumAvailableDevices(), expected_list); } diff --git a/tests/unit/windows/test_win_display_device_modes.cpp b/tests/unit/windows/test_win_display_device_modes.cpp index 543361b8..687be667 100644 --- a/tests/unit/windows/test_win_display_device_modes.cpp +++ b/tests/unit/windows/test_win_display_device_modes.cpp @@ -590,3 +590,127 @@ TEST_F_S_MOCKED(SetDisplayModes, Strict, ModesDidNotChange) { EXPECT_FALSE(m_win_dd.setDisplayModes(new_modes)); } + +// Session-only CCD changes must never replace the saved docking layout. +TEST_F_S_MOCKED(SetDisplayModes, Strict, Temporary) { + m_win_dd = display_device::WinDisplayDevice {m_layer, false}; + const display_device::DeviceDisplayModeMap new_modes { + {"DeviceId1", {1920, 1080, {120, 10}}}, + {"DeviceId2", {1000, 2160, {119995, 100}}}, + {"DeviceId3", {1000, 1000, {90, 1}}}, + {"DeviceId4", {3840, 2160, {90, 1}}}, + }; + + const auto pam_initial {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; + const auto pam_submitted {applyExpectedModesOntoInput(pam_initial, new_modes, {"DeviceId4"})}; + + InSequence sequence; + setupExpectedGetAllDeviceIdsCall(sequence); + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::All)) + .Times(1) + .WillOnce(Return(pam_initial)) + .RetiresOnSaturation(); + + // Relaxed try + { + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) + .Times(1) + .WillOnce(Return(pam_initial)) + .RetiresOnSaturation(); + setupExpectedGetActivePathCall(1, sequence); + setupExpectedGetActivePathCall(2, sequence); + setupExpectedGetActivePathCall(3, sequence); + setupExpectedGetActivePathCall(4, sequence); + + EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (RELAXED_FLAGS & ~SDC_SAVE_TO_DATABASE))) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)) + .RetiresOnSaturation(); + setupExpectedGetCurrentDisplayModesCall(sequence, pam_initial); + } + + // Strict try + { + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) + .Times(1) + .WillOnce(Return(pam_initial)) + .RetiresOnSaturation(); + setupExpectedGetActivePathCall(1, sequence); + setupExpectedGetActivePathCall(2, sequence); + setupExpectedGetActivePathCall(3, sequence); + setupExpectedGetActivePathCall(4, sequence); + + EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (STRICT_FLAGS & ~SDC_SAVE_TO_DATABASE))) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)) + .RetiresOnSaturation(); + setupExpectedGetCurrentDisplayModesCall(sequence, pam_submitted); + } + + EXPECT_TRUE(m_win_dd.setDisplayModes(new_modes)); +} + +TEST_F_S_MOCKED(SetDisplayModes, Strict, FailedToSetDisplayConfig, Temporary) { + m_win_dd = display_device::WinDisplayDevice {m_layer, false}; + const display_device::DeviceDisplayModeMap new_modes { + {"DeviceId1", {1920, 1080, {120, 10}}}, + {"DeviceId2", {1000, 2160, {119995, 100}}}, + {"DeviceId3", {1000, 1000, {90, 1}}}, + {"DeviceId4", {3840, 2160, {90, 1}}}, + }; + + const auto pam_initial {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; + const auto pam_submitted {applyExpectedModesOntoInput(pam_initial, new_modes, {"DeviceId4"})}; + + InSequence sequence; + setupExpectedGetAllDeviceIdsCall(sequence); + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::All)) + .Times(1) + .WillOnce(Return(pam_initial)) + .RetiresOnSaturation(); + + // Relaxed try + { + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) + .Times(1) + .WillOnce(Return(pam_initial)) + .RetiresOnSaturation(); + setupExpectedGetActivePathCall(1, sequence); + setupExpectedGetActivePathCall(2, sequence); + setupExpectedGetActivePathCall(3, sequence); + setupExpectedGetActivePathCall(4, sequence); + + EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (RELAXED_FLAGS & ~SDC_SAVE_TO_DATABASE))) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)) + .RetiresOnSaturation(); + setupExpectedGetCurrentDisplayModesCall(sequence, pam_initial); + } + + // Strict try + { + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) + .Times(1) + .WillOnce(Return(pam_initial)) + .RetiresOnSaturation(); + setupExpectedGetActivePathCall(1, sequence); + setupExpectedGetActivePathCall(2, sequence); + setupExpectedGetActivePathCall(3, sequence); + setupExpectedGetActivePathCall(4, sequence); + + EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (STRICT_FLAGS & ~SDC_SAVE_TO_DATABASE))) + .Times(1) + .WillOnce(Return(ERROR_ACCESS_DENIED)) + .RetiresOnSaturation(); + EXPECT_CALL(*m_layer, getErrorString(ERROR_ACCESS_DENIED)) + .Times(1) + .WillRepeatedly(Return("ErrorDesc")) + .RetiresOnSaturation(); + EXPECT_CALL(*m_layer, setDisplayConfig(pam_initial->m_paths, pam_initial->m_modes, (UNDO_FLAGS & ~SDC_SAVE_TO_DATABASE))) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)) + .RetiresOnSaturation(); + } + + EXPECT_FALSE(m_win_dd.setDisplayModes(new_modes)); +} diff --git a/tests/unit/windows/test_win_display_device_primary.cpp b/tests/unit/windows/test_win_display_device_primary.cpp index ca720c90..6f081ab7 100644 --- a/tests/unit/windows/test_win_display_device_primary.cpp +++ b/tests/unit/windows/test_win_display_device_primary.cpp @@ -317,3 +317,35 @@ TEST_F_S_MOCKED(SetAsPrimary, FailedToSetDisplayConfig) { EXPECT_FALSE(m_win_dd.setAsPrimary("DeviceId4")); } + +// Session-only CCD changes must never replace the saved docking layout. +TEST_F_S_MOCKED(SetAsPrimary, NonDuplicatePrimaryDeviceSet, Temporary) { + m_win_dd = display_device::WinDisplayDevice {m_layer, false}; + const auto initial_pam {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; + + auto origin_point {initial_pam->m_modes.at(initial_pam->m_paths.at(3).sourceInfo.sourceModeInfoIdx).sourceMode.position}; + auto expected_pam {initial_pam}; + shiftModeBy(expected_pam, 0, origin_point); + shiftModeBy(expected_pam, 1, origin_point); + shiftModeBy(expected_pam, 2, origin_point); + shiftModeBy(expected_pam, 3, origin_point); + + InSequence sequence; + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) + .Times(1) + .WillOnce(Return(initial_pam)); + setupExpectedGetActivePathCall(4, sequence); + for (int i = 1; i <= 4; ++i) { + EXPECT_CALL(*m_layer, getDeviceId(_)) + .Times(1) + .WillOnce(Return("DeviceId" + std::to_string(i))) + .RetiresOnSaturation(); + } + + EXPECT_CALL(*m_layer, setDisplayConfig(expected_pam->m_paths, expected_pam->m_modes, (FLAGS & ~SDC_SAVE_TO_DATABASE))) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)) + .RetiresOnSaturation(); + + EXPECT_TRUE(m_win_dd.setAsPrimary("DeviceId4")); +} diff --git a/tests/unit/windows/test_win_display_device_topology.cpp b/tests/unit/windows/test_win_display_device_topology.cpp index e747cf12..9e5d06c4 100644 --- a/tests/unit/windows/test_win_display_device_topology.cpp +++ b/tests/unit/windows/test_win_display_device_topology.cpp @@ -426,3 +426,84 @@ TEST_F_S_MOCKED(SetCurrentTopology, TopologyWasSetAccordingToWinApi, WinApiLied) EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); } + +// Session-only CCD changes must never replace the saved docking layout. +TEST_F_S_MOCKED(SetCurrentTopology, Temporary) { + m_win_dd = display_device::WinDisplayDevice {m_layer, false}; + InSequence sequence; + setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); + setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); + + auto expected_path {ut_consts::PAM_3_ACTIVE->m_paths.at(0)}; + display_device::win_utils::setCloneGroupId(expected_path, 0); + display_device::win_utils::setSourceIndex(expected_path, std::nullopt); + display_device::win_utils::setTargetIndex(expected_path, std::nullopt); + display_device::win_utils::setDesktopIndex(expected_path, std::nullopt); + display_device::win_utils::setActive(expected_path); + + UINT32 expected_flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + EXPECT_CALL(*m_layer, setDisplayConfig(std::vector {expected_path}, std::vector {}, expected_flags)) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)); + + // Report only 1 active device + EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) + .Times(1) + .WillOnce(Return(display_device::PathAndModeData {{ut_consts::PAM_3_ACTIVE->m_paths.at(0)}, {ut_consts::PAM_3_ACTIVE->m_modes.at(0)}})) + .RetiresOnSaturation(); + EXPECT_CALL(*m_layer, getMonitorDevicePath(_)) + .Times(1) + .WillOnce(Return("Path1")) + .RetiresOnSaturation(); + EXPECT_CALL(*m_layer, getDeviceId(_)) + .Times(1) + .WillOnce(Return("DeviceId1")) + .RetiresOnSaturation(); + EXPECT_CALL(*m_layer, getDisplayName(_)) + .Times(1) + .WillOnce(Return("DisplayName1")) + .RetiresOnSaturation(); + + EXPECT_TRUE(m_win_dd.setTopology({{"DeviceId1"}})); +} + +TEST_F_S_MOCKED(SetCurrentTopology, TopologyWasSetAccordingToWinApi, WinApiLied, Temporary) { + m_win_dd = display_device::WinDisplayDevice {m_layer, false}; + InSequence sequence; + setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); + setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); + + UINT32 expected_flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)); + + // Called when getting the topology + setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); + + // Called when doing the undo + expected_flags = SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_VIRTUAL_MODE_AWARE; + EXPECT_CALL(*m_layer, setDisplayConfig(ut_consts::PAM_3_ACTIVE->m_paths, ut_consts::PAM_3_ACTIVE->m_modes, expected_flags)) + .Times(1) + .WillOnce(Return(ERROR_SUCCESS)); + + EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); +} + +TEST_F_S_MOCKED(SetCurrentTopology, FailedToSetTopology, NoRecovery, Temporary) { + m_win_dd = display_device::WinDisplayDevice {m_layer, false}; + InSequence sequence; + setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); + setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); + + UINT32 expected_flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) + .Times(1) + .WillOnce(Return(ERROR_INVALID_PARAMETER)); + + EXPECT_CALL(*m_layer, getErrorString(ERROR_INVALID_PARAMETER)) + .Times(1) + .WillRepeatedly(Return("ErrorDesc")); + + EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); +} From 4f69b4d6fd572d5c83e2f2287331ed75bd08e31a Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Sat, 5 Sep 2026 19:35:53 -0500 Subject: [PATCH 02/12] refactor: simplify undock recovery and share display policy tests --- src/windows/settings_manager_revert.cpp | 45 +++--- src/windows/win_display_device_topology.cpp | 3 +- .../windows/test_settings_manager_undock.cpp | 73 +++++---- .../windows/test_win_display_device_modes.cpp | 152 +++--------------- .../test_win_display_device_primary.cpp | 50 ++---- .../test_win_display_device_topology.cpp | 105 ++---------- 6 files changed, 122 insertions(+), 306 deletions(-) diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index 8d0ef499..c4f8a503 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -22,6 +22,28 @@ namespace display_device { void noopFn() { // Intentionally empty guard callback. } + + /** + * @brief Append available groups without reactivating stream-only outputs or duplicating devices. + * @param topology Groups to consider, in priority order. + * @param available Currently enumerated device IDs. + * @param activated Device IDs activated only for the stream. + * @param target Recovery topology to extend. + * @param included IDs already included in the recovery topology. + */ + void appendRecoveryGroups(const ActiveTopology &topology, const StringSet &available, const StringSet &activated, ActiveTopology &target, StringSet &included) { + for (const auto &group : topology) { + std::vector remaining; + for (const auto &id : group) { + if (available.contains(id) && !activated.contains(id) && included.insert(id).second) { + remaining.push_back(id); + } + } + if (!remaining.empty()) { + target.push_back(std::move(remaining)); + } + } + } } // namespace SettingsManager::RevertResult SettingsManager::revertSettings() { @@ -111,7 +133,7 @@ namespace display_device { for (const auto &device : devices) { available.insert(device.m_device_id); } - if (available.empty() || std::ranges::all_of(initial, [&](const auto &id) { + if (available.empty() || std::ranges::all_of(initial, [&available](const auto &id) { return available.contains(id); })) { // An API failure with unchanged hardware must not discard the original state. @@ -122,25 +144,12 @@ namespace display_device { std::ranges::set_difference(modified, initial, std::inserter(activated, activated.end())); ActiveTopology target; StringSet included; - const auto append = [&](const ActiveTopology &topology) { - for (const auto &group : topology) { - std::vector remaining; - for (const auto &id : group) { - if (available.contains(id) && !activated.contains(id) && included.insert(id).second) { - remaining.push_back(id); - } - } - if (!remaining.empty()) { - target.push_back(std::move(remaining)); - } - } - }; - append(state.m_initial.m_topology); + appendRecoveryGroups(state.m_initial.m_topology, available, activated, target, included); if (target.empty()) { // An arbitrary virtual/unknown output is not proof of a usable laptop screen. for (const auto &device : devices) { if (device.m_is_internal && !activated.contains(device.m_device_id)) { - append({{device.m_device_id}}); + appendRecoveryGroups({{device.m_device_id}}, available, activated, target, included); break; } } @@ -149,7 +158,7 @@ namespace display_device { return false; // Closed lid and no original display: retain pending recovery. } const auto replacements {win_utils::flattenTopology(target)}; - append(current_topology); // Preserve unrelated displays activated by the user. + appendRecoveryGroups(current_topology, available, activated, target, included); // Preserve unrelated displays activated by the user. // First activate the replacement without switching off the current output. ActiveTopology staged {current_topology}; @@ -163,7 +172,7 @@ namespace display_device { return false; } const auto active {win_utils::flattenTopology(m_dd_api->getCurrentTopology())}; - if (!std::ranges::all_of(replacements, [&](const auto &id) { + if (!std::ranges::all_of(replacements, [&active](const auto &id) { return active.contains(id); })) { return false; diff --git a/src/windows/win_display_device_topology.cpp b/src/windows/win_display_device_topology.cpp index 3b69caca..e530ff06 100644 --- a/src/windows/win_display_device_topology.cpp +++ b/src/windows/win_display_device_topology.cpp @@ -36,8 +36,7 @@ namespace display_device { // TOPOLOGY_SUPPLIED also changes the remembered topology. Session-only // layouts must use the explicitly temporary supplied-config API path. const UINT32 flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; - const LONG result {w_api.setDisplayConfig(paths, {}, flags)}; - if (result != ERROR_SUCCESS) { + if (const LONG result {w_api.setDisplayConfig(paths, {}, flags)}; result != ERROR_SUCCESS) { DD_LOG(error) << w_api.getErrorString(result) << " failed to set temporary topology!"; return false; } diff --git a/tests/unit/windows/test_settings_manager_undock.cpp b/tests/unit/windows/test_settings_manager_undock.cpp index 1e867a56..a24044ee 100644 --- a/tests/unit/windows/test_settings_manager_undock.cpp +++ b/tests/unit/windows/test_settings_manager_undock.cpp @@ -1,3 +1,7 @@ +/** + * @file tests/unit/windows/test_settings_manager_undock.cpp + * @brief Tests for display recovery across laptop lid and docking changes. + */ #include "display_device/windows/settings_manager.h" #include "display_device/windows/settings_utils.h" #include "fixtures/fixtures.h" @@ -15,7 +19,7 @@ namespace { using ::testing::Return; class UndockRecovery: public BaseTest { - protected: + public: ActiveTopology active {{"stream"}}; EnumeratedDeviceList devices {{.m_device_id = "stream"}}; SingleDisplayConfigState state {{{{"dock"}}, {"dock"}}, {{{"stream"}}, {}, {}, {}}}; @@ -31,12 +35,44 @@ namespace { std::shared_ptr> audio = std::make_shared>(); std::unique_ptr manager; + /** + * @brief Simulate topology writes, including unavailable devices and unreliable API results. + * @param target Requested active display groups. + * @return Whether the simulated API reports success. + */ + bool applyTopology(const ActiveTopology &target) { + writes.push_back(target); + const auto ids = win_utils::flattenTopology(target); + if (ids.empty()) { + ADD_FAILURE() << "Attempted to blank every output"; + return false; + } + for (const auto &id : ids) { + if (std::ranges::none_of(devices, [&id](const auto &d) { + return d.m_device_id == id; + })) { + return false; + } + } + if (fail_activation && ids.contains("panel")) { + return false; + } + if (fail_cleanup && !ids.contains("stream")) { + return false; + } + if (!lie_about_activation && !(lie_about_cleanup && !ids.contains("stream"))) { + active = target; + } + return true; + } + + /** @brief Connect the stateful display and persistence mocks to the manager. */ void init() { ON_CALL(*api, isApiAccessAvailable()).WillByDefault(Return(true)); - ON_CALL(*api, enumAvailableDevices()).WillByDefault([&] { + ON_CALL(*api, enumAvailableDevices()).WillByDefault([this] { return devices; }); - ON_CALL(*api, getCurrentTopology()).WillByDefault([&] { + ON_CALL(*api, getCurrentTopology()).WillByDefault([this] { return active; }); ON_CALL(*api, isTopologyValid(_)).WillByDefault([](const auto &t) { @@ -45,36 +81,14 @@ namespace { ON_CALL(*api, isTopologyTheSame(_, _)).WillByDefault([](const auto &a, const auto &b) { return win_utils::flattenTopology(a) == win_utils::flattenTopology(b); }); - ON_CALL(*api, setTopology(_)).WillByDefault([&](const ActiveTopology &target) { - writes.push_back(target); - const auto ids = win_utils::flattenTopology(target); - if (ids.empty()) { - ADD_FAILURE() << "Attempted to blank every output"; - return false; - } - for (const auto &id : ids) { - if (std::ranges::none_of(devices, [&](const auto &d) { - return d.m_device_id == id; - })) { - return false; - } - } - if (fail_activation && ids.contains("panel")) { - return false; - } - if (fail_cleanup && !ids.contains("stream")) { - return false; - } - if (!lie_about_activation && !(lie_about_cleanup && !ids.contains("stream"))) { - active = target; - } - return true; + ON_CALL(*api, setTopology(_)).WillByDefault([this](const ActiveTopology &target) { + return applyTopology(target); }); - ON_CALL(*persistence, load()).WillByDefault([&] { + ON_CALL(*persistence, load()).WillByDefault([this] { return serializeState(state); }); ON_CALL(*persistence, store(_)).WillByDefault(Return(true)); - ON_CALL(*persistence, clear()).WillByDefault([&] { + ON_CALL(*persistence, clear()).WillByDefault([this] { if (fail_clear) { return false; } @@ -84,6 +98,7 @@ namespace { manager = std::make_unique(api, audio, std::make_unique(persistence), WinWorkarounds {}); } + /** @brief Make the built-in panel available after opening the lid. */ void openLid() { devices.push_back({.m_device_id = "panel", .m_is_internal = true}); } diff --git a/tests/unit/windows/test_win_display_device_modes.cpp b/tests/unit/windows/test_win_display_device_modes.cpp index 687be667..0ed204d6 100644 --- a/tests/unit/windows/test_win_display_device_modes.cpp +++ b/tests/unit/windows/test_win_display_device_modes.cpp @@ -102,8 +102,8 @@ namespace { } } - void setupExpectedDisplayModeUndo(const std::optional &pam_initial) const { - EXPECT_CALL(*m_layer, setDisplayConfig(pam_initial->m_paths, pam_initial->m_modes, UNDO_FLAGS)) + void setupExpectedDisplayModeUndo(const std::optional &pam_initial, const UINT32 flags = UNDO_FLAGS) const { + EXPECT_CALL(*m_layer, setDisplayConfig(pam_initial->m_paths, pam_initial->m_modes, flags)) .Times(1) .WillOnce(Return(ERROR_SUCCESS)) .RetiresOnSaturation(); @@ -113,6 +113,16 @@ namespace { display_device::WinDisplayDevice m_win_dd {m_layer}; }; + /** @brief Exercise saved and session-only display changes with the same assertions. */ + class WinDisplayDeviceModesPersistence: public WinDisplayDeviceModesMocked, public ::testing::WithParamInterface { + public: + WinDisplayDeviceModesPersistence() { + m_win_dd = display_device::WinDisplayDevice {m_layer, GetParam()}; + } + }; + + INSTANTIATE_TEST_SUITE_P(SaveToDatabase, WinDisplayDeviceModesPersistence, ::testing::Bool()); + // Specialized TEST macro(s) for this test file #define TEST_F_S(...) DD_MAKE_TEST(TEST_F, WinDisplayDeviceModes, __VA_ARGS__) #define TEST_F_S_MOCKED(...) DD_MAKE_TEST(TEST_F, WinDisplayDeviceModesMocked, __VA_ARGS__) @@ -339,7 +349,7 @@ TEST_F_S_MOCKED(SetDisplayModes, Relaxed) { EXPECT_TRUE(m_win_dd.setDisplayModes(new_modes)); } -TEST_F_S_MOCKED(SetDisplayModes, Strict) { +TEST_P(WinDisplayDeviceModesPersistence, Strict) { const auto new_modes {makeStrictDisplayModes()}; const auto &pam_initial {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; const auto pam_submitted {applyExpectedModesOntoInput(pam_initial, new_modes, {"DeviceId4"})}; @@ -349,13 +359,13 @@ TEST_F_S_MOCKED(SetDisplayModes, Strict) { // Relaxed try { - setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, RELAXED_FLAGS); + setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, (GetParam() ? RELAXED_FLAGS : RELAXED_FLAGS & ~SDC_SAVE_TO_DATABASE)); setupExpectedGetCurrentDisplayModesCall(sequence, pam_initial); } // Strict try { - setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, STRICT_FLAGS); + setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, (GetParam() ? STRICT_FLAGS : STRICT_FLAGS & ~SDC_SAVE_TO_DATABASE)); setupExpectedGetCurrentDisplayModesCall(sequence, pam_submitted); } @@ -517,7 +527,7 @@ TEST_F_S_MOCKED(SetDisplayModes, Relaxed, FailedToGetCurrentDisplayModes) { EXPECT_FALSE(m_win_dd.setDisplayModes(new_modes)); } -TEST_F_S_MOCKED(SetDisplayModes, Strict, FailedToSetDisplayConfig) { +TEST_P(WinDisplayDeviceModesPersistence, StrictFailureRestoresOriginalModes) { const auto new_modes {makeStrictDisplayModes()}; const auto &pam_initial {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; const auto pam_submitted {applyExpectedModesOntoInput(pam_initial, new_modes, {"DeviceId4"})}; @@ -527,14 +537,14 @@ TEST_F_S_MOCKED(SetDisplayModes, Strict, FailedToSetDisplayConfig) { // Relaxed try { - setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, RELAXED_FLAGS); + setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, (GetParam() ? RELAXED_FLAGS : RELAXED_FLAGS & ~SDC_SAVE_TO_DATABASE)); setupExpectedGetCurrentDisplayModesCall(sequence, pam_initial); } // Strict try { - setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, STRICT_FLAGS, ERROR_ACCESS_DENIED); - setupExpectedDisplayModeUndo(pam_initial); + setupExpectedDisplayModeSubmitAttempt(sequence, pam_initial, pam_submitted, (GetParam() ? STRICT_FLAGS : STRICT_FLAGS & ~SDC_SAVE_TO_DATABASE), ERROR_ACCESS_DENIED); + setupExpectedDisplayModeUndo(pam_initial, GetParam() ? UNDO_FLAGS : UNDO_FLAGS & ~SDC_SAVE_TO_DATABASE); } EXPECT_FALSE(m_win_dd.setDisplayModes(new_modes)); @@ -590,127 +600,3 @@ TEST_F_S_MOCKED(SetDisplayModes, Strict, ModesDidNotChange) { EXPECT_FALSE(m_win_dd.setDisplayModes(new_modes)); } - -// Session-only CCD changes must never replace the saved docking layout. -TEST_F_S_MOCKED(SetDisplayModes, Strict, Temporary) { - m_win_dd = display_device::WinDisplayDevice {m_layer, false}; - const display_device::DeviceDisplayModeMap new_modes { - {"DeviceId1", {1920, 1080, {120, 10}}}, - {"DeviceId2", {1000, 2160, {119995, 100}}}, - {"DeviceId3", {1000, 1000, {90, 1}}}, - {"DeviceId4", {3840, 2160, {90, 1}}}, - }; - - const auto pam_initial {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; - const auto pam_submitted {applyExpectedModesOntoInput(pam_initial, new_modes, {"DeviceId4"})}; - - InSequence sequence; - setupExpectedGetAllDeviceIdsCall(sequence); - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::All)) - .Times(1) - .WillOnce(Return(pam_initial)) - .RetiresOnSaturation(); - - // Relaxed try - { - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) - .Times(1) - .WillOnce(Return(pam_initial)) - .RetiresOnSaturation(); - setupExpectedGetActivePathCall(1, sequence); - setupExpectedGetActivePathCall(2, sequence); - setupExpectedGetActivePathCall(3, sequence); - setupExpectedGetActivePathCall(4, sequence); - - EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (RELAXED_FLAGS & ~SDC_SAVE_TO_DATABASE))) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)) - .RetiresOnSaturation(); - setupExpectedGetCurrentDisplayModesCall(sequence, pam_initial); - } - - // Strict try - { - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) - .Times(1) - .WillOnce(Return(pam_initial)) - .RetiresOnSaturation(); - setupExpectedGetActivePathCall(1, sequence); - setupExpectedGetActivePathCall(2, sequence); - setupExpectedGetActivePathCall(3, sequence); - setupExpectedGetActivePathCall(4, sequence); - - EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (STRICT_FLAGS & ~SDC_SAVE_TO_DATABASE))) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)) - .RetiresOnSaturation(); - setupExpectedGetCurrentDisplayModesCall(sequence, pam_submitted); - } - - EXPECT_TRUE(m_win_dd.setDisplayModes(new_modes)); -} - -TEST_F_S_MOCKED(SetDisplayModes, Strict, FailedToSetDisplayConfig, Temporary) { - m_win_dd = display_device::WinDisplayDevice {m_layer, false}; - const display_device::DeviceDisplayModeMap new_modes { - {"DeviceId1", {1920, 1080, {120, 10}}}, - {"DeviceId2", {1000, 2160, {119995, 100}}}, - {"DeviceId3", {1000, 1000, {90, 1}}}, - {"DeviceId4", {3840, 2160, {90, 1}}}, - }; - - const auto pam_initial {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; - const auto pam_submitted {applyExpectedModesOntoInput(pam_initial, new_modes, {"DeviceId4"})}; - - InSequence sequence; - setupExpectedGetAllDeviceIdsCall(sequence); - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::All)) - .Times(1) - .WillOnce(Return(pam_initial)) - .RetiresOnSaturation(); - - // Relaxed try - { - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) - .Times(1) - .WillOnce(Return(pam_initial)) - .RetiresOnSaturation(); - setupExpectedGetActivePathCall(1, sequence); - setupExpectedGetActivePathCall(2, sequence); - setupExpectedGetActivePathCall(3, sequence); - setupExpectedGetActivePathCall(4, sequence); - - EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (RELAXED_FLAGS & ~SDC_SAVE_TO_DATABASE))) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)) - .RetiresOnSaturation(); - setupExpectedGetCurrentDisplayModesCall(sequence, pam_initial); - } - - // Strict try - { - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) - .Times(1) - .WillOnce(Return(pam_initial)) - .RetiresOnSaturation(); - setupExpectedGetActivePathCall(1, sequence); - setupExpectedGetActivePathCall(2, sequence); - setupExpectedGetActivePathCall(3, sequence); - setupExpectedGetActivePathCall(4, sequence); - - EXPECT_CALL(*m_layer, setDisplayConfig(pam_submitted->m_paths, pam_submitted->m_modes, (STRICT_FLAGS & ~SDC_SAVE_TO_DATABASE))) - .Times(1) - .WillOnce(Return(ERROR_ACCESS_DENIED)) - .RetiresOnSaturation(); - EXPECT_CALL(*m_layer, getErrorString(ERROR_ACCESS_DENIED)) - .Times(1) - .WillRepeatedly(Return("ErrorDesc")) - .RetiresOnSaturation(); - EXPECT_CALL(*m_layer, setDisplayConfig(pam_initial->m_paths, pam_initial->m_modes, (UNDO_FLAGS & ~SDC_SAVE_TO_DATABASE))) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)) - .RetiresOnSaturation(); - } - - EXPECT_FALSE(m_win_dd.setDisplayModes(new_modes)); -} diff --git a/tests/unit/windows/test_win_display_device_primary.cpp b/tests/unit/windows/test_win_display_device_primary.cpp index 6f081ab7..f8de40ba 100644 --- a/tests/unit/windows/test_win_display_device_primary.cpp +++ b/tests/unit/windows/test_win_display_device_primary.cpp @@ -39,14 +39,14 @@ namespace { expectActivePathLookup(m_layer, id_number); } - void setupExpectedSetAsPrimaryConfigCall(InSequence &sequence /* To ensure that sequence is created outside this scope */, const std::optional &initial_pam, const std::optional &expected_pam, const int active_path_id, const LONG result = ERROR_SUCCESS) const { + void setupExpectedSetAsPrimaryConfigCall(InSequence &sequence /* To ensure that sequence is created outside this scope */, const std::optional &initial_pam, const std::optional &expected_pam, const int active_path_id, const LONG result = ERROR_SUCCESS, const UINT32 flags = FLAGS) const { EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) .Times(1) .WillOnce(Return(initial_pam)); setupExpectedGetActivePathCall(active_path_id, sequence); expectDeviceIdLookups(m_layer, 4); - EXPECT_CALL(*m_layer, setDisplayConfig(expected_pam->m_paths, expected_pam->m_modes, FLAGS)) + EXPECT_CALL(*m_layer, setDisplayConfig(expected_pam->m_paths, expected_pam->m_modes, flags)) .Times(1) .WillOnce(Return(result)) .RetiresOnSaturation(); @@ -63,6 +63,16 @@ namespace { display_device::WinDisplayDevice m_win_dd {m_layer}; }; + /** @brief Exercise saved and session-only display changes with the same assertions. */ + class WinDisplayDevicePrimaryPersistence: public WinDisplayDevicePrimaryMocked, public ::testing::WithParamInterface { + public: + WinDisplayDevicePrimaryPersistence() { + m_win_dd = display_device::WinDisplayDevice {m_layer, GetParam()}; + } + }; + + INSTANTIATE_TEST_SUITE_P(SaveToDatabase, WinDisplayDevicePrimaryPersistence, ::testing::Bool()); + // Specialized TEST macro(s) for this test file #define TEST_F_S(...) DD_MAKE_TEST(TEST_F, WinDisplayDevicePrimary, __VA_ARGS__) #define TEST_F_S_MOCKED(...) DD_MAKE_TEST(TEST_F, WinDisplayDevicePrimaryMocked, __VA_ARGS__) @@ -221,12 +231,12 @@ TEST_F_S_MOCKED(SetAsPrimary, DuplicatePrimaryDevicesSet) { EXPECT_TRUE(m_win_dd.setAsPrimary("DeviceId2")); } -TEST_F_S_MOCKED(SetAsPrimary, NonDuplicatePrimaryDeviceSet) { +TEST_P(WinDisplayDevicePrimaryPersistence, NonDuplicatePrimaryDeviceSet) { const auto &initial_pam {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; const auto expected_pam {makeShiftedPrimaryPam(initial_pam, 3, {0, 1, 2, 3})}; InSequence sequence; - setupExpectedSetAsPrimaryConfigCall(sequence, initial_pam, expected_pam, 4); + setupExpectedSetAsPrimaryConfigCall(sequence, initial_pam, expected_pam, 4, ERROR_SUCCESS, GetParam() ? FLAGS : FLAGS & ~SDC_SAVE_TO_DATABASE); EXPECT_TRUE(m_win_dd.setAsPrimary("DeviceId4")); } @@ -317,35 +327,3 @@ TEST_F_S_MOCKED(SetAsPrimary, FailedToSetDisplayConfig) { EXPECT_FALSE(m_win_dd.setAsPrimary("DeviceId4")); } - -// Session-only CCD changes must never replace the saved docking layout. -TEST_F_S_MOCKED(SetAsPrimary, NonDuplicatePrimaryDeviceSet, Temporary) { - m_win_dd = display_device::WinDisplayDevice {m_layer, false}; - const auto initial_pam {ut_consts::PAM_4_ACTIVE_WITH_2_DUPLICATES}; - - auto origin_point {initial_pam->m_modes.at(initial_pam->m_paths.at(3).sourceInfo.sourceModeInfoIdx).sourceMode.position}; - auto expected_pam {initial_pam}; - shiftModeBy(expected_pam, 0, origin_point); - shiftModeBy(expected_pam, 1, origin_point); - shiftModeBy(expected_pam, 2, origin_point); - shiftModeBy(expected_pam, 3, origin_point); - - InSequence sequence; - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) - .Times(1) - .WillOnce(Return(initial_pam)); - setupExpectedGetActivePathCall(4, sequence); - for (int i = 1; i <= 4; ++i) { - EXPECT_CALL(*m_layer, getDeviceId(_)) - .Times(1) - .WillOnce(Return("DeviceId" + std::to_string(i))) - .RetiresOnSaturation(); - } - - EXPECT_CALL(*m_layer, setDisplayConfig(expected_pam->m_paths, expected_pam->m_modes, (FLAGS & ~SDC_SAVE_TO_DATABASE))) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)) - .RetiresOnSaturation(); - - EXPECT_TRUE(m_win_dd.setAsPrimary("DeviceId4")); -} diff --git a/tests/unit/windows/test_win_display_device_topology.cpp b/tests/unit/windows/test_win_display_device_topology.cpp index 9e5d06c4..e436a8be 100644 --- a/tests/unit/windows/test_win_display_device_topology.cpp +++ b/tests/unit/windows/test_win_display_device_topology.cpp @@ -54,6 +54,16 @@ namespace { display_device::WinDisplayDevice m_win_dd {m_layer}; }; + /** @brief Exercise saved and session-only display changes with the same assertions. */ + class WinDisplayDeviceTopologyPersistence: public WinDisplayDeviceTopologyMocked, public ::testing::WithParamInterface { + public: + WinDisplayDeviceTopologyPersistence() { + m_win_dd = display_device::WinDisplayDevice {m_layer, GetParam()}; + } + }; + + INSTANTIATE_TEST_SUITE_P(SaveToDatabase, WinDisplayDeviceTopologyPersistence, ::testing::Bool()); + // Specialized TEST macro(s) for this test file #define TEST_F_S(...) DD_MAKE_TEST(TEST_F, WinDisplayDeviceTopology, __VA_ARGS__) #define TEST_F_S_MOCKED(...) DD_MAKE_TEST(TEST_F, WinDisplayDeviceTopologyMocked, __VA_ARGS__) @@ -252,7 +262,7 @@ TEST_F_S_MOCKED(isTopologyTheSame) { EXPECT_EQ(m_win_dd.isTopologyTheSame({{"ID_3"}, {"ID_1", "ID_2"}}, {{"ID_2", "ID_1"}, {"ID_3"}}), true); } -TEST_F_S_MOCKED(SetCurrentTopology) { +TEST_P(WinDisplayDeviceTopologyPersistence, SetCurrentTopology) { InSequence sequence; setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); @@ -264,7 +274,7 @@ TEST_F_S_MOCKED(SetCurrentTopology) { display_device::win_utils::setDesktopIndex(expected_path, std::nullopt); display_device::win_utils::setActive(expected_path); - UINT32 expected_flags {SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + UINT32 expected_flags {GetParam() ? SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE : SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; EXPECT_CALL(*m_layer, setDisplayConfig(std::vector {expected_path}, std::vector {}, expected_flags)) .Times(1) .WillOnce(Return(ERROR_SUCCESS)); @@ -364,12 +374,12 @@ TEST_F_S_MOCKED(SetCurrentTopology, WindowsDoesNotKnowAboutTheTopology, FailedTo EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); } -TEST_F_S_MOCKED(SetCurrentTopology, FailedToSetTopology, NoRecovery) { +TEST_P(WinDisplayDeviceTopologyPersistence, FailedToSetTopology) { InSequence sequence; setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); - UINT32 expected_flags {SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + UINT32 expected_flags {GetParam() ? SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE : SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) .Times(1) .WillOnce(Return(ERROR_INVALID_PARAMETER)); @@ -405,12 +415,12 @@ TEST_F_S_MOCKED(SetCurrentTopology, TopologyWasSetAccordingToWinApi, CouldNotGet EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); } -TEST_F_S_MOCKED(SetCurrentTopology, TopologyWasSetAccordingToWinApi, WinApiLied) { +TEST_P(WinDisplayDeviceTopologyPersistence, ReadbackFailureRestoresOriginalTopology) { InSequence sequence; setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); - UINT32 expected_flags {SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + UINT32 expected_flags {GetParam() ? SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE : SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) .Times(1) .WillOnce(Return(ERROR_SUCCESS)); @@ -419,91 +429,10 @@ TEST_F_S_MOCKED(SetCurrentTopology, TopologyWasSetAccordingToWinApi, WinApiLied) setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); // Called when doing the undo - expected_flags = SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_SAVE_TO_DATABASE | SDC_VIRTUAL_MODE_AWARE; + expected_flags = SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_VIRTUAL_MODE_AWARE | (GetParam() ? SDC_SAVE_TO_DATABASE : 0); EXPECT_CALL(*m_layer, setDisplayConfig(ut_consts::PAM_3_ACTIVE->m_paths, ut_consts::PAM_3_ACTIVE->m_modes, expected_flags)) .Times(1) .WillOnce(Return(ERROR_SUCCESS)); EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); } - -// Session-only CCD changes must never replace the saved docking layout. -TEST_F_S_MOCKED(SetCurrentTopology, Temporary) { - m_win_dd = display_device::WinDisplayDevice {m_layer, false}; - InSequence sequence; - setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); - setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); - - auto expected_path {ut_consts::PAM_3_ACTIVE->m_paths.at(0)}; - display_device::win_utils::setCloneGroupId(expected_path, 0); - display_device::win_utils::setSourceIndex(expected_path, std::nullopt); - display_device::win_utils::setTargetIndex(expected_path, std::nullopt); - display_device::win_utils::setDesktopIndex(expected_path, std::nullopt); - display_device::win_utils::setActive(expected_path); - - UINT32 expected_flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; - EXPECT_CALL(*m_layer, setDisplayConfig(std::vector {expected_path}, std::vector {}, expected_flags)) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)); - - // Report only 1 active device - EXPECT_CALL(*m_layer, queryDisplayConfig(display_device::QueryType::Active)) - .Times(1) - .WillOnce(Return(display_device::PathAndModeData {{ut_consts::PAM_3_ACTIVE->m_paths.at(0)}, {ut_consts::PAM_3_ACTIVE->m_modes.at(0)}})) - .RetiresOnSaturation(); - EXPECT_CALL(*m_layer, getMonitorDevicePath(_)) - .Times(1) - .WillOnce(Return("Path1")) - .RetiresOnSaturation(); - EXPECT_CALL(*m_layer, getDeviceId(_)) - .Times(1) - .WillOnce(Return("DeviceId1")) - .RetiresOnSaturation(); - EXPECT_CALL(*m_layer, getDisplayName(_)) - .Times(1) - .WillOnce(Return("DisplayName1")) - .RetiresOnSaturation(); - - EXPECT_TRUE(m_win_dd.setTopology({{"DeviceId1"}})); -} - -TEST_F_S_MOCKED(SetCurrentTopology, TopologyWasSetAccordingToWinApi, WinApiLied, Temporary) { - m_win_dd = display_device::WinDisplayDevice {m_layer, false}; - InSequence sequence; - setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); - setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); - - UINT32 expected_flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; - EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)); - - // Called when getting the topology - setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); - - // Called when doing the undo - expected_flags = SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_VIRTUAL_MODE_AWARE; - EXPECT_CALL(*m_layer, setDisplayConfig(ut_consts::PAM_3_ACTIVE->m_paths, ut_consts::PAM_3_ACTIVE->m_modes, expected_flags)) - .Times(1) - .WillOnce(Return(ERROR_SUCCESS)); - - EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); -} - -TEST_F_S_MOCKED(SetCurrentTopology, FailedToSetTopology, NoRecovery, Temporary) { - m_win_dd = display_device::WinDisplayDevice {m_layer, false}; - InSequence sequence; - setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); - setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); - - UINT32 expected_flags {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; - EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) - .Times(1) - .WillOnce(Return(ERROR_INVALID_PARAMETER)); - - EXPECT_CALL(*m_layer, getErrorString(ERROR_INVALID_PARAMETER)) - .Times(1) - .WillRepeatedly(Return("ErrorDesc")); - - EXPECT_FALSE(m_win_dd.setTopology({{"DeviceId1"}})); -} From 651e67a9eaf90e3b1e8d227f708e3c2998943ea8 Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Sat, 5 Sep 2026 19:36:49 -0500 Subject: [PATCH 03/12] docs: document recovery and device serialization helpers --- src/common/json_serializer.cpp | 10 ++++++++++ .../include/display_device/windows/settings_manager.h | 2 ++ 2 files changed, 12 insertions(+) diff --git a/src/common/json_serializer.cpp b/src/common/json_serializer.cpp index 877ddf96..4d043fd0 100644 --- a/src/common/json_serializer.cpp +++ b/src/common/json_serializer.cpp @@ -21,6 +21,11 @@ namespace display_device { DD_JSON_DEFINE_SERIALIZE_STRUCT(EdidData, manufacturer_id, product_code, serial_number) DD_JSON_DEFINE_SERIALIZE_STRUCT(EnumeratedDevice::Info, resolution, resolution_scale, refresh_rate, primary, origin_point, hdr_state) + /** + * @brief Serialize an enumerated display, including its internal-panel classification. + * @param nlohmann_json_j JSON output object. + * @param nlohmann_json_t Device to serialize. + */ void to_json(nlohmann::json &nlohmann_json_j, const EnumeratedDevice &nlohmann_json_t) { DD_JSON_TO(device_id) DD_JSON_TO(display_name) @@ -30,6 +35,11 @@ namespace display_device { DD_JSON_TO(is_internal) } + /** + * @brief Deserialize a display, treating missing internal-panel information as unknown. + * @param nlohmann_json_j JSON input object. + * @param nlohmann_json_t Device to populate. + */ void from_json(const nlohmann::json &nlohmann_json_j, EnumeratedDevice &nlohmann_json_t) { DD_JSON_FROM(device_id) DD_JSON_FROM(display_name) diff --git a/src/windows/include/display_device/windows/settings_manager.h b/src/windows/include/display_device/windows/settings_manager.h index 11cee6da..b5dea508 100644 --- a/src/windows/include/display_device/windows/settings_manager.h +++ b/src/windows/include/display_device/windows/settings_manager.h @@ -152,6 +152,8 @@ namespace display_device { * @brief Recover to surviving original displays or a built-in panel after an undock. * Only devices activated by this session may be removed. The original recovery * record remains intact on failure; a visible replacement is verified first. + * @param current_topology Active topology before attempting to restore the saved layout. + * @return True when the replacement topology has been applied and verified. */ [[nodiscard]] bool recoverMissingTopology(const ActiveTopology ¤t_topology); From dc47fdaeaa9f883f832a5eab139f2e93cef896f2 Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Sat, 5 Sep 2026 19:42:17 -0500 Subject: [PATCH 04/12] test: use unsigned topology flag constants --- tests/unit/windows/test_win_display_device_topology.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/unit/windows/test_win_display_device_topology.cpp b/tests/unit/windows/test_win_display_device_topology.cpp index e436a8be..73caa882 100644 --- a/tests/unit/windows/test_win_display_device_topology.cpp +++ b/tests/unit/windows/test_win_display_device_topology.cpp @@ -16,6 +16,9 @@ namespace { using ::testing::Return; using ::testing::StrictMock; + const UINT32 SAVED_TOPOLOGY_FLAGS {SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + const UINT32 TEMPORARY_TOPOLOGY_FLAGS {SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + // Test fixture(s) for this file class WinDisplayDeviceTopology: public BaseTest { public: @@ -274,7 +277,7 @@ TEST_P(WinDisplayDeviceTopologyPersistence, SetCurrentTopology) { display_device::win_utils::setDesktopIndex(expected_path, std::nullopt); display_device::win_utils::setActive(expected_path); - UINT32 expected_flags {GetParam() ? SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE : SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + UINT32 expected_flags {GetParam() ? SAVED_TOPOLOGY_FLAGS : TEMPORARY_TOPOLOGY_FLAGS}; EXPECT_CALL(*m_layer, setDisplayConfig(std::vector {expected_path}, std::vector {}, expected_flags)) .Times(1) .WillOnce(Return(ERROR_SUCCESS)); @@ -379,7 +382,7 @@ TEST_P(WinDisplayDeviceTopologyPersistence, FailedToSetTopology) { setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); - UINT32 expected_flags {GetParam() ? SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE : SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + UINT32 expected_flags {GetParam() ? SAVED_TOPOLOGY_FLAGS : TEMPORARY_TOPOLOGY_FLAGS}; EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) .Times(1) .WillOnce(Return(ERROR_INVALID_PARAMETER)); @@ -420,7 +423,7 @@ TEST_P(WinDisplayDeviceTopologyPersistence, ReadbackFailureRestoresOriginalTopol setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::Active, sequence); setupExpectCallFor3ActivePathsAndModes(display_device::QueryType::All, sequence); - UINT32 expected_flags {GetParam() ? SDC_APPLY | SDC_TOPOLOGY_SUPPLIED | SDC_ALLOW_PATH_ORDER_CHANGES | SDC_VIRTUAL_MODE_AWARE : SDC_APPLY | SDC_USE_SUPPLIED_DISPLAY_CONFIG | SDC_ALLOW_CHANGES | SDC_VIRTUAL_MODE_AWARE}; + UINT32 expected_flags {GetParam() ? SAVED_TOPOLOGY_FLAGS : TEMPORARY_TOPOLOGY_FLAGS}; EXPECT_CALL(*m_layer, setDisplayConfig(getExpectedPathToBeSet(), std::vector {}, expected_flags)) .Times(1) .WillOnce(Return(ERROR_SUCCESS)); From 13c2e8bca49d072271383aa5bfa728db0f25356f Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Sun, 27 Sep 2026 16:45:10 -0500 Subject: [PATCH 05/12] fix(windows): revert surviving displays when the modified topology is unplugged Keep surviving clone groups intact while staging recovery. --- .../display_device/windows/settings_manager.h | 2 + src/windows/settings_manager_revert.cpp | 96 +++++++++++++++---- .../windows/test_settings_manager_revert.cpp | 1 + .../windows/test_settings_manager_undock.cpp | 49 ++++++++++ 4 files changed, 132 insertions(+), 16 deletions(-) diff --git a/src/windows/include/display_device/windows/settings_manager.h b/src/windows/include/display_device/windows/settings_manager.h index b5dea508..d9caca69 100644 --- a/src/windows/include/display_device/windows/settings_manager.h +++ b/src/windows/include/display_device/windows/settings_manager.h @@ -112,6 +112,8 @@ namespace display_device { /** * @brief Try to revert the modified settings. + * If a device from the modified topology is no longer available, only the settings + * of the remaining devices are reverted and the persisted record is left unchanged. * @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. diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index c4f8a503..62cc3d12 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -44,6 +44,42 @@ namespace display_device { } } } + + /** + * @brief Collect the IDs of the enumerated devices. + * @param devices Currently enumerated devices. + * @return Set of available device IDs. + */ + StringSet toDeviceIds(const EnumeratedDeviceList &devices) { + StringSet ids; + for (const auto &device : devices) { + ids.insert(device.m_device_id); + } + return ids; + } + + /** + * @brief Restrict modified settings to the devices that are still available. + * @param modified Modified settings to filter. + * @param available Currently enumerated device IDs. + * @return Modified settings without the unavailable devices. + */ + SingleDisplayConfigState::Modified keepAvailableDevices(const SingleDisplayConfigState::Modified &modified, const StringSet &available) { + SingleDisplayConfigState::Modified result {modified}; + result.m_topology.clear(); + StringSet included; + appendRecoveryGroups(modified.m_topology, available, {}, result.m_topology, included); + std::erase_if(result.m_original_modes, [&available](const auto &entry) { + return !available.contains(entry.first); + }); + std::erase_if(result.m_original_hdr_states, [&available](const auto &entry) { + return !available.contains(entry.first); + }); + if (!available.contains(result.m_original_primary_device)) { + result.m_original_primary_device.clear(); + } + return result; + } } // namespace SettingsManager::RevertResult SettingsManager::revertSettings() { @@ -129,10 +165,7 @@ namespace display_device { const auto initial {win_utils::flattenTopology(state.m_initial.m_topology)}; const auto modified {win_utils::flattenTopology(state.m_modified.m_topology)}; const auto devices {m_dd_api->enumAvailableDevices()}; - StringSet available; - for (const auto &device : devices) { - available.insert(device.m_device_id); - } + const auto available {toDeviceIds(devices)}; if (available.empty() || std::ranges::all_of(initial, [&available](const auto &id) { return available.contains(id); })) { @@ -161,13 +194,10 @@ namespace display_device { appendRecoveryGroups(current_topology, available, activated, target, included); // Preserve unrelated displays activated by the user. // First activate the replacement without switching off the current output. + // Keep the target's groups intact, since cloned displays share a source. ActiveTopology staged {current_topology}; auto staged_ids {win_utils::flattenTopology(staged)}; - for (const auto &id : included) { - if (staged_ids.insert(id).second) { - staged.push_back({id}); - } - } + appendRecoveryGroups(target, available, {}, staged, staged_ids); if (!m_dd_api->setTopology(staged)) { return false; } @@ -277,12 +307,38 @@ namespace display_device { return RevertResult::TopologyIsInvalid; } - const bool is_topology_the_same {m_dd_api->isTopologyTheSame(current_topology, cached_state->m_modified.m_topology)}; + auto modified_state {cached_state->m_modified}; + bool is_topology_the_same {m_dd_api->isTopologyTheSame(current_topology, modified_state.m_topology)}; system_settings_touched = !is_topology_the_same; - if (!is_topology_the_same && !m_dd_api->setTopology(cached_state->m_modified.m_topology)) { + bool devices_missing {false}; + if (!is_topology_the_same && !m_dd_api->setTopology(modified_state.m_topology)) { DD_LOG(error) << "Failed to change topology to:\n" - << toJson(cached_state->m_modified.m_topology); - return RevertResult::SwitchingTopologyFailed; + << toJson(modified_state.m_topology); + + const auto available {toDeviceIds(m_dd_api->enumAvailableDevices())}; + const auto modified_ids {win_utils::flattenTopology(modified_state.m_topology)}; + devices_missing = !available.empty() && std::ranges::any_of(modified_ids, [&available](const auto &id) { + return !available.contains(id); + }); + if (!devices_missing) { + return RevertResult::SwitchingTopologyFailed; + } + + // A display from the modified topology has been unplugged. Revert the settings of the remaining + // displays so that the initial topology, or its recovery, can still be applied. + modified_state = keepAvailableDevices(modified_state, available); + DD_LOG(warning) << "Some modified devices are unavailable, reverting settings for the remaining topology:\n" + << toJson(modified_state.m_topology); + if (modified_state.m_topology.empty()) { + return RevertResult::Ok; + } + + is_topology_the_same = m_dd_api->isTopologyTheSame(current_topology, modified_state.m_topology); + if (!is_topology_the_same && !m_dd_api->setTopology(modified_state.m_topology)) { + DD_LOG(error) << "Failed to change topology to:\n" + << toJson(modified_state.m_topology); + return RevertResult::SwitchingTopologyFailed; + } } if (switched_topology) { *switched_topology = !is_topology_the_same; @@ -290,25 +346,33 @@ namespace display_device { DdGuardFn hdr_guard_fn {noopFn}; boost::scope::scope_exit hdr_guard {hdr_guard_fn}; - if (const auto result {revertModifiedHdrStates(cached_state->m_modified, hdr_guard_fn, system_settings_touched)}; result != RevertResult::Ok) { + if (const auto result {revertModifiedHdrStates(modified_state, hdr_guard_fn, system_settings_touched)}; result != RevertResult::Ok) { // Error already logged return result; } DdGuardFn mode_guard_fn {noopFn}; boost::scope::scope_exit mode_guard {mode_guard_fn}; - if (const auto result {revertModifiedDisplayModes(cached_state->m_modified, mode_guard_fn, system_settings_touched)}; result != RevertResult::Ok) { + if (const auto result {revertModifiedDisplayModes(modified_state, mode_guard_fn, system_settings_touched)}; result != RevertResult::Ok) { // Error already logged return result; } DdGuardFn primary_guard_fn {noopFn}; boost::scope::scope_exit primary_guard {primary_guard_fn}; - if (const auto result {revertModifiedPrimaryDevice(cached_state->m_modified, primary_guard_fn, system_settings_touched)}; result != RevertResult::Ok) { + if (const auto result {revertModifiedPrimaryDevice(modified_state, primary_guard_fn, system_settings_touched)}; result != RevertResult::Ok) { // Error already logged return result; } + if (devices_missing) { + // Keep the full record so that the unplugged displays can still be reverted if they return. + hdr_guard.set_active(false); + mode_guard.set_active(false); + primary_guard.set_active(false); + return RevertResult::Ok; + } + auto cleared_data {*cached_state}; cleared_data.m_modified = {cleared_data.m_modified.m_topology}; if (!m_persistence_state->persistState(cleared_data)) { diff --git a/tests/unit/windows/test_settings_manager_revert.cpp b/tests/unit/windows/test_settings_manager_revert.cpp index 095cf03e..fc30ff7e 100644 --- a/tests/unit/windows/test_settings_manager_revert.cpp +++ b/tests/unit/windows/test_settings_manager_revert.cpp @@ -340,6 +340,7 @@ TEST_F_S_MOCKED(RevertModifiedSettings, FailedToSetModifiedTopology) { EXPECT_CALL(*m_dd_api, setTopology(ut_consts::SDCS_FULL->m_modified.m_topology)) .Times(1) .WillOnce(Return(false)); + EXPECT_CALL(*m_dd_api, enumAvailableDevices()).WillOnce(Return(CURRENT_DEVICES)); expectedDefaultTopologyGuardCall(sequence); expectedHdrWorkaroundCalls(sequence); diff --git a/tests/unit/windows/test_settings_manager_undock.cpp b/tests/unit/windows/test_settings_manager_undock.cpp index a24044ee..5901a369 100644 --- a/tests/unit/windows/test_settings_manager_undock.cpp +++ b/tests/unit/windows/test_settings_manager_undock.cpp @@ -29,6 +29,7 @@ namespace { bool fail_cleanup {}; bool fail_clear {}; bool lie_about_cleanup {}; + bool split_clones {}; int clears {}; std::shared_ptr> api = std::make_shared>(); std::shared_ptr> persistence = std::make_shared>(); @@ -57,6 +58,11 @@ namespace { if (fail_activation && ids.contains("panel")) { return false; } + if (split_clones && ids.contains("left") && ids.contains("right") && std::ranges::none_of(target, [](const auto &group) { + return std::ranges::find(group, "left") != group.end() && std::ranges::find(group, "right") != group.end(); + })) { + return false; // The adapter cannot drive both members of the clone group from separate sources. + } if (fail_cleanup && !ids.contains("stream")) { return false; } @@ -216,3 +222,46 @@ TEST_F(UndockRecovery, CleanupRequiresReadback) { EXPECT_EQ(clears, 0); EXPECT_TRUE(win_utils::flattenTopology(active).contains("stream")); } + +TEST_F(UndockRecovery, UnpluggedDisplayInModifiedTopologyDoesNotBlockRecovery) { + const DisplayMode dock_mode {{3840, 2160}, {60, 1}}; + const DisplayMode stream_mode {{1920, 1080}, {60, 1}}; + state.m_modified = {{{"dock"}, {"stream"}}, {{"dock", dock_mode}, {"stream", stream_mode}}, {{"dock", HdrState::Enabled}, {"stream", HdrState::Disabled}}, "dock"}; + std::vector mode_writes; + std::vector hdr_writes; + init(); + ON_CALL(*api, setDisplayModes(_)).WillByDefault([&mode_writes](const DeviceDisplayModeMap &modes) { + mode_writes.push_back(modes); + return true; + }); + ON_CALL(*api, setHdrStates(_)).WillByDefault([&hdr_writes](const HdrStateMap &states) { + hdr_writes.push_back(states); + return true; + }); + EXPECT_CALL(*api, setAsPrimary(_)).Times(0); + + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::SwitchingTopologyFailed); + EXPECT_EQ(clears, 0); + EXPECT_EQ(active, (ActiveTopology {{"stream"}})); + ASSERT_FALSE(mode_writes.empty()); + EXPECT_EQ(mode_writes.front(), (DeviceDisplayModeMap {{"stream", stream_mode}})); + ASSERT_FALSE(hdr_writes.empty()); + EXPECT_EQ(hdr_writes.front(), (HdrStateMap {{"stream", HdrState::Disabled}})); + + openLid(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"panel"}})); + EXPECT_EQ(clears, 1); +} + +TEST_F(UndockRecovery, StagingKeepsSurvivingCloneGroup) { + state.m_initial.m_topology.push_back({"left", "right"}); + devices.push_back({.m_device_id = "left"}); + devices.push_back({.m_device_id = "right"}); + split_clones = true; + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"left", "right"}})); + ASSERT_GE(writes.size(), 3u); + EXPECT_EQ(writes[writes.size() - 2], (ActiveTopology {{"stream"}, {"left", "right"}})); +} From b213c339ef5ea4c4cf0fede310e87cc59636bff6 Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Tue, 29 Sep 2026 09:15:41 -0500 Subject: [PATCH 06/12] fix(windows): keep unplugged displays' pending settings and clone groups during recovery Retain the original mode, HDR and primary settings of displays that are still unplugged after a topology recovery, so redocking can restore them. Extend an active clone member's group in place while staging recovery instead of adding the other members as separate sources. --- src/windows/settings_manager_revert.cpp | 79 ++++++++++++++++++- .../windows/test_settings_manager_undock.cpp | 38 +++++++++ 2 files changed, 113 insertions(+), 4 deletions(-) diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index 62cc3d12..40a6f6d1 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -8,6 +8,7 @@ // system includes #include #include +#include // local includes #include "display_device/logging.h" @@ -45,6 +46,73 @@ namespace display_device { } } + /** + * @brief Add the groups of a target topology to a staged topology, keeping clone groups intact. + * + * A target group that shares a device with a staged group extends that group in place instead of + * being added separately. This way the staged topology never needs more sources than the target does. + * @param target Groups that must be active once staging is done. + * @param staged Topology to extend. + */ + void stageRecoveryGroups(const ActiveTopology &target, ActiveTopology &staged) { + for (const auto &group : target) { + const auto in_group {[&group](const auto &id) { + return std::ranges::find(group, id) != group.end(); + }}; + const auto host {std::ranges::find_if(staged, [&in_group](const auto &staged_group) { + return std::ranges::any_of(staged_group, in_group); + })}; + if (host == staged.end()) { + staged.push_back(group); + continue; + } + + for (const auto &id : group) { + if (std::ranges::find(*host, id) == host->end()) { + host->push_back(id); + } + } + for (auto &other : staged) { + if (&other != &*host) { + std::erase_if(other, in_group); + } + } + std::erase_if(staged, [](const auto &staged_group) { + return staged_group.empty(); + }); + } + } + + /** + * @brief Keep only the pending settings of original devices that are currently unavailable. + * + * These settings cannot be restored until the devices return, so they must outlive a topology recovery. + * @param state State to filter. + * @param available Currently enumerated device IDs. + * @return State with only the settings of unavailable original devices, or nothing if there are none. + */ + std::optional keepUnavailableSettings(const SingleDisplayConfigState &state, const StringSet &available) { + const auto initial {win_utils::flattenTopology(state.m_initial.m_topology)}; + const auto is_pending {[&initial, &available](const auto &id) { + return initial.contains(id) && !available.contains(id); + }}; + + SingleDisplayConfigState result {state}; + std::erase_if(result.m_modified.m_original_modes, [&is_pending](const auto &entry) { + return !is_pending(entry.first); + }); + std::erase_if(result.m_modified.m_original_hdr_states, [&is_pending](const auto &entry) { + return !is_pending(entry.first); + }); + if (!is_pending(result.m_modified.m_original_primary_device)) { + result.m_modified.m_original_primary_device.clear(); + } + if (!result.m_modified.hasModifications()) { + return std::nullopt; + } + return result; + } + /** * @brief Collect the IDs of the enumerated devices. * @param devices Currently enumerated devices. @@ -138,15 +206,19 @@ namespace display_device { const bool is_topology_the_same {m_dd_api->isTopologyTheSame(current_topology, cached_state->m_initial.m_topology)}; const bool need_to_switch_topology {!is_topology_the_same || switched_to_modified_topology}; system_settings_touched = system_settings_touched || !is_topology_the_same; + std::optional pending_state; 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); if (!recoverMissingTopology(current_topology)) { return RevertResult::SwitchingTopologyFailed; } + + // Original devices that are still unplugged keep their settings until they return. + pending_state = keepUnavailableSettings(*cached_state, toDeviceIds(m_dd_api->enumAvailableDevices())); } - if (!m_persistence_state->persistState(std::nullopt)) { + if (!m_persistence_state->persistState(pending_state)) { DD_LOG(error) << "Failed to save reverted settings! Undoing initial topology changes..."; return RevertResult::PersistenceSaveFailed; } @@ -194,10 +266,9 @@ namespace display_device { appendRecoveryGroups(current_topology, available, activated, target, included); // Preserve unrelated displays activated by the user. // First activate the replacement without switching off the current output. - // Keep the target's groups intact, since cloned displays share a source. + // Keep the target's groups intact, even when a member is already active, since cloned displays share a source. ActiveTopology staged {current_topology}; - auto staged_ids {win_utils::flattenTopology(staged)}; - appendRecoveryGroups(target, available, {}, staged, staged_ids); + stageRecoveryGroups(target, staged); if (!m_dd_api->setTopology(staged)) { return false; } diff --git a/tests/unit/windows/test_settings_manager_undock.cpp b/tests/unit/windows/test_settings_manager_undock.cpp index 5901a369..bb8710d8 100644 --- a/tests/unit/windows/test_settings_manager_undock.cpp +++ b/tests/unit/windows/test_settings_manager_undock.cpp @@ -251,9 +251,47 @@ TEST_F(UndockRecovery, UnpluggedDisplayInModifiedTopologyDoesNotBlockRecovery) { openLid(); EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); EXPECT_EQ(active, (ActiveTopology {{"panel"}})); + EXPECT_EQ(clears, 0); // The dock display still has settings to restore. + + // Redocking restores what the unplugged display had before the stream. + devices.push_back({.m_device_id = "dock"}); + mode_writes.clear(); + hdr_writes.clear(); + ::testing::Mock::VerifyAndClearExpectations(api.get()); + EXPECT_CALL(*api, setAsPrimary("dock")).WillOnce(Return(true)); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + ASSERT_FALSE(mode_writes.empty()); + EXPECT_EQ(mode_writes.front(), (DeviceDisplayModeMap {{"dock", dock_mode}})); + ASSERT_FALSE(hdr_writes.empty()); + EXPECT_EQ(hdr_writes.front(), (HdrStateMap {{"dock", HdrState::Enabled}})); + EXPECT_EQ(active, (ActiveTopology {{"dock"}})); + EXPECT_EQ(clears, 1); +} + +TEST_F(UndockRecovery, DoesNotRetainSettingsOfStreamOnlyDisplay) { + const DisplayMode stream_mode {{1920, 1080}, {60, 1}}; + state.m_modified = {{{"stream"}}, {{"stream", stream_mode}}, {}, {}}; + openLid(); + init(); + ON_CALL(*api, setDisplayModes(_)).WillByDefault(Return(true)); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"panel"}})); EXPECT_EQ(clears, 1); } +TEST_F(UndockRecovery, StagingKeepsCloneGroupWithActiveMember) { + state.m_initial.m_topology.push_back({"left", "right"}); + devices.push_back({.m_device_id = "left"}); + devices.push_back({.m_device_id = "right"}); + active = {{"left"}, {"stream"}}; + split_clones = true; + init(); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(active, (ActiveTopology {{"left", "right"}})); + ASSERT_GE(writes.size(), 3u); + EXPECT_EQ(writes[writes.size() - 2], (ActiveTopology {{"left", "right"}, {"stream"}})); +} + TEST_F(UndockRecovery, StagingKeepsSurvivingCloneGroup) { state.m_initial.m_topology.push_back({"left", "right"}); devices.push_back({.m_device_id = "left"}); From 65aa5d082be66f55798ac50f36629bf6b5feb01c Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Tue, 29 Sep 2026 09:37:11 -0500 Subject: [PATCH 07/12] fix(windows): keep pending settings usable for new applies and reverts while a display is away Base the retained record on the recovered layout so applying settings still works, carry the pending entries through an apply, and stage clone groups within the two-display limit. --- .../display_device/windows/settings_utils.h | 10 ++ src/windows/settings_manager_apply.cpp | 42 ++++++- src/windows/settings_manager_revert.cpp | 113 ++++++++++++++---- src/windows/settings_utils.cpp | 9 ++ .../windows/test_settings_manager_undock.cpp | 37 +++++- 5 files changed, 181 insertions(+), 30 deletions(-) diff --git a/src/windows/include/display_device/windows/settings_utils.h b/src/windows/include/display_device/windows/settings_utils.h index 36e63bb7..2bcd5f2b 100644 --- a/src/windows/include/display_device/windows/settings_utils.h +++ b/src/windows/include/display_device/windows/settings_utils.h @@ -27,6 +27,16 @@ namespace display_device::win_utils { */ StringSet flattenTopology(const ActiveTopology &topology); + /** + * @brief Get the ids of the enumerated devices. + * @param devices Enumerated devices. + * @return Device ids of the enumerated devices. + * @examples + * const auto device_ids { getDeviceIds(win_dd.enumAvailableDevices()) }; + * @examples_end + */ + StringSet getDeviceIds(const EnumeratedDeviceList &devices); + /** * @brief Create extended topology from all the available devices. * @param win_dd Interface for interacting with the OS. diff --git a/src/windows/settings_manager_apply.cpp b/src/windows/settings_manager_apply.cpp index 1bddf634..b7d87e98 100644 --- a/src/windows/settings_manager_apply.cpp +++ b/src/windows/settings_manager_apply.cpp @@ -22,6 +22,27 @@ namespace display_device { void noopFn() { // Intentionally empty guard callback. } + + /** + * @brief Split off the cached entries of devices that are not part of the topology being applied. + * + * Such devices, for example an unplugged display, cannot be restored now, so their entries must be carried over untouched. + * @param entries Cached per-device entries. The entries of inactive devices are removed. + * @param active Device IDs of the topology being applied. + * @return The removed entries. + */ + template + Entries takeInactive(Entries &entries, const StringSet &active) { + Entries inactive; + std::erase_if(entries, [&](const auto &entry) { + if (active.contains(entry.first)) { + return false; + } + inactive.insert(entry); + return true; + }); + return inactive; + } } // namespace SettingsManager::ApplyResult SettingsManager::applySettings(const SingleDisplayConfiguration &config) { @@ -206,7 +227,13 @@ namespace display_device { bool SettingsManager::preparePrimaryDevice(const SingleDisplayConfiguration &config, const std::string &device_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched) { const auto &cached_state {m_persistence_state->getState()}; - const auto cached_primary_device {cached_state ? cached_state->m_modified.m_original_primary_device : std::string {}}; + auto cached_primary_device {cached_state ? cached_state->m_modified.m_original_primary_device : std::string {}}; + std::string pending_primary_device; + if (!cached_primary_device.empty() && !win_utils::flattenTopology(new_state.m_modified.m_topology).contains(cached_primary_device)) { + // The original primary device is inactive (e.g. unplugged), so it can only be restored once it returns. + pending_primary_device = std::move(cached_primary_device); + cached_primary_device.clear(); + } const bool ensure_primary {config.m_device_prep == SingleDisplayConfiguration::DevicePreparation::EnsurePrimary}; const bool might_need_to_restore {!cached_primary_device.empty()}; @@ -246,7 +273,7 @@ namespace display_device { } // Here we preserve the data from persistence (unless there's none) as in the end that is what we want to go back to. - new_state.m_modified.m_original_primary_device = original_primary_device; + new_state.m_modified.m_original_primary_device = pending_primary_device.empty() ? original_primary_device : pending_primary_device; return true; } @@ -255,12 +282,14 @@ namespace display_device { return false; } + new_state.m_modified.m_original_primary_device = pending_primary_device; return true; } bool SettingsManager::prepareDisplayModes(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const StringSet &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched) { const auto &cached_state {m_persistence_state->getState()}; - const auto cached_display_modes {cached_state ? cached_state->m_modified.m_original_modes : DeviceDisplayModeMap {}}; + auto cached_display_modes {cached_state ? cached_state->m_modified.m_original_modes : DeviceDisplayModeMap {}}; + const auto pending_display_modes {takeInactive(cached_display_modes, win_utils::flattenTopology(new_state.m_modified.m_topology))}; const bool change_required {config.m_resolution || config.m_refresh_rate}; const bool might_need_to_restore {!cached_display_modes.empty()}; @@ -306,6 +335,7 @@ namespace display_device { // Here we preserve the data from persistence (unless there's none) as in the end that is what we want to go back to. new_state.m_modified.m_original_modes = original_display_modes; + new_state.m_modified.m_original_modes.insert(pending_display_modes.begin(), pending_display_modes.end()); return true; } @@ -314,12 +344,14 @@ namespace display_device { return false; } + new_state.m_modified.m_original_modes = pending_display_modes; return true; } [[nodiscard]] bool SettingsManager::prepareHdrStates(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const StringSet &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched) { const auto &cached_state {m_persistence_state->getState()}; - const auto cached_hdr_states {cached_state ? cached_state->m_modified.m_original_hdr_states : HdrStateMap {}}; + auto cached_hdr_states {cached_state ? cached_state->m_modified.m_original_hdr_states : HdrStateMap {}}; + const auto pending_hdr_states {takeInactive(cached_hdr_states, win_utils::flattenTopology(new_state.m_modified.m_topology))}; const bool change_required {config.m_hdr_state}; const bool might_need_to_restore {!cached_hdr_states.empty()}; @@ -360,6 +392,7 @@ namespace display_device { // Here we preserve the data from persistence (unless there's none) as in the end that is what we want to go back to. new_state.m_modified.m_original_hdr_states = original_hdr_states; + new_state.m_modified.m_original_hdr_states.insert(pending_hdr_states.begin(), pending_hdr_states.end()); return true; } @@ -368,6 +401,7 @@ namespace display_device { return false; } + new_state.m_modified.m_original_hdr_states = pending_hdr_states; return true; } } // namespace display_device diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index 40a6f6d1..fcafa6ad 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -50,7 +50,8 @@ namespace display_device { * @brief Add the groups of a target topology to a staged topology, keeping clone groups intact. * * A target group that shares a device with a staged group extends that group in place instead of - * being added separately. This way the staged topology never needs more sources than the target does. + * being added separately. This way the staged topology needs no more sources than necessary. + * Devices that are already staged stay where they are. * @param target Groups that must be active once staging is done. * @param staged Topology to extend. */ @@ -67,19 +68,23 @@ namespace display_device { continue; } + std::vector missing; for (const auto &id : group) { - if (std::ranges::find(*host, id) == host->end()) { - host->push_back(id); + if (std::ranges::none_of(staged, [&id](const auto &staged_group) { + return std::ranges::find(staged_group, id) != staged_group.end(); + })) { + missing.push_back(id); } } - for (auto &other : staged) { - if (&other != &*host) { - std::erase_if(other, in_group); + if (host->size() + missing.size() > 2) { + // Windows cannot clone more than two displays, so the extra members are staged on their own. + if (!missing.empty()) { + staged.push_back(std::move(missing)); } + continue; } - std::erase_if(staged, [](const auto &staged_group) { - return staged_group.empty(); - }); + + host->insert(host->end(), missing.begin(), missing.end()); } } @@ -87,11 +92,14 @@ namespace display_device { * @brief Keep only the pending settings of original devices that are currently unavailable. * * These settings cannot be restored until the devices return, so they must outlive a topology recovery. + * The modified topology is reduced to those devices, as they are all that is needed to restore the settings. + * The initial topology is extended with the recovered one, so that new settings can still be applied while the devices are away. * @param state State to filter. * @param available Currently enumerated device IDs. + * @param recovered_topology Topology that replaced the original one. * @return State with only the settings of unavailable original devices, or nothing if there are none. */ - std::optional keepUnavailableSettings(const SingleDisplayConfigState &state, const StringSet &available) { + std::optional keepUnavailableSettings(const SingleDisplayConfigState &state, const StringSet &available, const ActiveTopology &recovered_topology) { const auto initial {win_utils::flattenTopology(state.m_initial.m_topology)}; const auto is_pending {[&initial, &available](const auto &id) { return initial.contains(id) && !available.contains(id); @@ -110,20 +118,59 @@ namespace display_device { if (!result.m_modified.hasModifications()) { return std::nullopt; } + + // Only the returning devices need to be active to restore their settings. + result.m_modified.m_topology.clear(); + StringSet included; + for (const auto &group : state.m_initial.m_topology) { + std::vector pending_group; + for (const auto &id : group) { + if (is_pending(id) && included.insert(id).second) { + pending_group.push_back(id); + } + } + if (!pending_group.empty()) { + result.m_modified.m_topology.push_back(std::move(pending_group)); + } + } + + // The original devices alone would leave nothing to build on until one of them returns. + StringSet known {initial}; + for (const auto &group : recovered_topology) { + std::vector recovered_group; + for (const auto &id : group) { + if (known.insert(id).second) { + recovered_group.push_back(id); + } + } + if (!recovered_group.empty()) { + result.m_initial.m_topology.push_back(std::move(recovered_group)); + } + } return result; } /** - * @brief Collect the IDs of the enumerated devices. - * @param devices Currently enumerated devices. - * @return Set of available device IDs. + * @brief Find the devices that have settings recorded but are not part of the topology. + * @param modified Modified settings to inspect. + * @return IDs of the devices outside of the modified topology. */ - StringSet toDeviceIds(const EnumeratedDeviceList &devices) { - StringSet ids; - for (const auto &device : devices) { - ids.insert(device.m_device_id); + StringSet findCarriedDevices(const SingleDisplayConfigState::Modified &modified) { + const auto topology_ids {win_utils::flattenTopology(modified.m_topology)}; + StringSet carried; + const auto add {[&](const std::string &id) { + if (!id.empty() && !topology_ids.contains(id)) { + carried.insert(id); + } + }}; + for (const auto &entry : modified.m_original_modes) { + add(entry.first); } - return ids; + for (const auto &entry : modified.m_original_hdr_states) { + add(entry.first); + } + add(modified.m_original_primary_device); + return carried; } /** @@ -215,7 +262,7 @@ namespace display_device { } // Original devices that are still unplugged keep their settings until they return. - pending_state = keepUnavailableSettings(*cached_state, toDeviceIds(m_dd_api->enumAvailableDevices())); + pending_state = keepUnavailableSettings(*cached_state, win_utils::getDeviceIds(m_dd_api->enumAvailableDevices()), m_dd_api->getCurrentTopology()); } if (!m_persistence_state->persistState(pending_state)) { @@ -237,7 +284,7 @@ namespace display_device { const auto initial {win_utils::flattenTopology(state.m_initial.m_topology)}; const auto modified {win_utils::flattenTopology(state.m_modified.m_topology)}; const auto devices {m_dd_api->enumAvailableDevices()}; - const auto available {toDeviceIds(devices)}; + const auto available {win_utils::getDeviceIds(devices)}; if (available.empty() || std::ranges::all_of(initial, [&available](const auto &id) { return available.contains(id); })) { @@ -379,21 +426,37 @@ namespace display_device { } auto modified_state {cached_state->m_modified}; + bool keep_record {false}; + if (!findCarriedDevices(modified_state).empty()) { + // Settings carried over from an earlier session (e.g. of a display that was unplugged) can only be restored + // once their devices are active, so activate those that are back and postpone the rest. + const auto available {win_utils::getDeviceIds(m_dd_api->enumAvailableDevices())}; + if (!available.empty()) { + for (const auto &id : findCarriedDevices(modified_state)) { + if (available.contains(id)) { + modified_state.m_topology.push_back({id}); + } else { + keep_record = true; + } + } + modified_state = keepAvailableDevices(modified_state, available); + } + } bool is_topology_the_same {m_dd_api->isTopologyTheSame(current_topology, modified_state.m_topology)}; system_settings_touched = !is_topology_the_same; - bool devices_missing {false}; if (!is_topology_the_same && !m_dd_api->setTopology(modified_state.m_topology)) { DD_LOG(error) << "Failed to change topology to:\n" << toJson(modified_state.m_topology); - const auto available {toDeviceIds(m_dd_api->enumAvailableDevices())}; + const auto available {win_utils::getDeviceIds(m_dd_api->enumAvailableDevices())}; const auto modified_ids {win_utils::flattenTopology(modified_state.m_topology)}; - devices_missing = !available.empty() && std::ranges::any_of(modified_ids, [&available](const auto &id) { + const bool devices_missing {!available.empty() && std::ranges::any_of(modified_ids, [&available](const auto &id) { return !available.contains(id); - }); + })}; if (!devices_missing) { return RevertResult::SwitchingTopologyFailed; } + keep_record = true; // A display from the modified topology has been unplugged. Revert the settings of the remaining // displays so that the initial topology, or its recovery, can still be applied. @@ -436,7 +499,7 @@ namespace display_device { return result; } - if (devices_missing) { + if (keep_record) { // Keep the full record so that the unplugged displays can still be reverted if they return. hdr_guard.set_active(false); mode_guard.set_active(false); diff --git a/src/windows/settings_utils.cpp b/src/windows/settings_utils.cpp index a2798bc9..e978f03c 100644 --- a/src/windows/settings_utils.cpp +++ b/src/windows/settings_utils.cpp @@ -98,6 +98,15 @@ namespace display_device::win_utils { return flattened_topology; } + StringSet getDeviceIds(const EnumeratedDeviceList &devices) { + StringSet ids; + for (const auto &device : devices) { + ids.insert(device.m_device_id); + } + + return ids; + } + ActiveTopology createFullExtendedTopology(const WinDisplayDeviceInterface &win_dd) { const auto devices {win_dd.enumAvailableDevices()}; if (devices.empty()) { diff --git a/tests/unit/windows/test_settings_manager_undock.cpp b/tests/unit/windows/test_settings_manager_undock.cpp index bb8710d8..16c6f060 100644 --- a/tests/unit/windows/test_settings_manager_undock.cpp +++ b/tests/unit/windows/test_settings_manager_undock.cpp @@ -264,7 +264,7 @@ TEST_F(UndockRecovery, UnpluggedDisplayInModifiedTopologyDoesNotBlockRecovery) { EXPECT_EQ(mode_writes.front(), (DeviceDisplayModeMap {{"dock", dock_mode}})); ASSERT_FALSE(hdr_writes.empty()); EXPECT_EQ(hdr_writes.front(), (HdrStateMap {{"dock", HdrState::Enabled}})); - EXPECT_EQ(active, (ActiveTopology {{"dock"}})); + EXPECT_EQ(win_utils::flattenTopology(active), (StringSet {"dock", "panel"})); EXPECT_EQ(clears, 1); } @@ -279,6 +279,41 @@ TEST_F(UndockRecovery, DoesNotRetainSettingsOfStreamOnlyDisplay) { EXPECT_EQ(clears, 1); } +TEST_F(UndockRecovery, PendingSettingsSurviveApplyWhileStillUndocked) { + const DisplayMode dock_mode {{3840, 2160}, {60, 1}}; + state.m_initial = {{{"dock"}, {"panel"}}, {"dock"}}; // As retained after a recovery to the panel. + state.m_modified = {{{"dock"}}, {{"dock", dock_mode}}, {{"dock", HdrState::Enabled}}, "dock"}; + active = {{"panel"}}; + devices = {{.m_device_id = "panel", .m_info = EnumeratedDevice::Info {.m_primary = true}, .m_is_internal = true}}; + std::vector mode_writes; + init(); + ON_CALL(*api, setDisplayModes(_)).WillByDefault([&mode_writes](const DeviceDisplayModeMap &modes) { + mode_writes.push_back(modes); + return true; + }); + ON_CALL(*api, setHdrStates(_)).WillByDefault(Return(true)); + ON_CALL(*api, setAsPrimary(_)).WillByDefault(Return(true)); + ON_CALL(*api, isPrimary("panel")).WillByDefault(Return(true)); + ON_CALL(*api, getCurrentDisplayModes(_)).WillByDefault(Return(DeviceDisplayModeMap {{"panel", {{1920, 1080}, {60, 1}}}})); + ON_CALL(*api, getCurrentHdrStates(_)).WillByDefault(Return(HdrStateMap {{"panel", HdrState::Disabled}})); + + SingleDisplayConfiguration config; + config.m_device_id = "panel"; + config.m_device_prep = SingleDisplayConfiguration::DevicePreparation::VerifyOnly; + EXPECT_CALL(*api, setAsPrimary("dock")).Times(0); + EXPECT_EQ(manager->applySettings(config), SettingsManager::ApplyResult::Ok); + EXPECT_TRUE(mode_writes.empty()); + + // The dock returns and gets its own settings back. + devices.push_back({.m_device_id = "dock"}); + ::testing::Mock::VerifyAndClearExpectations(api.get()); + EXPECT_CALL(*api, setAsPrimary("dock")).WillOnce(Return(true)); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + ASSERT_FALSE(mode_writes.empty()); + EXPECT_EQ(mode_writes.front(), (DeviceDisplayModeMap {{"dock", dock_mode}})); + EXPECT_EQ(clears, 1); +} + TEST_F(UndockRecovery, StagingKeepsCloneGroupWithActiveMember) { state.m_initial.m_topology.push_back({"left", "right"}); devices.push_back({.m_device_id = "left"}); From ba4cef07fd910eb92f61007f125f20497c2cbfa4 Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Tue, 29 Sep 2026 09:44:56 -0500 Subject: [PATCH 08/12] fix(windows): only carry settings of original devices left out of the modified topology --- src/windows/settings_manager_revert.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index fcafa6ad..fb60a493 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -151,15 +151,19 @@ namespace display_device { } /** - * @brief Find the devices that have settings recorded but are not part of the topology. - * @param modified Modified settings to inspect. - * @return IDs of the devices outside of the modified topology. + * @brief Find the original devices that have settings recorded but are left out of the modified topology. + * + * These are carried over from an earlier session, e.g. the settings of a display that was unplugged at the time. + * @param state State to inspect. + * @return IDs of the devices that are in the initial topology only. */ - StringSet findCarriedDevices(const SingleDisplayConfigState::Modified &modified) { + StringSet findCarriedDevices(const SingleDisplayConfigState &state) { + const auto &modified {state.m_modified}; + const auto initial_ids {win_utils::flattenTopology(state.m_initial.m_topology)}; const auto topology_ids {win_utils::flattenTopology(modified.m_topology)}; StringSet carried; const auto add {[&](const std::string &id) { - if (!id.empty() && !topology_ids.contains(id)) { + if (initial_ids.contains(id) && !topology_ids.contains(id)) { carried.insert(id); } }}; @@ -427,12 +431,12 @@ namespace display_device { auto modified_state {cached_state->m_modified}; bool keep_record {false}; - if (!findCarriedDevices(modified_state).empty()) { + if (!findCarriedDevices(*cached_state).empty()) { // Settings carried over from an earlier session (e.g. of a display that was unplugged) can only be restored // once their devices are active, so activate those that are back and postpone the rest. const auto available {win_utils::getDeviceIds(m_dd_api->enumAvailableDevices())}; if (!available.empty()) { - for (const auto &id : findCarriedDevices(modified_state)) { + for (const auto &id : findCarriedDevices(*cached_state)) { if (available.contains(id)) { modified_state.m_topology.push_back({id}); } else { From 94247bf5512c34b54116849a68b903cca8d9028b Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Tue, 29 Sep 2026 10:59:22 -0500 Subject: [PATCH 09/12] refactor: address static analysis findings in undock recovery --- src/windows/settings_manager_apply.cpp | 2 +- src/windows/settings_manager_revert.cpp | 101 +++++++++++++++--------- 2 files changed, 65 insertions(+), 38 deletions(-) diff --git a/src/windows/settings_manager_apply.cpp b/src/windows/settings_manager_apply.cpp index b7d87e98..426d67e7 100644 --- a/src/windows/settings_manager_apply.cpp +++ b/src/windows/settings_manager_apply.cpp @@ -34,7 +34,7 @@ namespace display_device { template Entries takeInactive(Entries &entries, const StringSet &active) { Entries inactive; - std::erase_if(entries, [&](const auto &entry) { + std::erase_if(entries, [&active, &inactive](const auto &entry) { if (active.contains(entry.first)) { return false; } diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index fb60a493..0ce0b7df 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -9,6 +9,7 @@ #include #include #include +#include // local includes #include "display_device/logging.h" @@ -88,6 +89,17 @@ namespace display_device { } } + /** + * @brief Check whether an original device is away, so that its settings have to wait for its return. + * @param id Device ID to check. + * @param initial IDs of the devices in the initial topology. + * @param available Currently enumerated device IDs. + * @return True if the device is part of the initial topology and is unavailable. + */ + bool isPendingDevice(const std::string &id, const StringSet &initial, const StringSet &available) { + return initial.contains(id) && !available.contains(id); + } + /** * @brief Keep only the pending settings of original devices that are currently unavailable. * @@ -101,18 +113,15 @@ namespace display_device { */ std::optional keepUnavailableSettings(const SingleDisplayConfigState &state, const StringSet &available, const ActiveTopology &recovered_topology) { const auto initial {win_utils::flattenTopology(state.m_initial.m_topology)}; - const auto is_pending {[&initial, &available](const auto &id) { - return initial.contains(id) && !available.contains(id); - }}; SingleDisplayConfigState result {state}; - std::erase_if(result.m_modified.m_original_modes, [&is_pending](const auto &entry) { - return !is_pending(entry.first); + std::erase_if(result.m_modified.m_original_modes, [&initial, &available](const auto &entry) { + return !isPendingDevice(entry.first, initial, available); }); - std::erase_if(result.m_modified.m_original_hdr_states, [&is_pending](const auto &entry) { - return !is_pending(entry.first); + std::erase_if(result.m_modified.m_original_hdr_states, [&initial, &available](const auto &entry) { + return !isPendingDevice(entry.first, initial, available); }); - if (!is_pending(result.m_modified.m_original_primary_device)) { + if (!isPendingDevice(result.m_modified.m_original_primary_device, initial, available)) { result.m_modified.m_original_primary_device.clear(); } if (!result.m_modified.hasModifications()) { @@ -125,7 +134,7 @@ namespace display_device { for (const auto &group : state.m_initial.m_topology) { std::vector pending_group; for (const auto &id : group) { - if (is_pending(id) && included.insert(id).second) { + if (isPendingDevice(id, initial, available) && included.insert(id).second) { pending_group.push_back(id); } } @@ -159,21 +168,23 @@ namespace display_device { */ StringSet findCarriedDevices(const SingleDisplayConfigState &state) { const auto &modified {state.m_modified}; + StringSet recorded; + for (const auto &id : modified.m_original_modes | std::views::keys) { + recorded.insert(id); + } + for (const auto &id : modified.m_original_hdr_states | std::views::keys) { + recorded.insert(id); + } + recorded.insert(modified.m_original_primary_device); + const auto initial_ids {win_utils::flattenTopology(state.m_initial.m_topology)}; const auto topology_ids {win_utils::flattenTopology(modified.m_topology)}; StringSet carried; - const auto add {[&](const std::string &id) { + for (const auto &id : recorded) { if (initial_ids.contains(id) && !topology_ids.contains(id)) { carried.insert(id); } - }}; - for (const auto &entry : modified.m_original_modes) { - add(entry.first); - } - for (const auto &entry : modified.m_original_hdr_states) { - add(entry.first); } - add(modified.m_original_primary_device); return carried; } @@ -199,6 +210,37 @@ namespace display_device { } return result; } + + /** + * @brief Activate the carried devices that are back and drop the settings of those that are not. + * @param dd_api Display device API. + * @param state Persisted state that carries the settings. + * @param modified_state Modified settings to prepare for the revert. + * @return True if some carried devices are still unavailable, so their settings have to be kept. + */ + bool prepareCarriedDevices(const WinDisplayDeviceInterface &dd_api, const SingleDisplayConfigState &state, SingleDisplayConfigState::Modified &modified_state) { + const auto carried {findCarriedDevices(state)}; + if (carried.empty()) { + return false; + } + + // Settings carried over from an earlier session can only be restored once their devices are active. + const auto available {win_utils::getDeviceIds(dd_api.enumAvailableDevices())}; + if (available.empty()) { + return false; + } + + bool some_unavailable {false}; + for (const auto &id : carried) { + if (available.contains(id)) { + modified_state.m_topology.push_back({id}); + } else { + some_unavailable = true; + } + } + modified_state = keepAvailableDevices(modified_state, available); + return some_unavailable; + } } // namespace SettingsManager::RevertResult SettingsManager::revertSettings() { @@ -430,22 +472,7 @@ namespace display_device { } auto modified_state {cached_state->m_modified}; - bool keep_record {false}; - if (!findCarriedDevices(*cached_state).empty()) { - // Settings carried over from an earlier session (e.g. of a display that was unplugged) can only be restored - // once their devices are active, so activate those that are back and postpone the rest. - const auto available {win_utils::getDeviceIds(m_dd_api->enumAvailableDevices())}; - if (!available.empty()) { - for (const auto &id : findCarriedDevices(*cached_state)) { - if (available.contains(id)) { - modified_state.m_topology.push_back({id}); - } else { - keep_record = true; - } - } - modified_state = keepAvailableDevices(modified_state, available); - } - } + bool keep_record {prepareCarriedDevices(*m_dd_api, *cached_state, modified_state)}; bool is_topology_the_same {m_dd_api->isTopologyTheSame(current_topology, modified_state.m_topology)}; system_settings_touched = !is_topology_the_same; if (!is_topology_the_same && !m_dd_api->setTopology(modified_state.m_topology)) { @@ -454,10 +481,10 @@ namespace display_device { const auto available {win_utils::getDeviceIds(m_dd_api->enumAvailableDevices())}; const auto modified_ids {win_utils::flattenTopology(modified_state.m_topology)}; - const bool devices_missing {!available.empty() && std::ranges::any_of(modified_ids, [&available](const auto &id) { - return !available.contains(id); - })}; - if (!devices_missing) { + if (const bool devices_missing {!available.empty() && std::ranges::any_of(modified_ids, [&available](const auto &id) { + return !available.contains(id); + })}; + !devices_missing) { return RevertResult::SwitchingTopologyFailed; } keep_record = true; From 4f45f1298d27df1b40a385e3a81f99aec60f65f5 Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Tue, 29 Sep 2026 11:12:21 -0500 Subject: [PATCH 10/12] refactor: declare the active devices in the readback check's init-statement --- src/windows/settings_manager_revert.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index 0ce0b7df..69f03263 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -365,8 +365,7 @@ namespace display_device { if (!m_dd_api->setTopology(staged)) { return false; } - const auto active {win_utils::flattenTopology(m_dd_api->getCurrentTopology())}; - if (!std::ranges::all_of(replacements, [&active](const auto &id) { + if (const auto active {win_utils::flattenTopology(m_dd_api->getCurrentTopology())}; !std::ranges::all_of(replacements, [&active](const auto &id) { return active.contains(id); })) { return false; From adada6ff231a7dc775248b56f862346c7d868435 Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Tue, 29 Sep 2026 14:03:11 -0500 Subject: [PATCH 11/12] fix(windows): preserve the surviving primary when the original one is away Track the display to restore while the original primary is unavailable, so an EnsurePrimary session can be reverted without leaving its display primary. --- .../include/display_device/windows/types.h | 1 + src/windows/json_serializer.cpp | 30 ++++++++++++++- src/windows/settings_manager_apply.cpp | 16 ++++++-- src/windows/settings_manager_revert.cpp | 5 ++- tests/unit/windows/test_json_converter.cpp | 1 + .../windows/test_settings_manager_undock.cpp | 37 +++++++++++++++++++ 6 files changed, 85 insertions(+), 5 deletions(-) diff --git a/src/windows/include/display_device/windows/types.h b/src/windows/include/display_device/windows/types.h index 1a072026..038b0cb6 100644 --- a/src/windows/include/display_device/windows/types.h +++ b/src/windows/include/display_device/windows/types.h @@ -146,6 +146,7 @@ namespace display_device { DeviceDisplayModeMap m_original_modes {}; ///< Original display modes before modification. HdrStateMap m_original_hdr_states {}; ///< Original HDR states before modification. std::string m_original_primary_device {}; ///< Original primary device before modification. + std::string m_interim_primary_device {}; ///< Primary device to restore while the original one is unavailable. /** * @brief Check if the changed topology has any other modifications. diff --git a/src/windows/json_serializer.cpp b/src/windows/json_serializer.cpp index c553dca3..e65a7c8d 100644 --- a/src/windows/json_serializer.cpp +++ b/src/windows/json_serializer.cpp @@ -13,7 +13,35 @@ namespace display_device { // Structs DD_JSON_DEFINE_SERIALIZE_STRUCT(DisplayMode, resolution, refresh_rate) DD_JSON_DEFINE_SERIALIZE_STRUCT(SingleDisplayConfigState::Initial, topology, primary_devices) - DD_JSON_DEFINE_SERIALIZE_STRUCT(SingleDisplayConfigState::Modified, topology, original_modes, original_hdr_states, original_primary_device) + + /** + * @brief Serialize the modified state, including the interim primary device when there is one. + * @param nlohmann_json_j JSON output object. + * @param nlohmann_json_t Modified state to serialize. + */ + void to_json(nlohmann::json &nlohmann_json_j, const SingleDisplayConfigState::Modified &nlohmann_json_t) { + DD_JSON_TO(topology) + DD_JSON_TO(original_modes) + DD_JSON_TO(original_hdr_states) + DD_JSON_TO(original_primary_device) + if (!nlohmann_json_t.m_interim_primary_device.empty()) { + DD_JSON_TO(interim_primary_device) + } + } + + /** + * @brief Deserialize the modified state, treating a missing interim primary device as none. + * @param nlohmann_json_j JSON input object. + * @param nlohmann_json_t Modified state to populate. + */ + void from_json(const nlohmann::json &nlohmann_json_j, SingleDisplayConfigState::Modified &nlohmann_json_t) { + DD_JSON_FROM(topology) + DD_JSON_FROM(original_modes) + DD_JSON_FROM(original_hdr_states) + DD_JSON_FROM(original_primary_device) + nlohmann_json_t.m_interim_primary_device = nlohmann_json_j.value("interim_primary_device", std::string {}); + } + DD_JSON_DEFINE_SERIALIZE_STRUCT(SingleDisplayConfigState, initial, modified) DD_JSON_DEFINE_SERIALIZE_STRUCT(WinWorkarounds, hdr_blank_delay) } // namespace display_device diff --git a/src/windows/settings_manager_apply.cpp b/src/windows/settings_manager_apply.cpp index 426d67e7..d68d2b09 100644 --- a/src/windows/settings_manager_apply.cpp +++ b/src/windows/settings_manager_apply.cpp @@ -228,11 +228,16 @@ namespace display_device { bool SettingsManager::preparePrimaryDevice(const SingleDisplayConfiguration &config, const std::string &device_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched) { const auto &cached_state {m_persistence_state->getState()}; auto cached_primary_device {cached_state ? cached_state->m_modified.m_original_primary_device : std::string {}}; + const auto cached_interim_device {cached_state ? cached_state->m_modified.m_interim_primary_device : std::string {}}; std::string pending_primary_device; - if (!cached_primary_device.empty() && !win_utils::flattenTopology(new_state.m_modified.m_topology).contains(cached_primary_device)) { + if (const auto active_devices {win_utils::flattenTopology(new_state.m_modified.m_topology)}; !cached_primary_device.empty() && !active_devices.contains(cached_primary_device)) { // The original primary device is inactive (e.g. unplugged), so it can only be restored once it returns. + // Until then, the device that was primary before this happened is the one to go back to. pending_primary_device = std::move(cached_primary_device); - cached_primary_device.clear(); + cached_primary_device = cached_interim_device; + if (!active_devices.contains(cached_primary_device)) { + cached_primary_device.clear(); + } } const bool ensure_primary {config.m_device_prep == SingleDisplayConfiguration::DevicePreparation::EnsurePrimary}; const bool might_need_to_restore {!cached_primary_device.empty()}; @@ -273,7 +278,12 @@ namespace display_device { } // Here we preserve the data from persistence (unless there's none) as in the end that is what we want to go back to. - new_state.m_modified.m_original_primary_device = pending_primary_device.empty() ? original_primary_device : pending_primary_device; + if (pending_primary_device.empty()) { + new_state.m_modified.m_original_primary_device = original_primary_device; + } else { + new_state.m_modified.m_original_primary_device = pending_primary_device; + new_state.m_modified.m_interim_primary_device = original_primary_device; + } return true; } diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index 69f03263..e28bf9b8 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -124,6 +124,7 @@ namespace display_device { if (!isPendingDevice(result.m_modified.m_original_primary_device, initial, available)) { result.m_modified.m_original_primary_device.clear(); } + result.m_modified.m_interim_primary_device.clear(); // Already restored by the revert that led here. if (!result.m_modified.hasModifications()) { return std::nullopt; } @@ -206,8 +207,10 @@ namespace display_device { return !available.contains(entry.first); }); if (!available.contains(result.m_original_primary_device)) { - result.m_original_primary_device.clear(); + // The interim device is what was primary before the original one went away. + result.m_original_primary_device = available.contains(result.m_interim_primary_device) ? result.m_interim_primary_device : std::string {}; } + result.m_interim_primary_device.clear(); return result; } diff --git a/tests/unit/windows/test_json_converter.cpp b/tests/unit/windows/test_json_converter.cpp index 5f2afd42..67812e76 100644 --- a/tests/unit/windows/test_json_converter.cpp +++ b/tests/unit/windows/test_json_converter.cpp @@ -45,6 +45,7 @@ TEST_F_S(SingleDisplayConfigState) { executeTestCase(display_device::SingleDisplayConfigState {}, R"({"initial":{"primary_devices":[],"topology":[]},"modified":{"original_hdr_states":{},"original_modes":{},"original_primary_device":"","topology":[]}})"); executeTestCase(valid_input, R"({"initial":{"primary_devices":["DeviceId1"],"topology":[["DeviceId1"]]},"modified":{"original_hdr_states":{"DeviceId2":"Disabled"},"original_modes":{"DeviceId2":{"refresh_rate":{"denominator":1,"numerator":120},"resolution":{"height":1080,"width":1920}}},"original_primary_device":"DeviceId2","topology":[["DeviceId2"]]}})"); + executeTestCase(display_device::SingleDisplayConfigState {.m_modified = {.m_original_primary_device = "DeviceId2", .m_interim_primary_device = "DeviceId3"}}, R"({"initial":{"primary_devices":[],"topology":[]},"modified":{"interim_primary_device":"DeviceId3","original_hdr_states":{},"original_modes":{},"original_primary_device":"DeviceId2","topology":[]}})"); executeInvalidJsonTestCase(); executeFromJsonFailureTestCase(R"({})"); executeToJsonFailureTestCase(display_device::SingleDisplayConfigState { diff --git a/tests/unit/windows/test_settings_manager_undock.cpp b/tests/unit/windows/test_settings_manager_undock.cpp index 16c6f060..41b49c33 100644 --- a/tests/unit/windows/test_settings_manager_undock.cpp +++ b/tests/unit/windows/test_settings_manager_undock.cpp @@ -314,6 +314,43 @@ TEST_F(UndockRecovery, PendingSettingsSurviveApplyWhileStillUndocked) { EXPECT_EQ(clears, 1); } +TEST_F(UndockRecovery, EnsurePrimaryWhileOriginalPrimaryIsAway) { + // The dock was primary. After it left, "A" took over and the dock's primary status is pending. + state.m_initial = {{{"dock"}, {"A"}, {"B"}}, {"dock"}}; + state.m_modified = {{{"dock"}}, {}, {}, "dock"}; + active = {{"A"}, {"B"}}; + devices = { + {.m_device_id = "A", .m_info = EnumeratedDevice::Info {.m_primary = true}}, + {.m_device_id = "B", .m_info = EnumeratedDevice::Info {}}, + }; + std::string primary {"A"}; + init(); + ON_CALL(*api, isPrimary(_)).WillByDefault([&primary](const std::string &id) { + return id == primary; + }); + ON_CALL(*api, setAsPrimary(_)).WillByDefault([&primary](const std::string &id) { + primary = id; + return true; + }); + + SingleDisplayConfiguration config; + config.m_device_id = "B"; + config.m_device_prep = SingleDisplayConfiguration::DevicePreparation::EnsurePrimary; + EXPECT_EQ(manager->applySettings(config), SettingsManager::ApplyResult::Ok); + EXPECT_EQ(primary, "B"); + + // Reverting while the dock is still away brings back the display that was primary before the session. + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(primary, "A"); + EXPECT_EQ(clears, 0); + + // The dock's own primary status is still pending and is restored once it returns. + devices.push_back({.m_device_id = "dock"}); + EXPECT_EQ(manager->revertSettings(), SettingsManager::RevertResult::Ok); + EXPECT_EQ(primary, "dock"); + EXPECT_EQ(clears, 1); +} + TEST_F(UndockRecovery, StagingKeepsCloneGroupWithActiveMember) { state.m_initial.m_topology.push_back({"left", "right"}); devices.push_back({.m_device_id = "left"}); From f72c62ac22a24ff18578a485348215e2cbd5030d Mon Sep 17 00:00:00 2001 From: Jim Martin Date: Tue, 29 Sep 2026 14:15:21 -0500 Subject: [PATCH 12/12] test: take primary device ids as string views --- tests/unit/windows/test_settings_manager_undock.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit/windows/test_settings_manager_undock.cpp b/tests/unit/windows/test_settings_manager_undock.cpp index 41b49c33..dd71ddca 100644 --- a/tests/unit/windows/test_settings_manager_undock.cpp +++ b/tests/unit/windows/test_settings_manager_undock.cpp @@ -11,6 +11,7 @@ #include "utils/mock_win_display_device.h" #include +#include namespace { using namespace display_device; @@ -325,11 +326,11 @@ TEST_F(UndockRecovery, EnsurePrimaryWhileOriginalPrimaryIsAway) { }; std::string primary {"A"}; init(); - ON_CALL(*api, isPrimary(_)).WillByDefault([&primary](const std::string &id) { + ON_CALL(*api, isPrimary(_)).WillByDefault([&primary](std::string_view id) { return id == primary; }); - ON_CALL(*api, setAsPrimary(_)).WillByDefault([&primary](const std::string &id) { - primary = id; + ON_CALL(*api, setAsPrimary(_)).WillByDefault([&primary](std::string_view id) { + primary = std::string {id}; return true; });