Removing the scribe in favor of combining it with the policy

include/platform/mir/graphics/display_configuration_policy.h

@@ -30,6 +30,7 @@ class DisplayConfigurationPolicy
     virtual ~DisplayConfigurationPolicy() = default;
 
     virtual void apply_to(DisplayConfiguration& conf) = 0;
+    virtual void confirm(DisplayConfiguration& conf) = 0;
 
 protected:
     DisplayConfigurationPolicy() = default;

src/include/server/mir/default_server_configuration.h

@@ -104,7 +104,6 @@ class Platform;
 class Display;
 class DisplayReport;
 class DisplayConfigurationObserver;
-class DisplayConfigurationScribe;
 class GraphicBufferAllocator;
 class Cursor;
 class CursorImage;
@@ -217,7 +216,6 @@ class DefaultServerConfiguration : public virtual ServerConfiguration
     virtual std::shared_ptr<renderer::RendererFactory>   the_renderer_factory();
     virtual std::shared_ptr<shell::DisplayConfigurationController> the_display_configuration_controller();
     virtual std::shared_ptr<graphics::DisplayConfigurationPolicy> the_display_configuration_policy();
-    virtual std::shared_ptr<graphics::DisplayConfigurationScribe> the_display_configuration_scribe();
 
     virtual std::shared_ptr<graphics::GLConfig> the_gl_config();
     /** @} */
@@ -416,7 +414,6 @@ class DefaultServerConfiguration : public virtual ServerConfiguration
     CachedPtr<MainLoop> main_loop;
     CachedPtr<ServerStatusListener> server_status_listener;
     CachedPtr<graphics::DisplayConfigurationPolicy> display_configuration_policy;
-    CachedPtr<graphics::DisplayConfigurationScribe> display_configuration_scribe;
     CachedPtr<scene::MediatingDisplayChanger> mediating_display_changer;
     CachedPtr<graphics::GLConfig> gl_config;
     CachedPtr<scene::PromptSessionListener> prompt_session_listener;

src/include/server/mir/graphics/default_display_configuration_policy.h

@@ -29,21 +29,24 @@ namespace graphics
 class CloneDisplayConfigurationPolicy : public DisplayConfigurationPolicy
 {
 public:
-    void apply_to(DisplayConfiguration& conf);
+    void apply_to(DisplayConfiguration& conf) override;
+    void confirm(DisplayConfiguration& conf) override;
 };
 
 /// Each screen placed to the right of the previous one
 class SideBySideDisplayConfigurationPolicy : public DisplayConfigurationPolicy
 {
 public:
-    void apply_to(graphics::DisplayConfiguration& conf);
+    void apply_to(graphics::DisplayConfiguration& conf) override;
+    void confirm(DisplayConfiguration& conf) override;
 };
 
 /// Just use the first screen
 class SingleDisplayConfigurationPolicy : public DisplayConfigurationPolicy
 {
 public:
-    void apply_to(graphics::DisplayConfiguration& conf);
+    void apply_to(graphics::DisplayConfiguration& conf) override;
+    void confirm(DisplayConfiguration& conf) override;
 };
 /** @} */
 }

src/include/server/mir/server.h

@@ -328,8 +328,6 @@ class Server
     /// Sets a wrapper functor for creating the display configuration policy.
     void wrap_display_configuration_policy(Wrapper<graphics::DisplayConfigurationPolicy> const& wrapper);
 
-    void override_the_display_configuration_scribe(Builder<graphics::DisplayConfigurationScribe> const& display_config_author);
-
     /// Sets a wrapper functor for creating the shell.
     void wrap_shell(Wrapper<shell::Shell> const& wrapper);
 
@@ -438,9 +436,6 @@ class Server
     auto the_seat_observer_registrar() const ->
         std::shared_ptr<ObserverRegistrar<input::SeatObserver>>;
 
-    auto the_display_configuration_scribe() const ->
-        std::shared_ptr<graphics::DisplayConfigurationScribe>;
-
 /** @} */
 
 /** @name Client side support

src/miral/display_configuration_option.cpp

@@ -51,6 +51,7 @@ class PixelFormatSelector : public mg::DisplayConfigurationPolicy
 public:
     PixelFormatSelector(std::shared_ptr<mg::DisplayConfigurationPolicy> const& base_policy, bool with_alpha);
     virtual void apply_to(mg::DisplayConfiguration& conf);
+    virtual void confirm(mg::DisplayConfiguration& conf);
 private:
     std::shared_ptr<mg::DisplayConfigurationPolicy> const base_policy;
     bool const with_alpha;
@@ -61,73 +62,6 @@ bool contains_alpha(MirPixelFormat format)
     return (format == mir_pixel_format_abgr_8888 ||
             format == mir_pixel_format_argb_8888);
 }
-
-struct ParsedOptions
-{
-    enum class ParsedOptionsType
-    {
-        none,
-        side_by_side,
-        single_opt,
-        static_opt
-    };
-    ParsedOptionsType display_layout;
-    double scale{0};
-    bool with_alpha;
-    std::string static_directory;
-};
-
-auto parse_options(mir::Server& server) -> ParsedOptions
-{
-    auto const options = server.get_options();
-    auto display_layout = options->get<std::string>(display_config_opt);
-    auto with_alpha = options->get<std::string>(display_alpha_opt) == display_alpha_on;
-    auto const scale_str = options->get<std::string>(display_scale_opt);
-    std::string static_directory;
-
-    double scale{0};
-    static double const scale_min = 0.01;
-    static double const scale_max = 100.0;
-    try
-    {
-        scale = std::stod(scale_str);
-    }
-    catch (std::invalid_argument const&)
-    {
-        mir::fatal_error("Failed to parse scale '%s' as double", scale_str.c_str());
-    }
-    if (scale < scale_min || scale > scale_max)
-    {
-        mir::fatal_error("Invalid scale %f, must be between %f and %f", scale, scale_min, scale_max);
-    }
-
-    ParsedOptions::ParsedOptionsType option_type = ParsedOptions::ParsedOptionsType::none;
-    if (display_layout == sidebyside_opt_val)
-    {
-        option_type = ParsedOptions::ParsedOptionsType::side_by_side;
-    }
-    else if (display_layout == single_opt_val)
-    {
-        option_type = ParsedOptions::ParsedOptionsType::single_opt;
-    }
-    else if (display_layout.compare(0, strlen(static_opt_val), static_opt_val) == 0)
-    {
-        if (scale != 1.0)
-        {
-            mir::fatal_error("Display scale option can't be used with static display configuration");
-        }
-        option_type = ParsedOptions::ParsedOptionsType::static_opt;
-        static_directory = display_layout.substr(strlen(static_opt_val));
-    }
-
-    return {
-        option_type,
-        scale,
-        with_alpha,
-        static_directory
-    };
-};
-
 }
 
 PixelFormatSelector::PixelFormatSelector(
@@ -161,6 +95,11 @@ void PixelFormatSelector::apply_to(mg::DisplayConfiguration& conf)
         });
 }
 
+void PixelFormatSelector::confirm(mg::DisplayConfiguration& conf)
+{
+    base_policy->confirm(conf);
+}
+
 class ScaleSetter : public mg::DisplayConfigurationPolicy
 {
 public:
@@ -171,6 +110,7 @@ class ScaleSetter : public mg::DisplayConfigurationPolicy
     }
 
     void apply_to(mg::DisplayConfiguration& conf) override;
+    void confirm(mg::DisplayConfiguration& conf) override;
 private:
     std::shared_ptr<mg::DisplayConfigurationPolicy> const base_policy;
     float const with_scale;
@@ -186,58 +126,69 @@ void ScaleSetter::apply_to(mg::DisplayConfiguration& conf)
         });
 }
 
+void ScaleSetter::confirm(mg::DisplayConfiguration&)
+{
+}
+
 void miral::display_configuration_options(mir::Server& server)
 {
     // Add choice of monitor configuration
     server.add_configuration_option(display_config_opt, display_config_descr,   sidebyside_opt_val);
     server.add_configuration_option(display_alpha_opt,  display_alpha_descr,    display_alpha_off);
     server.add_configuration_option(display_scale_opt,  display_scale_descr,    display_scale_default);
 
-    server.override_the_display_configuration_scribe([&] -> std::shared_ptr<mg::DisplayConfigurationScribe>
-    {
-        auto options = parse_options(server);
-        if (options.display_layout == ParsedOptions::ParsedOptionsType::static_opt)
-        {
-            return std::make_shared<StaticDisplayConfig>(options.static_directory);
-        }
-
-        class NullDisplayConfigurationScribe : public mir::graphics::DisplayConfigurationScribe
-        {
-        public:
-            void write(const mir::graphics::DisplayConfiguration&) override {}
-        };
-
-        return std::make_shared<NullDisplayConfigurationScribe>();
-    });
-
     server.wrap_display_configuration_policy(
         [&](std::shared_ptr<mg::DisplayConfigurationPolicy> const& wrapped)
         -> std::shared_ptr<mg::DisplayConfigurationPolicy>
         {
-            auto options = parse_options(server);
+            auto const options = server.get_options();
+            auto display_layout = options->get<std::string>(display_config_opt);
+            auto with_alpha = options->get<std::string>(display_alpha_opt) == display_alpha_on;
+            auto const scale_str = options->get<std::string>(display_scale_opt);
+
+            double scale{0};
+            static double const scale_min = 0.01;
+            static double const scale_max = 100.0;
+            try
+            {
+                scale = std::stod(scale_str);
+            }
+            catch (std::invalid_argument const&)
+            {
+                mir::fatal_error("Failed to parse scale '%s' as double", scale_str.c_str());
+            }
+            if (scale < scale_min || scale > scale_max)
+            {
+                mir::fatal_error("Invalid scale %f, must be between %f and %f", scale, scale_min, scale_max);
+            }
+
             auto layout_selector = wrapped;
 
-            if (options.display_layout == ParsedOptions::ParsedOptionsType::side_by_side)
+            if (display_layout == sidebyside_opt_val)
             {
                 layout_selector = std::make_shared<mg::SideBySideDisplayConfigurationPolicy>();
             }
-            else if (options.display_layout == ParsedOptions::ParsedOptionsType::single_opt)
+            else if (display_layout == single_opt_val)
             {
                 layout_selector = std::make_shared<mg::SingleDisplayConfigurationPolicy>();
             }
-            else if (options.display_layout == ParsedOptions::ParsedOptionsType::static_opt)
+            else if (display_layout.compare(0, strlen(static_opt_val), static_opt_val) == 0)
             {
-                auto sdc = std::make_shared<StaticDisplayConfig>(options.static_directory);
+                if (scale != 1.0)
+                {
+                    mir::fatal_error("Display scale option can't be used with static display configuration");
+                }
+                auto sdc = std::make_shared<StaticDisplayConfig>(display_layout.substr(strlen(static_opt_val)));
                 server.add_init_callback([sdc, &server]{ sdc->init_auto_reload(server); });
                 layout_selector = std::move(sdc);
             }
 
-            if (options.scale != 1.0)
+            if (scale != 1.0)
             {
-                layout_selector = std::make_shared<ScaleSetter>(layout_selector, options.scale);
+                layout_selector = std::make_shared<ScaleSetter>(layout_selector, scale);
             }
 
             // Whatever the layout select a pixel format with requested alpha
-            return std::make_shared<PixelFormatSelector>(layout_selector, options.with_alpha);
+            return std::make_shared<PixelFormatSelector>(layout_selector, with_alpha);
         });
 }

src/miral/static_display_config.cpp

@@ -329,6 +329,10 @@ void miral::YamlFileDisplayConfig::apply_to(mg::DisplayConfiguration& conf)
     mir::log_info(out.str());
 }
 
+void miral::YamlFileDisplayConfig::confirm(mir::graphics::DisplayConfiguration&)
+{
+}
+
 void miral::YamlFileDisplayConfig::apply_default_configuration(mg::DisplayConfiguration& conf)
 {
     conf.for_each_output([config=Config{}](mg::UserDisplayConfigurationOutput& conf_output)
@@ -587,7 +591,7 @@ void miral::ReloadingYamlFileDisplayConfig::check_for_layout_override()
     }
 }
 
-void miral::ReloadingYamlFileDisplayConfig::write(const mir::graphics::DisplayConfiguration &conf)
+void miral::ReloadingYamlFileDisplayConfig::confirm(mir::graphics::DisplayConfiguration& conf)
 {
     if (!config_path_.has_value())
     {

src/miral/static_display_config.h

@@ -21,7 +21,6 @@
 #include <mir/fd.h>
 #include <mir/graphics/default_display_configuration_policy.h>
 #include <mir/graphics/display_configuration.h>
-#include <mir/graphics/display_configuration_scribe.h>
 #include <mir/log.h>
 
 #include <functional>
@@ -46,6 +45,7 @@ class YamlFileDisplayConfig : public mir::graphics::DisplayConfigurationPolicy
     void load_config(std::istream& config_file, std::string const& filename);
 
     void apply_to(mir::graphics::DisplayConfiguration& conf) override;
+    virtual void confirm(mir::graphics::DisplayConfiguration& conf) override;
 
     void select_layout(std::string const& layout);
 
@@ -80,7 +80,7 @@ class YamlFileDisplayConfig : public mir::graphics::DisplayConfigurationPolicy
     static void apply_to_output(mir::graphics::UserDisplayConfigurationOutput& conf_output, Config const& conf);
 };
 
-class ReloadingYamlFileDisplayConfig : public YamlFileDisplayConfig, public mir::graphics::DisplayConfigurationScribe
+class ReloadingYamlFileDisplayConfig : public YamlFileDisplayConfig
 {
 public:
     explicit ReloadingYamlFileDisplayConfig(std::string basename);
@@ -92,7 +92,8 @@ class ReloadingYamlFileDisplayConfig : public YamlFileDisplayConfig, public mir:
     void config_path(std::string newpath);
 
     void check_for_layout_override();
-    void write(mir::graphics::DisplayConfiguration const& conf) override;
+
+    void confirm(mir::graphics::DisplayConfiguration& conf) override;
 
 private:
     auto the_main_loop() const -> std::shared_ptr<mir::MainLoop>;

src/miroil/persist_display_config.cpp

@@ -77,6 +77,10 @@ struct DisplayConfigurationPolicyAdapter : mg::DisplayConfigurationPolicy
         self->apply_to(conf, *wrapped_policy, *custom_policy);
     }
 
+    void confirm(mg::DisplayConfiguration&) override
+    {
+    }
+
     std::shared_ptr<PersistDisplayConfigPolicy> const self;
     std::shared_ptr<miroil::DisplayConfigurationPolicy> const wrapped_policy;
     std::shared_ptr<miroil::DisplayConfigurationPolicy> const custom_policy;