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

[Alan Griffiths]

Enable CommandLineOptions to be processed before server initialization

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.

[Mir CI Bot (mir-ci-bot)]

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)]

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)]

> 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)]

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)]

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)]

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)]

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)]

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).

=== modified file 'debian/changelog'
--- debian/changelog	2017-08-11 09:42:14 +0000
+++ debian/changelog	2017-08-16 11:21:00 +0000
@@ -8,6 +8,7 @@
       . [libmiral, miral-shell] handle display reconfiguration better and allow
         shells to customize maximized placements.
       . Fix FTBFS with GMock 1.7 matchers (when compiling against Mir 0.27)
+      . Enable CommandLineOptions to be processed before server initialization
     - Bugs fixed:
       . floating window manager allows resizing maximized windows (LP: #1704776)
       . [miral-shell] doesn't work with breeze X cursor theme (LP: #1699084)

=== modified file 'debian/libmiral2.symbols'
--- debian/libmiral2.symbols	2017-07-28 13:38:14 +0000
+++ debian/libmiral2.symbols	2017-08-16 11:21:00 +0000
@@ -394,5 +394,6 @@
  (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
  (c++)"typeinfo for miral::WindowManagementPolicyAddendum2@MIRAL_1.4.0" 1.4.0
  MIRAL_1.5.0@MIRAL_1.5.0 1.5.0
+ (c++)"miral::pre_init(miral::CommandLineOption const&)@MIRAL_1.5.0" 1.5.0
  (c++)"typeinfo for miral::WindowManagementPolicyAddendum3@MIRAL_1.5.0" 1.5.0
  (c++)"miral::WindowManagerTools::active_output()@MIRAL_1.5.0" 1.5.0
\ No newline at end of file

=== modified file 'include/miral/command_line_option.h'
--- include/miral/command_line_option.h	2017-08-10 16:59:22 +0000
+++ include/miral/command_line_option.h	2017-08-16 11:21:00 +0000
@@ -30,8 +30,12 @@
 namespace miral
 {
 /// Add a user configuration option to Mir's option handling.
-/// The callback will be invoked during initialisation with a value supplied
+/// By default the callback will be invoked following Mir initialisation but
+/// prior to the server starting. The value supplied to the callback will come
 /// from the command line, environment variable, config file or the default.
+///
+/// \note Except for re-ordering implied by "pre_init()" the callbacks will be
+/// invoked in the order supplied.
 class CommandLineOption
 {
 public:
@@ -87,6 +91,9 @@
 
     void operator()(mir::Server& server) const;
 
+    // Call the callback *before* Mir initialization starts
+    friend auto pre_init(CommandLineOption const& clo) -> CommandLineOption;
+
     ~CommandLineOption();
     CommandLineOption(CommandLineOption const&);
     auto operator=(CommandLineOption const&) -> CommandLineOption&;
@@ -95,6 +102,8 @@
     struct Self;
     std::shared_ptr<Self> self;
 };
+
+auto pre_init(CommandLineOption const& clo) -> CommandLineOption;
 }
 
 #endif //MIRAL_COMMAND_LINE_OPTION_H

=== modified file 'miral-shell/shell_main.cpp'
--- miral-shell/shell_main.cpp	2017-08-10 16:59:22 +0000
+++ miral-shell/shell_main.cpp	2017-08-16 11:21:00 +0000
@@ -92,7 +92,7 @@
             debug_extensions,
             AppendEventFilter{quit_on_ctrl_alt_bksp},
             StartupInternalClient{"Intro", spinner},
-            CommandLineOption{[&](std::string const& typeface) { ::titlebar::font_file(typeface); },
-                              "shell-titlebar-font", "font file to use for titlebars", ::titlebar::font_file()}
+            pre_init(CommandLineOption{[&](std::string const& typeface) { ::titlebar::font_file(typeface); },
+                              "shell-titlebar-font", "font file to use for titlebars", ::titlebar::font_file()})
         });
 }

=== modified file 'miral/command_line_option.cpp'
--- miral/command_line_option.cpp	2017-08-10 16:59:22 +0000
+++ miral/command_line_option.cpp	2017-08-16 11:21:00 +0000
@@ -107,6 +107,7 @@
     {
     }
 
+    bool pre_init{false};
     std::function<void(mir::Server& server)> setup;
     std::function<void(mir::Server& server)> callback;
 };
@@ -188,10 +189,20 @@
 {
 }
 
+auto miral::pre_init(CommandLineOption const& clo) -> CommandLineOption
+{
+    clo.self->pre_init = true;
+    return clo;
+}
+
 void miral::CommandLineOption::operator()(mir::Server& server) const
 {
     self->setup(server);
-    server.add_init_callback([&]{ self->callback(server); });
+
+    if (self->pre_init)
+        server.add_pre_init_callback([&]{ self->callback(server); });
+    else
+        server.add_init_callback([&]{ self->callback(server); });
 }
 
 miral::CommandLineOption::~CommandLineOption() = default;

=== modified file 'miral/symbols.map'
--- miral/symbols.map	2017-07-28 13:38:14 +0000
+++ miral/symbols.map	2017-08-16 11:21:00 +0000
@@ -430,6 +430,7 @@
     miral::WindowManagementPolicyAddendum3::WindowManagementPolicyAddendum3*;
     miral::WindowManagementPolicyAddendum3::operator*;
     miral::WindowManagerTools::active_output*;
+    miral::pre_init*;
     non-virtual?thunk?to?miral::WindowManagementPolicyAddendum3::?WindowManagementPolicyAddendum3*;
     typeinfo?for?miral::WindowManagementPolicyAddendum3;
     vtable?for?miral::WindowManagementPolicyAddendum3;