MirAL

Merge lp:~alan-griffiths/miral/pre_init-CommandLineOption into lp:miral

  1. pre_init-CommandLineOption
  2. Merge into trunk
Proposed by Alan Griffiths on 2017-08-16
Status: Merged
Approved by: Gerry Boland on 2017-08-16
Approved revision: 578
Merged at revision: 577
Proposed branch: lp:~alan-griffiths/miral/pre_init-CommandLineOption
Merge into: lp:miral
Diff against target: 120 lines (+27/-4)
6 files modified
debian/changelog (+1/-0)
debian/libmiral2.symbols (+1/-0)
include/miral/command_line_option.h (+10/-1)
miral-shell/shell_main.cpp (+2/-2)
miral/command_line_option.cpp (+12/-1)
miral/symbols.map (+1/-0)
To merge this branch: bzr merge lp:~alan-griffiths/miral/pre_init-CommandLineOption
Reviewer Review Type Date Requested Status
Gerry Boland 2017-08-16 Abstain on 2017-08-16
Alberto Aguirre Approve on 2017-08-16
Mir CI Bot continuous-integration Approve on 2017-08-16
Review via email: mp+329098@code.launchpad.net

Commit Message

Enable CommandLineOptions to be processed before server initialization

Description of the Change

Enable CommandLineOptions to be processed before server initialization

The need for this became apparent in discussion with Yunit developers. Vis:

    https://github.com/yunit-io/unity-system-compositor/issues/1#issuecomment-322532709

It ought to be generally useful.

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

PASSED: Continuous integration, rev:578
https://mir-jenkins.ubuntu.com/job/miral-ci/64/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-miral/106
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5086
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5075
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5075
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5075
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=amd64,release=artful/110
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=amd64,release=artful/110/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=amd64,release=xenial/110
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=amd64,release=xenial/110/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=amd64,release=zesty/110
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=amd64,release=zesty/110/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=i386,release=artful/110
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=i386,release=artful/110/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=i386,release=xenial/110
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=i386,release=xenial/110/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=i386,release=zesty/110
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-miral/arch=i386,release=zesty/110/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote :

I never realized command line args were processed so late in initialization. Is there a reason we want them to be processed post-init at all?

review: Needs Information
Alan Griffiths (alan-griffiths) wrote :

> I never realized command line args were processed so late in initialization.
> Is there a reason we want them to be processed post-init at all?

Good question.

I guess before starting MirAL I "usually" used post_init as that ensures that Mir "structural" objects are accessible. And that default fed into the initial version of CommandLineOption. But, for MirAL users, access to these objects is less relevant.

OTOH I'm reluctant to change the default behaviour even if I *think* no users will be affected.

Gerry Boland (gerboland) wrote :

I understand your concern, but IMO current behaviour does break the principle of least surprise, and now having both pre-init and post-init command line parsing strikes me as making that situation worse.

My vote is to change behaviour and document clearly in the changelog.

Joseph Wakeling (webdrake) wrote :

For the record, I can confirm that the feature addresses the concern discussed in https://github.com/yunit-io/unity-system-compositor/issues/1#issuecomment-322532709.

As for the question of default behaviour: (i) the flexibility to choose between pre- and post-init handling of command line options seems useful in principle and (ii) I'd rather not have a breaking change in a minor release.

Alberto Aguirre (albaguirre) wrote :

Unfortunately being a library, we sometimes have to carry some cruft before it can be removed (i.e. removing post-init), so LGTM.

review: Approve
Gerry Boland (gerboland) wrote :

Well I'm not proposing any api removal, just a behaviour change. But seems I'm out voted, so I abstain!

review: Abstain
Joseph Wakeling (webdrake) wrote :

FWIW I would be happy for the default behaviour to change in a new major release. Would be reluctant to lose the ability to specify post-init handling of command line options, though (just for flexibility).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2017-08-11 09:42:14 +0000
3+++ debian/changelog 2017-08-16 11:21:00 +0000
4@@ -8,6 +8,7 @@
5 . [libmiral, miral-shell] handle display reconfiguration better and allow
6 shells to customize maximized placements.
7 . Fix FTBFS with GMock 1.7 matchers (when compiling against Mir 0.27)
8+ . Enable CommandLineOptions to be processed before server initialization
9 - Bugs fixed:
10 . floating window manager allows resizing maximized windows (LP: #1704776)
11 . [miral-shell] doesn't work with breeze X cursor theme (LP: #1699084)
12
13=== modified file 'debian/libmiral2.symbols'
14--- debian/libmiral2.symbols 2017-07-28 13:38:14 +0000
15+++ debian/libmiral2.symbols 2017-08-16 11:21:00 +0000
16@@ -394,5 +394,6 @@
17 (c++)"miral::WindowManagerTools::start_drag_and_drop(miral::WindowInfo&, std::vector<unsigned char, std::allocator<unsigned char> > const&)@MIRAL_1.4.0" 1.4.0
18 (c++)"typeinfo for miral::WindowManagementPolicyAddendum2@MIRAL_1.4.0" 1.4.0
19 MIRAL_1.5.0@MIRAL_1.5.0 1.5.0
20+ (c++)"miral::pre_init(miral::CommandLineOption const&)@MIRAL_1.5.0" 1.5.0
21 (c++)"typeinfo for miral::WindowManagementPolicyAddendum3@MIRAL_1.5.0" 1.5.0
22 (c++)"miral::WindowManagerTools::active_output()@MIRAL_1.5.0" 1.5.0
23\ No newline at end of file
24
25=== modified file 'include/miral/command_line_option.h'
26--- include/miral/command_line_option.h 2017-08-10 16:59:22 +0000
27+++ include/miral/command_line_option.h 2017-08-16 11:21:00 +0000
28@@ -30,8 +30,12 @@
29 namespace miral
30 {
31 /// Add a user configuration option to Mir's option handling.
32-/// The callback will be invoked during initialisation with a value supplied
33+/// By default the callback will be invoked following Mir initialisation but
34+/// prior to the server starting. The value supplied to the callback will come
35 /// from the command line, environment variable, config file or the default.
36+///
37+/// \note Except for re-ordering implied by "pre_init()" the callbacks will be
38+/// invoked in the order supplied.
39 class CommandLineOption
40 {
41 public:
42@@ -87,6 +91,9 @@
43
44 void operator()(mir::Server& server) const;
45
46+ // Call the callback *before* Mir initialization starts
47+ friend auto pre_init(CommandLineOption const& clo) -> CommandLineOption;
48+
49 ~CommandLineOption();
50 CommandLineOption(CommandLineOption const&);
51 auto operator=(CommandLineOption const&) -> CommandLineOption&;
52@@ -95,6 +102,8 @@
53 struct Self;
54 std::shared_ptr<Self> self;
55 };
56+
57+auto pre_init(CommandLineOption const& clo) -> CommandLineOption;
58 }
59
60 #endif //MIRAL_COMMAND_LINE_OPTION_H
61
62=== modified file 'miral-shell/shell_main.cpp'
63--- miral-shell/shell_main.cpp 2017-08-10 16:59:22 +0000
64+++ miral-shell/shell_main.cpp 2017-08-16 11:21:00 +0000
65@@ -92,7 +92,7 @@
66 debug_extensions,
67 AppendEventFilter{quit_on_ctrl_alt_bksp},
68 StartupInternalClient{"Intro", spinner},
69- CommandLineOption{[&](std::string const& typeface) { ::titlebar::font_file(typeface); },
70- "shell-titlebar-font", "font file to use for titlebars", ::titlebar::font_file()}
71+ pre_init(CommandLineOption{[&](std::string const& typeface) { ::titlebar::font_file(typeface); },
72+ "shell-titlebar-font", "font file to use for titlebars", ::titlebar::font_file()})
73 });
74 }
75
76=== modified file 'miral/command_line_option.cpp'
77--- miral/command_line_option.cpp 2017-08-10 16:59:22 +0000
78+++ miral/command_line_option.cpp 2017-08-16 11:21:00 +0000
79@@ -107,6 +107,7 @@
80 {
81 }
82
83+ bool pre_init{false};
84 std::function<void(mir::Server& server)> setup;
85 std::function<void(mir::Server& server)> callback;
86 };
87@@ -188,10 +189,20 @@
88 {
89 }
90
91+auto miral::pre_init(CommandLineOption const& clo) -> CommandLineOption
92+{
93+ clo.self->pre_init = true;
94+ return clo;
95+}
96+
97 void miral::CommandLineOption::operator()(mir::Server& server) const
98 {
99 self->setup(server);
100- server.add_init_callback([&]{ self->callback(server); });
101+
102+ if (self->pre_init)
103+ server.add_pre_init_callback([&]{ self->callback(server); });
104+ else
105+ server.add_init_callback([&]{ self->callback(server); });
106 }
107
108 miral::CommandLineOption::~CommandLineOption() = default;
109
110=== modified file 'miral/symbols.map'
111--- miral/symbols.map 2017-07-28 13:38:14 +0000
112+++ miral/symbols.map 2017-08-16 11:21:00 +0000
113@@ -430,6 +430,7 @@
114 miral::WindowManagementPolicyAddendum3::WindowManagementPolicyAddendum3*;
115 miral::WindowManagementPolicyAddendum3::operator*;
116 miral::WindowManagerTools::active_output*;
117+ miral::pre_init*;
118 non-virtual?thunk?to?miral::WindowManagementPolicyAddendum3::?WindowManagementPolicyAddendum3*;
119 typeinfo?for?miral::WindowManagementPolicyAddendum3;
120 vtable?for?miral::WindowManagementPolicyAddendum3;

Subscribers

People subscribed via source and target branches