Merge lp:~alan-griffiths/mir/mir-server-with-add_configuration_option-chaining into lp:mir

[Alan Griffiths]

Support for chaining mir::Server::add_configuration_option()

Support for chaining mir::Server::add_configuration_option()

The utility vs cost of this support was contentious in the original MP, so I've separated it out for discussion.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1985
http://jenkins.qa.ubuntu.com/job/mir-ci/1829/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2143
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2150
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2079
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/169
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/169/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1016
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1016/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3080
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14714

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/1829/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

I think this mechanism offers very little (if anything) over just calling add_configuration_option() multiple times to deserve a place in our public API. Plus, I am not fond of this particular form of syntactic sugar ("add_configuration_option(x,y,z)(x,y,z)(x,y,z)...") anyway.

My vote is a non-blocking disapproval.

review: Disapprove

[Alan Griffiths (alan-griffiths)]

It comes down to whether we allow (without imposing):

    // Add choice of monitor configuration
    server.add_configuration_option(
        me::display_config_opt, me::display_config_descr, me::clone_opt_val)(
        me::display_alpha_opt, me::display_alpha_descr, me::display_alpha_off);

Or enforce:

    // Add choice of monitor configuration
    server.add_configuration_option(
        me::display_config_opt, me::display_config_descr, me::clone_opt_val);
    server.add_configuration_option(
        me::display_alpha_opt, me::display_alpha_descr, me::display_alpha_off);

We could imagine users adding even more options, but the above is real code from basic_server.cpp

[Andreas Pokorny (andreas-pokorny)]

even if somebody has a use case with a lot of configuration options he or she could do that similar to:

struct add_options
{
    mir::Server * server;
    add_options(Server* server) : server(server){}
    add_options const& operator()(std::string const& option, std::string const& description, int default_value) const
    {
         return server->add_configuration_option(option, description, default_value);
         return *this;
    }
     ....
};

not needing any further support from the mir api. Still I guess nobody will do it, since it requires probably more lines of code than one can gain with it.. So I vote for leaving it out.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> even if somebody has a use case with a lot of configuration options he or she
> could do that similar to:
>
> struct add_options
> {
> mir::Server * server;
> add_options(Server* server) : server(server){}
> add_options const& operator()(std::string const& option, std::string
> const& description, int default_value) const
> {
> return server->add_configuration_option(option, description,
> default_value);
> return *this;
> }
> ....
> };
>
> not needing any further support from the mir api. Still I guess nobody will do
> it, since it requires probably more lines of code than one can gain with it..

Your argument that any clients that wants it could do it themselves is wrong: If there is a need to do it we should do it once and every one of these client will benefit.

> So I vote for leaving it out.

OK (but your reasoning is wrong).

[Andreas Pokorny (andreas-pokorny)]

> Your argument that any clients that wants it could do it themselves is wrong:
> If there is a need to do it we should do it once and every one of these client
> will benefit.

agreed, lame argument. The thing that I left out was, that given your example the need or benefit is minimal. I believe most people will, unless we hilight its usage in big letters not even mind looking at how it has to be used.

I think you can invent or suspect a need for pretty much every imaginable feature.
So from that perspective, as the code exists and is mergeable we could simply merge it.

Unmerged revisions

1985. By Alan Griffiths on 2014-10-15

Support for chaining add_configuration_option()

=== modified file 'include/server/mir/server.h'
--- include/server/mir/server.h	2014-10-15 09:49:26 +0000
+++ include/server/mir/server.h	2014-10-15 09:49:26 +0000
@@ -74,29 +74,33 @@
 
 /** @name Configuration options
  *  @{ */
-    /// Add user configuration option(s) to Mir's option handling.
-    /// These will be resolved during initialisation from the command line,
-    /// environment variables, a config file or the supplied default.
-    void add_configuration_option(
-        std::string const& option,
-        std::string const& description,
-        int default_value);
-
-    /// Add user configuration option(s) to Mir's option handling.
-    /// These will be resolved during initialisation from the command line,
-    /// environment variables, a config file or the supplied default.
-    void add_configuration_option(
-        std::string const& option,
-        std::string const& description,
-        std::string const& default_value);
-
-    /// Add user configuration option(s) to Mir's option handling.
-    /// These will be resolved during initialisation from the command line,
-    /// environment variables, a config file or the supplied default.
-    void add_configuration_option(
-        std::string const& option,
-        std::string const& description,
-        OptionType type);
+
+    /// Helper class to facilitate adding multiple options
+    class ChainAddConfigurationOption;
+
+    /// Add user configuration option(s) to Mir's option handling.
+    /// These will be resolved during initialisation from the command line,
+    /// environment variables, a config file or the supplied default.
+    auto add_configuration_option(
+        std::string const& option,
+        std::string const& description,
+        int default_value) -> ChainAddConfigurationOption;
+
+    /// Add user configuration option(s) to Mir's option handling.
+    /// These will be resolved during initialisation from the command line,
+    /// environment variables, a config file or the supplied default.
+    auto add_configuration_option(
+        std::string const& option,
+        std::string const& description,
+        std::string const& default_value) -> ChainAddConfigurationOption;
+
+    /// Add user configuration option(s) to Mir's option handling.
+    /// These will be resolved during initialisation from the command line,
+    /// environment variables, a config file or the supplied default.
+    auto add_configuration_option(
+        std::string const& option,
+        std::string const& description,
+        OptionType type) -> ChainAddConfigurationOption;
 
     /// Set a handler for any command line options Mir does not recognise.
     /// This will be invoked if any unrecognised options are found during initialisation.
@@ -213,5 +217,41 @@
     struct Self;
     std::shared_ptr<Self> const self;
 };
+
+class Server::ChainAddConfigurationOption
+{
+public:
+    // Yes, I know a single forwarding template would cover all three cases.
+    // but this can give clearer error messages.
+    auto operator()(
+        std::string const& option,
+        std::string const& description,
+        int default_value) const -> ChainAddConfigurationOption
+    {
+        return server.add_configuration_option(option, description, default_value);
+    }
+
+    auto operator()(
+        std::string const& option,
+        std::string const& description,
+        std::string const& default_value) const -> ChainAddConfigurationOption
+    {
+        return server.add_configuration_option(option, description, default_value);
+    }
+
+    auto operator()(
+        std::string const& option,
+        std::string const& description,
+        OptionType type) const -> ChainAddConfigurationOption
+    {
+        return server.add_configuration_option(option, description, type);
+    }
+
+private:
+    friend class ::mir::Server;
+    ChainAddConfigurationOption(Server& server) : server(server) {}
+
+    Server& server;
+};
 }
 #endif /* SERVER_H_ */

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2014-10-15 09:49:26 +0000
+++ server-ABI-sha1sums	2014-10-15 09:49:26 +0000
@@ -95,7 +95,7 @@
 993e9f458ffc4288d304413f3fa0b1dcc95a093d  include/server/mir/scene/surface_observer.h
 7ef3e99901168cda296d74d05a979f47bf9c3ff1  include/server/mir/server_action_queue.h
 8d83a51c278b8b71866d2178d9b6387c1f91a7d0  include/server/mir/server_configuration.h
-5d79d4a973b597d7cc1de766bd0ef3d30b389e18  include/server/mir/server.h
+b1c55e8baf0ba00f67adc924694718cfbf87d4ea  include/server/mir/server.h
 86098b500339bfccd07a9bed8298f75a68b18f5c  include/server/mir/server_status_listener.h
 860c04f32b60e680140148dc9dc2295de145b9c1  include/server/mir/shell/display_layout.h
 6a2107b01feae13060d5c305804906e53c52e0be  include/server/mir/shell/focus_controller.h

=== modified file 'src/server/server.cpp'
--- src/server/server.cpp	2014-10-15 09:49:26 +0000
+++ src/server/server.cpp	2014-10-15 09:49:26 +0000
@@ -277,10 +277,10 @@
 
 #undef MIR_SERVER_WRAP
 
-void mir::Server::add_configuration_option(
+auto mir::Server::add_configuration_option(
     std::string const& option,
     std::string const& description,
-    int default_)
+    int default_) -> ChainAddConfigurationOption
 {
     namespace po = boost::program_options;
 
@@ -295,12 +295,14 @@
         };
 
     self->set_add_configuration_options(option_adder);
+
+    return {*this};
 }
 
-void mir::Server::add_configuration_option(
+auto mir::Server::add_configuration_option(
     std::string const& option,
     std::string const& description,
-    std::string const& default_)
+    std::string const& default_) -> ChainAddConfigurationOption
 {
     namespace po = boost::program_options;
 
@@ -315,12 +317,14 @@
         };
 
     self->set_add_configuration_options(option_adder);
+
+    return {*this};
 }
 
-void mir::Server::add_configuration_option(
+auto mir::Server::add_configuration_option(
     std::string const& option,
     std::string const& description,
-    OptionType type)
+    OptionType type) -> ChainAddConfigurationOption
 {
     namespace po = boost::program_options;
 
@@ -370,4 +374,6 @@
         }
         break;
     }
+
+    return {*this};
 }