Mir

Merge lp:~mir-team/mir/set-state-remove-mir-wait into lp:mir

Proposed by Brandon Schaefer on 2017-01-13
Status: Rejected
Rejected by: Brandon Schaefer on 2017-01-23
Proposed branch: lp:~mir-team/mir/set-state-remove-mir-wait
Merge into: lp:mir
Prerequisite: lp:~mir-team/mir/remove-mir-wait-handle-persistent-id
Diff against target: 255 lines (+74/-46)
6 files modified
include/client/mir_toolkit/mir_surface.h (+2/-3)
src/client/mir_surface_api.cpp (+20/-11)
tests/acceptance-tests/test_client_library.cpp (+29/-20)
tests/acceptance-tests/test_client_surface_events.cpp (+10/-6)
tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp (+8/-4)
tests/acceptance-tests/test_surface_modifications.cpp (+5/-2)
To merge this branch: bzr merge lp:~mir-team/mir/set-state-remove-mir-wait
Reviewer Review Type Date Requested Status
Cemil Azizoglu (community) Needs Fixing on 2017-01-16
Alan Griffiths 2017-01-13 Needs Information on 2017-01-16
Mir CI Bot continuous-integration Needs Fixing on 2017-01-14
Review via email: mp+314755@code.launchpad.net

Commit message

Remove the wait handle for set_state and use the old version for tests.

Description of the change

Remove the wait handle for set_state and use the old version for tests.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3936
https://mir-jenkins.ubuntu.com/job/mir-ci/2698/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3504/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3572
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3562
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3562
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/3562
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/3531
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/3531/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3531
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3531/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/3531/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3531
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3531/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3531
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3531/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3531
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3531/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2698/rebuild

review: Needs Fixing (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

Using the old version seems like storing up problems for later.

/1/ We should have tests that validate the new versions.

/2/ While the old versions do need tests in this release, they are going to be deleted in 0.1.

I'd like to see corresponding tests covering both new and old APIs (and the latter tests can simply be deleted with the APIs).

review: Needs Information
Cemil Azizoglu (cemil-azizoglu) wrote :

I agree with Alan.

We need to port the instances where this API is used to the new way including the tests. In the process of doing that we'll eliminate the possibility of releasing something that isn't useful/useable. We won't know that until/unless we do the work ourselves.

Don't worry about the time too much. Let's do the right thing.

review: Needs Fixing

Unmerged revisions

3936. By Brandon Schaefer on 2017-01-13

* Remove mir_wait_handle

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/client/mir_toolkit/mir_surface.h'
2--- include/client/mir_toolkit/mir_surface.h 2017-01-13 22:52:30 +0000
3+++ include/client/mir_toolkit/mir_surface.h 2017-01-13 22:52:30 +0000
4@@ -678,10 +678,9 @@
5 * Change the state of a window.
6 * \param [in] window The window to operate on
7 * \param [in] state The new state of the window
8- * \return A wait handle that can be passed to mir_wait_for
9 */
10-MirWaitHandle* mir_window_set_state(MirWindow* window,
11- MirWindowState state);
12+void mir_window_set_state(MirWindow* window,
13+ MirWindowState state);
14
15 /**
16 * Get the current state of a window.
17
18=== modified file 'src/client/mir_surface_api.cpp'
19--- src/client/mir_surface_api.cpp 2017-01-13 22:52:30 +0000
20+++ src/client/mir_surface_api.cpp 2017-01-13 22:52:30 +0000
21@@ -528,6 +528,23 @@
22 }
23 }
24
25+static MirWaitHandle* mir_window_set_state_helper(
26+ MirWindow* window,
27+ MirWindowState state)
28+{
29+ mir::require(mir_window_is_valid(window));
30+
31+ try
32+ {
33+ return window->configure(mir_window_attrib_state, state);
34+ }
35+ catch (std::exception const& ex)
36+ {
37+ MIR_LOG_UNCAUGHT_EXCEPTION(ex);
38+ return nullptr;
39+ }
40+}
41+
42 class WindowSync
43 {
44 public:
45@@ -657,17 +674,9 @@
46 return type;
47 }
48
49-MirWaitHandle* mir_window_set_state(MirWindow* window, MirWindowState state)
50+void mir_window_set_state(MirWindow* window, MirWindowState state)
51 {
52- try
53- {
54- return window ? window->configure(mir_window_attrib_state, state) : nullptr;
55- }
56- catch (std::exception const& ex)
57- {
58- MIR_LOG_UNCAUGHT_EXCEPTION(ex);
59- return nullptr;
60- }
61+ mir_window_set_state_helper(window, state);
62 }
63
64 MirWindowState mir_window_get_state(MirWindow* window)
65@@ -1189,7 +1198,7 @@
66
67 MirWaitHandle* mir_surface_set_state(MirSurface* surf, MirSurfaceState state)
68 {
69- return mir_window_set_state(surf, static_cast<MirWindowState>(state));
70+ return mir_window_set_state_helper(surf, static_cast<MirWindowState>(state));
71 }
72
73 MirSurfaceState mir_surface_get_state(MirSurface* surf)
74
75=== modified file 'tests/acceptance-tests/test_client_library.cpp'
76--- tests/acceptance-tests/test_client_library.cpp 2017-01-12 16:45:44 +0000
77+++ tests/acceptance-tests/test_client_library.cpp 2017-01-13 22:52:30 +0000
78@@ -246,14 +246,17 @@
79 {
80 for (int i = 0; i < 10; i++)
81 {
82- mir_wait_for_one(mir_window_set_state(surf,
83- mir_window_state_maximized));
84- mir_wait_for_one(mir_window_set_state(surf,
85- mir_window_state_restored));
86- mir_wait_for_one(mir_window_set_state(surf,
87- mir_window_state_fullscreen));
88- mir_wait_for_one(mir_window_set_state(surf,
89- mir_window_state_minimized));
90+#pragma GCC diagnostic push
91+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
92+ mir_wait_for_one(mir_surface_set_state(surf,
93+ mir_surface_state_maximized));
94+ mir_wait_for_one(mir_surface_set_state(surf,
95+ mir_surface_state_restored));
96+ mir_wait_for_one(mir_surface_set_state(surf,
97+ mir_surface_state_fullscreen));
98+ mir_wait_for_one(mir_surface_set_state(surf,
99+ mir_surface_state_minimized));
100+#pragma GCC diagnostic pop
101 }
102 }
103 };
104@@ -405,24 +408,30 @@
105
106 EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_restored));
107
108- mir_wait_for(mir_window_set_state(window, mir_window_state_fullscreen));
109- EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_fullscreen));
110-
111- mir_wait_for(mir_window_set_state(window, static_cast<MirWindowState>(999)));
112- EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_fullscreen));
113-
114- mir_wait_for(mir_window_set_state(window, mir_window_state_horizmaximized));
115- EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_horizmaximized));
116-
117- mir_wait_for(mir_window_set_state(window, static_cast<MirWindowState>(888)));
118- EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_horizmaximized));
119+#pragma GCC diagnostic push
120+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
121+ mir_wait_for(mir_surface_set_state(window, mir_surface_state_fullscreen));
122+ EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_fullscreen));
123+
124+ mir_wait_for(mir_surface_set_state(window, static_cast<MirSurfaceState>(999)));
125+ EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_fullscreen));
126+
127+ mir_wait_for(mir_surface_set_state(window, mir_surface_state_horizmaximized));
128+ EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_horizmaximized));
129+
130+ mir_wait_for(mir_surface_set_state(window, static_cast<MirSurfaceState>(888)));
131+ EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_horizmaximized));
132+#pragma GCC diagnostic pop
133
134 // Stress-test synchronization logic with some flooding
135 for (int i = 0; i < 100; i++)
136 {
137 mir_window_set_state(window, mir_window_state_maximized);
138 mir_window_set_state(window, mir_window_state_restored);
139- mir_wait_for(mir_window_set_state(window, mir_window_state_fullscreen));
140+#pragma GCC diagnostic push
141+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
142+ mir_wait_for(mir_surface_set_state(window, mir_surface_state_fullscreen));
143+#pragma GCC diagnostic pop
144 ASSERT_THAT(mir_window_get_state(window), Eq(mir_window_state_fullscreen));
145 }
146
147
148=== modified file 'tests/acceptance-tests/test_client_surface_events.cpp'
149--- tests/acceptance-tests/test_client_surface_events.cpp 2017-01-11 20:17:22 +0000
150+++ tests/acceptance-tests/test_client_surface_events.cpp 2017-01-13 22:52:30 +0000
151@@ -196,9 +196,11 @@
152
153 TEST_F(ClientSurfaceEvents, surface_receives_state_events)
154 {
155+#pragma GCC diagnostic push
156+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
157 {
158- mir_wait_for(mir_window_set_state(window, mir_window_state_fullscreen));
159- mir_wait_for(mir_window_set_state(other_surface, mir_window_state_vertmaximized));
160+ mir_wait_for(mir_surface_set_state(window, mir_surface_state_fullscreen));
161+ mir_wait_for(mir_surface_set_state(other_surface, mir_surface_state_vertmaximized));
162
163 std::lock_guard<decltype(last_event_mutex)> last_event_lock{last_event_mutex};
164
165@@ -206,7 +208,7 @@
166 }
167
168 {
169- mir_wait_for(mir_window_set_state(window, static_cast<MirWindowState>(999)));
170+ mir_wait_for(mir_surface_set_state(window, static_cast<MirSurfaceState>(999)));
171
172 std::lock_guard<decltype(last_event_mutex)> last_event_lock{last_event_mutex};
173 EXPECT_THAT(last_event, mt::WindowEvent(mir_window_attrib_state, mir_window_state_fullscreen));
174@@ -215,7 +217,7 @@
175 reset_last_event();
176
177 {
178- mir_wait_for(mir_window_set_state(window, mir_window_state_vertmaximized));
179+ mir_wait_for(mir_surface_set_state(window, mir_surface_state_vertmaximized));
180
181 std::lock_guard<decltype(last_event_mutex)> last_event_lock{last_event_mutex};
182
183@@ -225,13 +227,14 @@
184 reset_last_event();
185
186 {
187- mir_wait_for(mir_window_set_state(window, static_cast<MirWindowState>(777)));
188- mir_wait_for(mir_window_set_state(other_surface, mir_window_state_maximized));
189+ mir_wait_for(mir_surface_set_state(window, static_cast<MirSurfaceState>(777)));
190+ mir_wait_for(mir_surface_set_state(other_surface, mir_surface_state_maximized));
191
192 std::lock_guard<decltype(last_event_mutex)> last_event_lock{last_event_mutex};
193
194 EXPECT_EQ(nullptr, last_event);
195 }
196+#pragma GCC diagnostic pop
197 }
198
199 struct OrientationEvents : ClientSurfaceEvents, ::testing::WithParamInterface<MirOrientation> {};
200@@ -274,6 +277,7 @@
201 }
202 }
203
204+
205 TEST_F(ClientSurfaceEvents, surface_receives_close_event)
206 {
207 set_event_filter(mir_event_type_close_window);
208
209=== modified file 'tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp'
210--- tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp 2017-01-12 16:45:44 +0000
211+++ tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp 2017-01-13 22:52:30 +0000
212@@ -87,8 +87,10 @@
213 {
214 EXPECT_CALL(*mock_window_manager,
215 set_surface_attribute(_, _, mir_window_attrib_state, Eq(mir_window_state_maximized)));
216-
217- mir_wait_for(mir_window_set_state(window, mir_window_state_maximized));
218+#pragma GCC diagnostic push
219+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
220+ mir_wait_for(mir_surface_set_state(window, mir_surface_state_maximized));
221+#pragma GCC diagnostic pop
222
223 EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_maximized));
224 }
225@@ -108,8 +110,10 @@
226 EXPECT_CALL(*mock_window_manager,
227 set_surface_attribute(_, _, mir_window_attrib_state, Eq(mir_window_state_maximized)))
228 .WillOnce(Invoke(set_to_vertmax));
229-
230- mir_wait_for(mir_window_set_state(window, mir_window_state_maximized));
231+#pragma GCC diagnostic push
232+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
233+ mir_wait_for(mir_surface_set_state(window, mir_surface_state_maximized));
234+#pragma GCC diagnostic pop
235
236 EXPECT_THAT(mir_window_get_state(window), Eq(mir_window_state_vertmaximized));
237 }
238
239=== modified file 'tests/acceptance-tests/test_surface_modifications.cpp'
240--- tests/acceptance-tests/test_surface_modifications.cpp 2017-01-10 23:55:25 +0000
241+++ tests/acceptance-tests/test_surface_modifications.cpp 2017-01-13 22:52:30 +0000
242@@ -572,8 +572,11 @@
243 EXPECT_CALL(surface_observer, hidden_set_to(is_visible(initial_state)));
244 EXPECT_CALL(surface_observer, hidden_set_to(is_visible(new_state)));
245
246- mir_wait_for(mir_window_set_state(window, initial_state));
247- mir_wait_for(mir_window_set_state(window, new_state));
248+#pragma GCC diagnostic push
249+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
250+ mir_wait_for(mir_surface_set_state(window, static_cast<MirSurfaceState>(initial_state)));
251+ mir_wait_for(mir_surface_set_state(window, static_cast<MirSurfaceState>(new_state)));
252+#pragma GCC diagnostic pop
253 }
254
255 INSTANTIATE_TEST_CASE_P(SurfaceModifications, SurfaceStateCase,

Subscribers

People subscribed via source and target branches