Mir

Merge lp:~vanvugt/mir/deduplicate-NullDisplayReport into lp:mir

  1. deduplicate-NullDisplayReport
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Work in progress
Proposed branch: lp:~vanvugt/mir/deduplicate-NullDisplayReport
Merge into: lp:mir
Diff against target: 272 lines (+40/-63)
9 files modified
include/platform/mir/graphics/null_display_report.h (+9/-12)
src/platform/graphics/CMakeLists.txt (+1/-0)
src/platform/graphics/null_display_report.cpp (+19/-17)
src/platform/symbols.map (+3/-0)
src/platforms/android/server/device_quirks.cpp (+2/-14)
src/platforms/android/utils/render_overlays.cpp (+2/-15)
src/server/report/null/CMakeLists.txt (+0/-1)
src/server/report/null/null_report_factory.cpp (+2/-2)
tests/unit-tests/platforms/nested/test_nested_display.cpp (+2/-2)
To merge this branch: bzr merge lp:~vanvugt/mir/deduplicate-NullDisplayReport
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve
Alan Griffiths Disapprove
Review via email: mp+304905@code.launchpad.net

Commit message

Deduplicate three null DisplayReports into one.

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

FAILED: Continuous integration, rev:3685
https://mir-jenkins.ubuntu.com/job/mir-ci/1634/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2044/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2106
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2097
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2097
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2097
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2070/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2070/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2070/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2070/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/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2070/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/2070
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2070/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Needing NullDisplayReport in mirplatform seems more like a problem than a solution.

review: Disapprove
Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Logically you make a good point. This isn't addressing the android code at the root of the issue.

However it's also worth remembering that DisplayReport is a report for Display. And Display is implemented in src/platforms/* so it makes sense that any commonality should exist in libmirplatform rather than libmirserver.

Reply
lp:~vanvugt/mir/deduplicate-NullDisplayReport updated
3686. By Daniel van Vugt

Ping Jenkins

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3686
https://mir-jenkins.ubuntu.com/job/mir-ci/1636/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2049/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2111/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2102/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2102/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2102/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2075/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2075/console

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

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Come to think of it, any report class that legitimately has a null use case and that might only need a partial implementation in many cases really should not be pure virtual at all. A possibly cleaner solution to all this is to merge DisplayReport with null::DisplayReport.

Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

And it looks like I might be proposing four more places where NullDisplayReport gets used in future:
   http://bazaar.launchpad.net/~vanvugt/mir/physical-frame/revision/3605

Reply
lp:~vanvugt/mir/deduplicate-NullDisplayReport updated
3687. By Daniel van Vugt

Merge latest trunk

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3687
https://mir-jenkins.ubuntu.com/job/mir-ci/1657/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2072
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2134
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2125
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2125
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2125
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2100/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/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2100/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2100/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/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2100/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/2100
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2100/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

On second thoughts, mirserver is the only place in production where the report should be selected and constructed.

So ideally we should only see a null DisplayReport in mirserver and tests. I guess the current placement is correct if we can clean up and remove the two occurrences from src/platforms/android/. But that's an "if".

Reply

Unmerged revisions

3687. By Daniel van Vugt

Merge latest trunk

3686. By Daniel van Vugt

Ping Jenkins

3685. By Daniel van Vugt

It's LGPL, not GPL

3684. By Daniel van Vugt

First attempt

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== renamed file 'src/server/report/null/display_report.h' => 'include/platform/mir/graphics/null_display_report.h'
2--- src/server/report/null/display_report.h 2015-04-28 07:54:10 +0000
3+++ include/platform/mir/graphics/null_display_report.h 2016-09-07 03:00:58 +0000
4@@ -2,23 +2,23 @@
5 * Copyright © 2012 Canonical Ltd.
6 *
7 * This program is free software: you can redistribute it and/or modify it
8- * under the terms of the GNU General Public License version 3,
9+ * under the terms of the GNU Lesser General Public License version 3,
10 * as published by the Free Software Foundation.
11 *
12 * This program is distributed in the hope that it will be useful,
13 * but WITHOUT ANY WARRANTY; without even the implied warranty of
14 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15- * GNU General Public License for more details.
16+ * GNU Lesser General Public License for more details.
17 *
18- * You should have received a copy of the GNU General Public License
19+ * You should have received a copy of the GNU Lesser General Public License
20 * along with this program. If not, see <http://www.gnu.org/licenses/>.
21 *
22 * Authored by: Alan Griffiths <alan@octopull.co.uk>
23 */
24
25
26-#ifndef MIR_REPORT_NULL_DISPLAY_REPORT_H_
27-#define MIR_REPORT_NULL_DISPLAY_REPORT_H_
28+#ifndef MIR_GRAPHICS_NULL_DISPLAY_REPORT_H_
29+#define MIR_GRAPHICS_NULL_DISPLAY_REPORT_H_
30
31
32 #include "mir/graphics/display_report.h"
33@@ -26,11 +26,9 @@
34
35 namespace mir
36 {
37-namespace report
38-{
39-namespace null
40-{
41-class DisplayReport : public graphics::DisplayReport
42+namespace graphics
43+{
44+class NullDisplayReport : public graphics::DisplayReport
45 {
46 public:
47
48@@ -47,6 +45,5 @@
49 };
50 }
51 }
52-}
53
54-#endif /* MIR_REPORT_NULL_DISPLAY_REPORT_H_ */
55+#endif /* MIR_GRAPHICS_NULL_DISPLAY_REPORT_H_ */
56
57=== modified file 'src/platform/graphics/CMakeLists.txt'
58--- src/platform/graphics/CMakeLists.txt 2016-06-02 05:33:50 +0000
59+++ src/platform/graphics/CMakeLists.txt 2016-09-07 03:00:58 +0000
60@@ -11,6 +11,7 @@
61 pixel_format_utils.cpp
62 overlapping_output_grouping.cpp
63 platform_probe.cpp
64+ null_display_report.cpp
65 )
66
67 add_library(mirplatformgraphicscommon OBJECT
68
69=== renamed file 'src/server/report/null/display_report.cpp' => 'src/platform/graphics/null_display_report.cpp'
70--- src/server/report/null/display_report.cpp 2015-04-28 07:54:10 +0000
71+++ src/platform/graphics/null_display_report.cpp 2016-09-07 03:00:58 +0000
72@@ -2,31 +2,33 @@
73 * Copyright © 2013 Canonical Ltd.
74 *
75 * This program is free software: you can redistribute it and/or modify it
76- * under the terms of the GNU General Public License version 3,
77+ * under the terms of the GNU Lesser General Public License version 3,
78 * as published by the Free Software Foundation.
79 *
80 * This program is distributed in the hope that it will be useful,
81 * but WITHOUT ANY WARRANTY; without even the implied warranty of
82 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
83- * GNU General Public License for more details.
84+ * GNU Lesser General Public License for more details.
85 *
86- * You should have received a copy of the GNU General Public License
87+ * You should have received a copy of the GNU Lesser General Public License
88 * along with this program. If not, see <http://www.gnu.org/licenses/>.
89 *
90 * Authored by: Alan Griffiths <alan@octopull.co.uk>
91 */
92
93-#include "display_report.h"
94-
95-namespace mrn = mir::report::null;
96-
97-void mrn::DisplayReport::report_successful_setup_of_native_resources() {}
98-void mrn::DisplayReport::report_successful_egl_make_current_on_construction() {}
99-void mrn::DisplayReport::report_successful_egl_buffer_swap_on_construction() {}
100-void mrn::DisplayReport::report_successful_drm_mode_set_crtc_on_construction() {}
101-void mrn::DisplayReport::report_successful_display_construction() {}
102-void mrn::DisplayReport::report_drm_master_failure(int) {}
103-void mrn::DisplayReport::report_vt_switch_away_failure() {}
104-void mrn::DisplayReport::report_vt_switch_back_failure() {}
105-void mrn::DisplayReport::report_egl_configuration(EGLDisplay, EGLConfig) {}
106-void mrn::DisplayReport::report_vsync(unsigned int) {}
107+#include "mir/graphics/null_display_report.h"
108+
109+namespace mir { namespace graphics {
110+
111+void NullDisplayReport::report_successful_setup_of_native_resources() {}
112+void NullDisplayReport::report_successful_egl_make_current_on_construction() {}
113+void NullDisplayReport::report_successful_egl_buffer_swap_on_construction() {}
114+void NullDisplayReport::report_successful_drm_mode_set_crtc_on_construction() {}
115+void NullDisplayReport::report_successful_display_construction() {}
116+void NullDisplayReport::report_drm_master_failure(int) {}
117+void NullDisplayReport::report_vt_switch_away_failure() {}
118+void NullDisplayReport::report_vt_switch_back_failure() {}
119+void NullDisplayReport::report_egl_configuration(EGLDisplay, EGLConfig) {}
120+void NullDisplayReport::report_vsync(unsigned int) {}
121+
122+}} // namespace mir::graphics
123
124=== modified file 'src/platform/symbols.map'
125--- src/platform/symbols.map 2016-08-22 14:36:40 +0000
126+++ src/platform/symbols.map 2016-09-07 03:00:58 +0000
127@@ -270,6 +270,9 @@
128 android::isEligibleBuiltInKeyboard*;
129 __android_log_assert;
130 __android_log_print;
131+
132+ vtable?for?mir::graphics::NullDisplayReport*;
133+ mir::graphics::NullDisplayReport*;
134 };
135 local: *;
136 };
137
138=== modified file 'src/platforms/android/server/device_quirks.cpp'
139--- src/platforms/android/server/device_quirks.cpp 2016-08-07 20:38:35 +0000
140+++ src/platforms/android/server/device_quirks.cpp 2016-09-07 03:00:58 +0000
141@@ -18,7 +18,7 @@
142
143 #include <GLES2/gl2.h>
144 #include <EGL/egl.h>
145-#include "mir/graphics/display_report.h"
146+#include "mir/graphics/null_display_report.h"
147 #include "mir/graphics/gl_config.h"
148 #include "gl_context.h"
149 #include "device_quirks.h"
150@@ -100,19 +100,7 @@
151 return false;
152 }
153
154-struct NullReport : mg::DisplayReport
155-{
156- void report_successful_setup_of_native_resources() override {};
157- void report_successful_egl_make_current_on_construction() override {};
158- void report_successful_egl_buffer_swap_on_construction() override {};
159- void report_successful_display_construction() override {};
160- void report_egl_configuration(EGLDisplay, EGLConfig) override {};
161- void report_vsync(unsigned int) override {};
162- void report_successful_drm_mode_set_crtc_on_construction() override {};
163- void report_drm_master_failure(int) override {};
164- void report_vt_switch_away_failure() override {};
165- void report_vt_switch_back_failure() override {};
166-} report;
167+mg::NullDisplayReport report;
168
169 struct Config : mg::GLConfig
170 {
171
172=== modified file 'src/platforms/android/utils/render_overlays.cpp'
173--- src/platforms/android/utils/render_overlays.cpp 2016-08-16 19:34:28 +0000
174+++ src/platforms/android/utils/render_overlays.cpp 2016-09-07 03:00:58 +0000
175@@ -26,7 +26,7 @@
176 #include "mir/graphics/graphic_buffer_allocator.h"
177 #include "mir/graphics/buffer_properties.h"
178 #include "mir/graphics/gl_config.h"
179-#include "mir/graphics/display_report.h"
180+#include "mir/graphics/null_display_report.h"
181 #include "mir/renderer/gl/render_target.h"
182 #include "mir/renderer/sw/pixel_source.h"
183 #include "mir_image.h"
184@@ -226,19 +226,6 @@
185 int stencil_buffer_bits() const override { return 0; }
186 };
187
188-struct DisplayReport : mg::DisplayReport
189-{
190- void report_successful_setup_of_native_resources() override {}
191- void report_successful_egl_make_current_on_construction() override {}
192- void report_successful_egl_buffer_swap_on_construction() override {}
193- void report_successful_display_construction() override {}
194- void report_egl_configuration(EGLDisplay, EGLConfig) override {}
195- void report_vsync(unsigned int) override {}
196- void report_successful_drm_mode_set_crtc_on_construction() override {}
197- void report_drm_master_failure(int) override {}
198- void report_vt_switch_away_failure() override {}
199- void report_vt_switch_back_failure() override {}
200-};
201 }
202
203 int main(int argc, char const** argv)
204@@ -265,7 +252,7 @@
205
206 auto platform_fn = platform_library->load_function<mg::CreateHostPlatform>(
207 "create_host_platform", MIR_SERVER_GRAPHICS_PLATFORM_VERSION);
208- auto platform = platform_fn(options, nullptr, std::make_shared<DisplayReport>());
209+ auto platform = platform_fn(options, nullptr, std::make_shared<mg::NullDisplayReport>());
210
211 //Strange issues going on here with dlopen() + hybris (which uses gnu_indirect_functions)
212 //https://github.com/libhybris/libhybris/issues/315
213
214=== modified file 'src/server/report/null/CMakeLists.txt'
215--- src/server/report/null/CMakeLists.txt 2016-07-28 23:00:22 +0000
216+++ src/server/report/null/CMakeLists.txt 2016-09-07 03:00:58 +0000
217@@ -3,7 +3,6 @@
218
219 compositor_report.cpp
220 connector_report.cpp
221- display_report.cpp
222 input_report.cpp
223 message_processor_report.cpp
224 null_report_factory.cpp
225
226=== modified file 'src/server/report/null/null_report_factory.cpp'
227--- src/server/report/null/null_report_factory.cpp 2016-07-28 23:00:22 +0000
228+++ src/server/report/null/null_report_factory.cpp 2016-09-07 03:00:58 +0000
229@@ -22,11 +22,11 @@
230 #include "connector_report.h"
231 #include "message_processor_report.h"
232 #include "session_mediator_report.h"
233-#include "display_report.h"
234 #include "input_report.h"
235 #include "seat_report.h"
236 #include "shell_report.h"
237 #include "scene_report.h"
238+#include "mir/graphics/null_display_report.h"
239 #include "mir/logging/null_shared_library_prober_report.h"
240
241 std::shared_ptr<mir::compositor::CompositorReport> mir::report::NullReportFactory::create_compositor_report()
242@@ -36,7 +36,7 @@
243
244 std::shared_ptr<mir::graphics::DisplayReport> mir::report::NullReportFactory::create_display_report()
245 {
246- return std::make_shared<null::DisplayReport>();
247+ return std::make_shared<mir::graphics::NullDisplayReport>();
248 }
249
250 std::shared_ptr<mir::scene::SceneReport> mir::report::NullReportFactory::create_scene_report()
251
252=== modified file 'tests/unit-tests/platforms/nested/test_nested_display.cpp'
253--- tests/unit-tests/platforms/nested/test_nested_display.cpp 2016-05-04 20:53:03 +0000
254+++ tests/unit-tests/platforms/nested/test_nested_display.cpp 2016-09-07 03:00:58 +0000
255@@ -19,7 +19,7 @@
256 #include "src/server/graphics/nested/display.h"
257 #include "src/server/graphics/nested/host_connection.h"
258 #include "src/server/graphics/nested/host_surface.h"
259-#include "src/server/report/null/display_report.h"
260+#include "mir/graphics/null_display_report.h"
261 #include "mir/graphics/default_display_configuration_policy.h"
262 #include "src/server/input/null_input_dispatcher.h"
263 #include "mir_display_configuration_builder.h"
264@@ -77,7 +77,7 @@
265
266 testing::NiceMock<mtd::MockEGL> mock_egl;
267 mi::NullInputDispatcher null_input_dispatcher;
268- mir::report::null::DisplayReport null_display_report;
269+ mg::NullDisplayReport null_display_report;
270 mg::CloneDisplayConfigurationPolicy default_conf_policy;
271 mtd::StubGLConfig stub_gl_config;
272 std::shared_ptr<mtd::NullPlatform> null_platform{

Subscribers

People subscribed via source and target branches