diff --git a/src/windows/include/display_device/windows/settings_manager.h b/src/windows/include/display_device/windows/settings_manager.h index 9d955a2..5a43e32 100644 --- a/src/windows/include/display_device/windows/settings_manager.h +++ b/src/windows/include/display_device/windows/settings_manager.h @@ -105,13 +105,14 @@ namespace display_device { /** * @brief Try to revert the modified settings. * @param current_topology Topology before this method is called. - * @param system_settings_touched Inticates whether a "write" operation could have been performed on the OS. + * @param system_settings_touched Indicates whether a "write" operation could have been performed on the OS. + * @param switched_topology [Optional] Indicates whether the current topology was switched to revert settings. * @returns True on success, false otherwise. * @warning The method assumes that the caller will ensure restoring the topology * in case of a failure! */ [[nodiscard]] bool - revertModifiedSettings(const ActiveTopology ¤t_topology, bool &system_settings_touched); + revertModifiedSettings(const ActiveTopology ¤t_topology, bool &system_settings_touched, bool *switched_topology = nullptr); std::shared_ptr m_dd_api; std::shared_ptr m_audio_context_api; diff --git a/src/windows/settings_manager_revert.cpp b/src/windows/settings_manager_revert.cpp index 22f9b1f..a1526eb 100644 --- a/src/windows/settings_manager_revert.cpp +++ b/src/windows/settings_manager_revert.cpp @@ -65,7 +65,8 @@ namespace display_device { } }; // We can revert the modified setting independently before playing around with initial topology. - if (!revertModifiedSettings(current_topology, system_settings_touched)) { + bool switched_to_modified_topology { false }; + if (!revertModifiedSettings(current_topology, system_settings_touched, &switched_to_modified_topology)) { // Error already logged return false; } @@ -77,8 +78,9 @@ 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; - if (!is_topology_the_same && !m_dd_api->setTopology(cached_state->m_initial.m_topology)) { + if (need_to_switch_topology && !m_dd_api->setTopology(cached_state->m_initial.m_topology)) { DD_LOG(error) << "Failed to change topology to:\n" << toJson(cached_state->m_initial.m_topology); return false; @@ -99,7 +101,7 @@ namespace display_device { } bool - SettingsManager::revertModifiedSettings(const ActiveTopology ¤t_topology, bool &system_settings_touched) { + SettingsManager::revertModifiedSettings(const ActiveTopology ¤t_topology, bool &system_settings_touched, bool *switched_topology) { const auto &cached_state { m_persistence_state->getState() }; if (!cached_state || !cached_state->m_modified.hasModifications()) { return true; @@ -118,6 +120,9 @@ namespace display_device { << toJson(cached_state->m_modified.m_topology); return false; } + if (switched_topology) { + *switched_topology = !is_topology_the_same; + } DdGuardFn hdr_guard_fn { noopFn }; boost::scope::scope_exit hdr_guard { hdr_guard_fn }; @@ -152,7 +157,7 @@ namespace display_device { // It is possible that the display modes will not actually change even though the "current != new" condition is true. // This is because of some additional internal checks that determine whether the change is actually needed. - // Therefore we should check the current display modes after the fact! + // Therefore, we should check the current display modes after the fact! if (current_modes != m_dd_api->getCurrentDisplayModes(win_utils::flattenTopology(cached_state->m_modified.m_topology))) { system_settings_touched = true; mode_guard_fn = win_utils::modeGuardFn(*m_dd_api, current_modes); diff --git a/tests/unit/windows/test_settings_manager_revert.cpp b/tests/unit/windows/test_settings_manager_revert.cpp index a8ace9f..3b1fb99 100644 --- a/tests/unit/windows/test_settings_manager_revert.cpp +++ b/tests/unit/windows/test_settings_manager_revert.cpp @@ -170,8 +170,8 @@ namespace { } void - expectedDefaultRevertModifiedSettingsCall(InSequence &sequence /* To ensure that sequence is created outside this scope */) { - auto expected_persistent_input { ut_consts::SDCS_FULL }; + expectedDefaultRevertModifiedSettingsCall(InSequence &sequence /* To ensure that sequence is created outside this scope */, const std::optional &state = ut_consts::SDCS_FULL) { + auto expected_persistent_input { state }; expected_persistent_input->m_modified = { expected_persistent_input->m_modified.m_topology }; expectedDefaultMofifiedTopologyCalls(sequence); @@ -188,16 +188,16 @@ namespace { } void - expectedDefaultInitialTopologyCalls(InSequence &sequence /* To ensure that sequence is created outside this scope */) { - EXPECT_CALL(*m_dd_api, isTopologyValid(ut_consts::SDCS_FULL->m_initial.m_topology)) + expectedDefaultInitialTopologyCalls(InSequence &sequence /* To ensure that sequence is created outside this scope */, const std::optional &state = ut_consts::SDCS_FULL) { + EXPECT_CALL(*m_dd_api, isTopologyValid(state->m_initial.m_topology)) .Times(1) .WillOnce(Return(true)) .RetiresOnSaturation(); - EXPECT_CALL(*m_dd_api, isTopologyTheSame(CURRENT_TOPOLOGY, ut_consts::SDCS_FULL->m_initial.m_topology)) + EXPECT_CALL(*m_dd_api, isTopologyTheSame(CURRENT_TOPOLOGY, state->m_initial.m_topology)) .Times(1) - .WillOnce(Return(false)) + .WillOnce(Return(CURRENT_TOPOLOGY == state->m_initial.m_topology)) .RetiresOnSaturation(); - EXPECT_CALL(*m_dd_api, setTopology(ut_consts::SDCS_FULL->m_initial.m_topology)) + EXPECT_CALL(*m_dd_api, setTopology(state->m_initial.m_topology)) .Times(1) .WillOnce(Return(true)) .RetiresOnSaturation(); @@ -592,7 +592,7 @@ TEST_F_S_MOCKED(SuccesfullyReverted, WithAudioCapture) { expectedHdrWorkaroundCalls(sequence); EXPECT_TRUE(getImpl().revertSettings()); - EXPECT_TRUE(getImpl().revertSettings()); // Seconds call after success is NOOP + EXPECT_TRUE(getImpl().revertSettings()); // Second call after success is NOOP } TEST_F_S_MOCKED(SuccesfullyReverted, NoAudioCapture) { @@ -605,7 +605,22 @@ TEST_F_S_MOCKED(SuccesfullyReverted, NoAudioCapture) { expectedHdrWorkaroundCalls(sequence); EXPECT_TRUE(getImpl().revertSettings()); - EXPECT_TRUE(getImpl().revertSettings()); // Seconds call after success is NOOP + EXPECT_TRUE(getImpl().revertSettings()); // Second call after success is NOOP +} + +TEST_F_S_MOCKED(SuccesfullyReverted, TopologySetToBackToInitialSinceItWasChangedToModified) { + auto initial_state { ut_consts::SDCS_FULL }; + initial_state->m_initial.m_topology = CURRENT_TOPOLOGY; + + InSequence sequence; + expectedDefaultCallsUntilModifiedSettings(sequence, initial_state); + expectedDefaultRevertModifiedSettingsCall(sequence, initial_state); + expectedDefaultInitialTopologyCalls(sequence, initial_state); + expectedDefaultFinalPersistenceCalls(sequence); + expectedDefaultAudioContextCalls(sequence, false); + expectedHdrWorkaroundCalls(sequence); + + EXPECT_TRUE(getImpl().revertSettings()); } TEST_F_S_MOCKED(RevertModifiedSettings, CachedSettingsAreUpdated) {