Merge lp:~afrantzis/mir/fix-1511798-alternative into lp:mir
- fix-1511798-alternative
- Merge into development-branch
Status: | Rejected |
---|---|
Rejected by: | Alexandros Frantzis |
Proposed branch: | lp:~afrantzis/mir/fix-1511798-alternative |
Merge into: | lp:mir |
Diff against target: |
280 lines (+156/-18) 5 files modified
src/server/scene/mediating_display_changer.cpp (+1/-1) src/server/shell/abstract_shell.cpp (+20/-11) tests/acceptance-tests/test_display_configuration.cpp (+3/-3) tests/acceptance-tests/test_nested_mir.cpp (+82/-0) tests/unit-tests/scene/test_abstract_shell.cpp (+50/-3) |
To merge this branch: | bzr merge lp:~afrantzis/mir/fix-1511798-alternative |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alan Griffiths | Needs Fixing | ||
PS Jenkins bot (community) | continuous-integration | Needs Fixing | |
Review via email: mp+276852@code.launchpad.net |
Commit message
shell: fix updating of display configuration when applications and nested servers exit
Description of the change
shell: fix updating of display configuration when applications and nested servers exit
The tests for nested mir behavior were cherrypicked from Alan's original proposal.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:3093
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Alan Griffiths (alan-griffiths) wrote : | # |
> I'm confused.
>
> When I try the scenario in lp:1511798 I don't see an attempt to restore the
> host config after exiting the client. (But that's what
> display_
>
> I need to check what is different between the automated and manual test
> scenarios.
I understand the difference - AbstractShell:
I have to think about the right solution here.
Alan Griffiths (alan-griffiths) wrote : | # |
I think the right solution is to move the "session-
It seems like a PITA, but this allows the window management policy control over the point at which the surface and decorations are destroyed.
Alan Griffiths (alan-griffiths) wrote : | # |
Alexandros Frantzis (afrantzis) wrote : | # |
Rejected in favor of the other alternative.
Preview Diff
1 | === modified file 'src/server/scene/mediating_display_changer.cpp' | |||
2 | --- src/server/scene/mediating_display_changer.cpp 2015-11-05 12:54:02 +0000 | |||
3 | +++ src/server/scene/mediating_display_changer.cpp 2015-11-06 10:57:51 +0000 | |||
4 | @@ -148,7 +148,7 @@ | |||
5 | 148 | { | 148 | { |
6 | 149 | std::lock_guard<std::mutex> lg{configuration_mutex}; | 149 | std::lock_guard<std::mutex> lg{configuration_mutex}; |
7 | 150 | 150 | ||
9 | 151 | return base_configuration_; | 151 | return base_configuration_->clone(); |
10 | 152 | } | 152 | } |
11 | 153 | 153 | ||
12 | 154 | void ms::MediatingDisplayChanger::configure_for_hardware_change( | 154 | void ms::MediatingDisplayChanger::configure_for_hardware_change( |
13 | 155 | 155 | ||
14 | === modified file 'src/server/shell/abstract_shell.cpp' | |||
15 | --- src/server/shell/abstract_shell.cpp 2015-11-04 07:43:28 +0000 | |||
16 | +++ src/server/shell/abstract_shell.cpp 2015-11-06 10:57:51 +0000 | |||
17 | @@ -107,8 +107,11 @@ | |||
18 | 107 | mf::SurfaceId surface) | 107 | mf::SurfaceId surface) |
19 | 108 | { | 108 | { |
20 | 109 | report->destroying_surface(*session, surface); | 109 | report->destroying_surface(*session, surface); |
22 | 110 | window_manager->remove_surface(session, session->surface(surface)); | 110 | // Order matters. The surface needs to be destroyed before calling the WM, |
23 | 111 | // so the WM and its tools can make decisions based on up-to-date information. | ||
24 | 112 | auto weak_surface = std::weak_ptr<ms::Surface>{session->surface(surface)}; | ||
25 | 111 | session->destroy_surface(surface); | 113 | session->destroy_surface(surface); |
26 | 114 | window_manager->remove_surface(session, weak_surface); | ||
27 | 112 | } | 115 | } |
28 | 113 | 116 | ||
29 | 114 | std::shared_ptr<ms::PromptSession> msh::AbstractShell::start_prompt_session_for( | 117 | std::shared_ptr<ms::PromptSession> msh::AbstractShell::start_prompt_session_for( |
30 | @@ -163,16 +166,22 @@ | |||
31 | 163 | void msh::AbstractShell::focus_next_session() | 166 | void msh::AbstractShell::focus_next_session() |
32 | 164 | { | 167 | { |
33 | 165 | std::unique_lock<std::mutex> lock(focus_mutex); | 168 | std::unique_lock<std::mutex> lock(focus_mutex); |
44 | 166 | auto session = focus_session.lock(); | 169 | auto const focused_session = focus_session.lock(); |
45 | 167 | 170 | auto successor = session_coordinator->successor_of(focused_session); | |
46 | 168 | session = session_coordinator->successor_of(session); | 171 | |
47 | 169 | 172 | while (successor != nullptr && | |
48 | 170 | std::shared_ptr<ms::Surface> surface; | 173 | successor != focused_session && |
49 | 171 | 174 | successor->default_surface() == nullptr) | |
50 | 172 | if (session) | 175 | { |
51 | 173 | surface = session->default_surface(); | 176 | successor = session_coordinator->successor_of(successor); |
52 | 174 | 177 | } | |
53 | 175 | set_focus_to_locked(lock, session, surface); | 178 | |
54 | 179 | auto const surface = successor ? successor->default_surface() : nullptr; | ||
55 | 180 | |||
56 | 181 | if (!surface) | ||
57 | 182 | successor = nullptr; | ||
58 | 183 | |||
59 | 184 | set_focus_to_locked(lock, successor, surface); | ||
60 | 176 | } | 185 | } |
61 | 177 | 186 | ||
62 | 178 | std::shared_ptr<ms::Session> msh::AbstractShell::focused_session() const | 187 | std::shared_ptr<ms::Session> msh::AbstractShell::focused_session() const |
63 | 179 | 188 | ||
64 | === modified file 'tests/acceptance-tests/test_display_configuration.cpp' | |||
65 | --- tests/acceptance-tests/test_display_configuration.cpp 2015-10-26 03:33:22 +0000 | |||
66 | +++ tests/acceptance-tests/test_display_configuration.cpp 2015-11-06 10:57:51 +0000 | |||
67 | @@ -22,7 +22,7 @@ | |||
68 | 22 | #include "mir/graphics/event_handler_register.h" | 22 | #include "mir/graphics/event_handler_register.h" |
69 | 23 | #include "mir/shell/display_configuration_controller.h" | 23 | #include "mir/shell/display_configuration_controller.h" |
70 | 24 | 24 | ||
72 | 25 | #include "mir_test_framework/connected_client_headless_server.h" | 25 | #include "mir_test_framework/connected_client_with_a_surface.h" |
73 | 26 | #include "mir/test/doubles/null_platform.h" | 26 | #include "mir/test/doubles/null_platform.h" |
74 | 27 | #include "mir/test/doubles/null_display.h" | 27 | #include "mir/test/doubles/null_display.h" |
75 | 28 | #include "mir/test/doubles/null_display_sync_group.h" | 28 | #include "mir/test/doubles/null_display_sync_group.h" |
76 | @@ -143,13 +143,13 @@ | |||
77 | 143 | } | 143 | } |
78 | 144 | } | 144 | } |
79 | 145 | 145 | ||
81 | 146 | struct DisplayConfigurationTest : mtf::ConnectedClientHeadlessServer | 146 | struct DisplayConfigurationTest : mtf::ConnectedClientWithASurface |
82 | 147 | { | 147 | { |
83 | 148 | void SetUp() override | 148 | void SetUp() override |
84 | 149 | { | 149 | { |
85 | 150 | server.override_the_session_authorizer([this] { return mt::fake_shared(stub_authorizer); }); | 150 | server.override_the_session_authorizer([this] { return mt::fake_shared(stub_authorizer); }); |
86 | 151 | preset_display(mt::fake_shared(mock_display)); | 151 | preset_display(mt::fake_shared(mock_display)); |
88 | 152 | mtf::ConnectedClientHeadlessServer::SetUp(); | 152 | mtf::ConnectedClientWithASurface::SetUp(); |
89 | 153 | } | 153 | } |
90 | 154 | 154 | ||
91 | 155 | testing::NiceMock<MockDisplay> mock_display; | 155 | testing::NiceMock<MockDisplay> mock_display; |
92 | 156 | 156 | ||
93 | === modified file 'tests/acceptance-tests/test_nested_mir.cpp' | |||
94 | --- tests/acceptance-tests/test_nested_mir.cpp 2015-11-04 17:59:20 +0000 | |||
95 | +++ tests/acceptance-tests/test_nested_mir.cpp 2015-11-06 10:57:51 +0000 | |||
96 | @@ -714,3 +714,85 @@ | |||
97 | 714 | mir_surface_release_sync(surface); | 714 | mir_surface_release_sync(surface); |
98 | 715 | mir_connection_release(connection); | 715 | mir_connection_release(connection); |
99 | 716 | } | 716 | } |
100 | 717 | |||
101 | 718 | // lp:1511798 | ||
102 | 719 | TEST_F(NestedServer, display_configuration_reset_when_application_exits) | ||
103 | 720 | { | ||
104 | 721 | NestedMirRunner nested_mir{new_connection()}; | ||
105 | 722 | ignore_rebuild_of_egl_context(); | ||
106 | 723 | |||
107 | 724 | auto const connection = mir_connect_sync(nested_mir.new_connection().c_str(), __PRETTY_FUNCTION__); | ||
108 | 725 | |||
109 | 726 | // Need a painted surface to have focus | ||
110 | 727 | auto const painted_surface = make_and_paint_surface(connection); | ||
111 | 728 | |||
112 | 729 | auto const configuration = mir_connection_create_display_config(connection); | ||
113 | 730 | |||
114 | 731 | configuration->outputs->used = false; | ||
115 | 732 | |||
116 | 733 | mt::WaitCondition initial_condition; | ||
117 | 734 | |||
118 | 735 | EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_)) | ||
119 | 736 | .WillRepeatedly(InvokeWithoutArgs([&] { initial_condition.wake_up_everyone(); })); | ||
120 | 737 | |||
121 | 738 | mir_wait_for(mir_connection_apply_display_config(connection, configuration)); | ||
122 | 739 | |||
123 | 740 | mir_display_config_destroy(configuration); | ||
124 | 741 | mir_surface_release_sync(painted_surface); | ||
125 | 742 | |||
126 | 743 | // Wait for initial config to be applied | ||
127 | 744 | initial_condition.wait_for_at_most_seconds(1); | ||
128 | 745 | Mock::VerifyAndClearExpectations(the_mock_display_configuration_report().get()); | ||
129 | 746 | ASSERT_TRUE(initial_condition.woken()); | ||
130 | 747 | |||
131 | 748 | mt::WaitCondition condition; | ||
132 | 749 | EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_)) | ||
133 | 750 | .WillRepeatedly(InvokeWithoutArgs([&] { condition.wake_up_everyone(); })); | ||
134 | 751 | |||
135 | 752 | mir_connection_release(connection); | ||
136 | 753 | |||
137 | 754 | condition.wait_for_at_most_seconds(1); | ||
138 | 755 | EXPECT_TRUE(condition.woken()); | ||
139 | 756 | Mock::VerifyAndClearExpectations(the_mock_display_configuration_report().get()); | ||
140 | 757 | } | ||
141 | 758 | |||
142 | 759 | // lp:1511798 | ||
143 | 760 | TEST_F(NestedServer, display_configuration_reset_when_nested_server_exits) | ||
144 | 761 | { | ||
145 | 762 | mt::WaitCondition condition; | ||
146 | 763 | |||
147 | 764 | { | ||
148 | 765 | NestedMirRunner nested_mir{new_connection()}; | ||
149 | 766 | ignore_rebuild_of_egl_context(); | ||
150 | 767 | |||
151 | 768 | auto const connection = mir_connect_sync(nested_mir.new_connection().c_str(), __PRETTY_FUNCTION__); | ||
152 | 769 | |||
153 | 770 | // Need a painted surface to have focus | ||
154 | 771 | auto const painted_surface = make_and_paint_surface(connection); | ||
155 | 772 | |||
156 | 773 | std::shared_ptr<mg::DisplayConfiguration> const new_config{nested_mir.server.the_display()->configuration()}; | ||
157 | 774 | new_config->for_each_output([](mg::UserDisplayConfigurationOutput& output) | ||
158 | 775 | { output.orientation = mir_orientation_inverted;}); | ||
159 | 776 | |||
160 | 777 | mt::WaitCondition initial_condition; | ||
161 | 778 | |||
162 | 779 | EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_)) | ||
163 | 780 | .WillRepeatedly(InvokeWithoutArgs([&] { initial_condition.wake_up_everyone(); })); | ||
164 | 781 | |||
165 | 782 | nested_mir.server.the_display_configuration_controller()->set_base_configuration(new_config); | ||
166 | 783 | |||
167 | 784 | // Wait for initial config to be applied | ||
168 | 785 | initial_condition.wait_for_at_most_seconds(1); | ||
169 | 786 | Mock::VerifyAndClearExpectations(the_mock_display_configuration_report().get()); | ||
170 | 787 | ASSERT_TRUE(initial_condition.woken()); | ||
171 | 788 | |||
172 | 789 | EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_)) | ||
173 | 790 | .WillRepeatedly(InvokeWithoutArgs([&] { condition.wake_up_everyone(); })); | ||
174 | 791 | |||
175 | 792 | mir_surface_release_sync(painted_surface); | ||
176 | 793 | mir_connection_release(connection); | ||
177 | 794 | } | ||
178 | 795 | |||
179 | 796 | condition.wait_for_at_most_seconds(1); | ||
180 | 797 | EXPECT_TRUE(condition.woken()); | ||
181 | 798 | } | ||
182 | 717 | 799 | ||
183 | === modified file 'tests/unit-tests/scene/test_abstract_shell.cpp' | |||
184 | --- tests/unit-tests/scene/test_abstract_shell.cpp 2015-11-04 07:43:28 +0000 | |||
185 | +++ tests/unit-tests/scene/test_abstract_shell.cpp 2015-11-06 10:57:51 +0000 | |||
186 | @@ -31,7 +31,6 @@ | |||
187 | 31 | 31 | ||
188 | 32 | #include "mir/test/doubles/mock_window_manager.h" | 32 | #include "mir/test/doubles/mock_window_manager.h" |
189 | 33 | #include "mir/test/doubles/mock_surface_coordinator.h" | 33 | #include "mir/test/doubles/mock_surface_coordinator.h" |
190 | 34 | #include "mir/test/doubles/mock_session_listener.h" | ||
191 | 35 | #include "mir/test/doubles/mock_surface.h" | 34 | #include "mir/test/doubles/mock_surface.h" |
192 | 36 | #include "mir/test/doubles/null_event_sink.h" | 35 | #include "mir/test/doubles/null_event_sink.h" |
193 | 37 | #include "mir/test/doubles/null_snapshot_strategy.h" | 36 | #include "mir/test/doubles/null_snapshot_strategy.h" |
194 | @@ -107,7 +106,6 @@ | |||
195 | 107 | NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator; | 106 | NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator; |
196 | 108 | NiceMock<MockSessionContainer> session_container; | 107 | NiceMock<MockSessionContainer> session_container; |
197 | 109 | NiceMock<MockSessionEventSink> session_event_sink; | 108 | NiceMock<MockSessionEventSink> session_event_sink; |
198 | 110 | NiceMock<mtd::MockSessionListener> session_listener; | ||
199 | 111 | NiceMock<MockSurfaceFactory> surface_factory; | 109 | NiceMock<MockSurfaceFactory> surface_factory; |
200 | 112 | mtd::StubDisplay display{3}; | 110 | mtd::StubDisplay display{3}; |
201 | 113 | 111 | ||
202 | @@ -118,7 +116,7 @@ | |||
203 | 118 | mt::fake_shared(session_container), | 116 | mt::fake_shared(session_container), |
204 | 119 | std::make_shared<mtd::NullSnapshotStrategy>(), | 117 | std::make_shared<mtd::NullSnapshotStrategy>(), |
205 | 120 | mt::fake_shared(session_event_sink), | 118 | mt::fake_shared(session_event_sink), |
207 | 121 | mt::fake_shared(session_listener), | 119 | std::make_shared<ms::NullSessionListener>(), |
208 | 122 | mt::fake_shared(display), | 120 | mt::fake_shared(display), |
209 | 123 | std::make_shared<mtd::NullANRDetector>()}; | 121 | std::make_shared<mtd::NullANRDetector>()}; |
210 | 124 | 122 | ||
211 | @@ -367,6 +365,9 @@ | |||
212 | 367 | msh::FocusController& focus_controller = shell; | 365 | msh::FocusController& focus_controller = shell; |
213 | 368 | auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>()); | 366 | auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>()); |
214 | 369 | auto session1 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>()); | 367 | auto session1 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>()); |
215 | 368 | shell.create_surface(session, ms::a_surface(), nullptr); | ||
216 | 369 | shell.create_surface(session1, ms::a_surface(), nullptr); | ||
217 | 370 | |||
218 | 370 | focus_controller.set_focus_to(session, {}); | 371 | focus_controller.set_focus_to(session, {}); |
219 | 371 | 372 | ||
220 | 372 | EXPECT_CALL(session_container, successor_of(session)). | 373 | EXPECT_CALL(session_container, successor_of(session)). |
221 | @@ -381,6 +382,9 @@ | |||
222 | 381 | { | 382 | { |
223 | 382 | auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>()); | 383 | auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>()); |
224 | 383 | auto session1 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>()); | 384 | auto session1 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>()); |
225 | 385 | shell.create_surface(session, ms::a_surface(), nullptr); | ||
226 | 386 | shell.create_surface(session1, ms::a_surface(), nullptr); | ||
227 | 387 | |||
228 | 384 | EXPECT_CALL(session_container, successor_of(session1)). | 388 | EXPECT_CALL(session_container, successor_of(session1)). |
229 | 385 | WillOnce(Return(session)); | 389 | WillOnce(Return(session)); |
230 | 386 | 390 | ||
231 | @@ -440,6 +444,49 @@ | |||
232 | 440 | EXPECT_THAT(focus_controller.surface_at(cursor), Eq(surface)); | 444 | EXPECT_THAT(focus_controller.surface_at(cursor), Eq(surface)); |
233 | 441 | } | 445 | } |
234 | 442 | 446 | ||
235 | 447 | TEST_F(AbstractShell, as_focus_controller_focus_next_session_skips_surfaceless_sessions) | ||
236 | 448 | { | ||
237 | 449 | msh::FocusController& focus_controller = shell; | ||
238 | 450 | auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>()); | ||
239 | 451 | auto session1 = shell.open_session(__LINE__, "Surfaceless", std::shared_ptr<mf::EventSink>()); | ||
240 | 452 | auto session2 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>()); | ||
241 | 453 | auto surface_id = shell.create_surface(session, ms::a_surface(), nullptr); | ||
242 | 454 | shell.create_surface(session2, ms::a_surface(), nullptr); | ||
243 | 455 | |||
244 | 456 | focus_controller.set_focus_to(session, session->surface(surface_id)); | ||
245 | 457 | |||
246 | 458 | EXPECT_CALL(session_container, successor_of(session)). | ||
247 | 459 | WillOnce(Return(session1)); | ||
248 | 460 | EXPECT_CALL(session_container, successor_of(session1)). | ||
249 | 461 | WillOnce(Return(session2)); | ||
250 | 462 | |||
251 | 463 | EXPECT_CALL(session_event_sink, handle_focus_change(session2)); | ||
252 | 464 | |||
253 | 465 | focus_controller.focus_next_session(); | ||
254 | 466 | } | ||
255 | 467 | |||
256 | 468 | TEST_F(AbstractShell, | ||
257 | 469 | as_focus_controller_focus_next_session_does_not_focus_any_session_if_no_session_has_surfaces) | ||
258 | 470 | { | ||
259 | 471 | msh::FocusController& focus_controller = shell; | ||
260 | 472 | auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>()); | ||
261 | 473 | auto session1 = shell.open_session(__LINE__, "Surfaceless", std::shared_ptr<mf::EventSink>()); | ||
262 | 474 | auto surface_id = shell.create_surface(session, ms::a_surface(), nullptr); | ||
263 | 475 | |||
264 | 476 | focus_controller.set_focus_to(session, session->surface(surface_id)); | ||
265 | 477 | |||
266 | 478 | shell.destroy_surface(session, surface_id); | ||
267 | 479 | |||
268 | 480 | EXPECT_CALL(session_container, successor_of(session)). | ||
269 | 481 | WillOnce(Return(session1)); | ||
270 | 482 | EXPECT_CALL(session_container, successor_of(session1)). | ||
271 | 483 | WillOnce(Return(session)); | ||
272 | 484 | |||
273 | 485 | EXPECT_CALL(session_event_sink, handle_no_focus()); | ||
274 | 486 | |||
275 | 487 | focus_controller.focus_next_session(); | ||
276 | 488 | } | ||
277 | 489 | |||
278 | 443 | TEST_F(AbstractShell, modify_surface_with_only_streams_doesnt_call_into_wm) | 490 | TEST_F(AbstractShell, modify_surface_with_only_streams_doesnt_call_into_wm) |
279 | 444 | { | 491 | { |
280 | 445 | std::shared_ptr<ms::Session> session = | 492 | std::shared_ptr<ms::Session> session = |
I'm confused.
When I try the scenario in lp:1511798 I don't see an attempt to restore the host config after exiting the client. (But that's what display_ configuration_ reset_when_ application_ exits is supposed to test.)
I need to check what is different between the automated and manual test scenarios.