Code review comment for lp://staging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage

> +++ src/platforms/mirserver/miral/display_configuration_storage.h
> Copyright 2017 now

Done

>
> +struct DisplayOutputOptions
> To be clear, this struct is filled in to save a subset of options for the
> display. So one could set the display to not be used, or used, or else leave
> unset to allow shell to decide?
>
> +++ src/platforms/mirserver/miral/persist_display_config.cpp
> + for(auto iter = output.modes.cbegin(); iter != output.modes.cend(); ++iter,
> i++) {
> + if ((*iter).size == config.size.value()) {
> + mode_index = i;
> break?
>
> + if (output_index == config.clone_output_index.value()) {
> + output.top_left = find_output.top_left;
> break again?
>
> + if (config.orientation.is_set()) {output.orientation =
> config.orientation.value(); }
> + if (config.used.is_set()) {output.used = config.used.value();
> }
> + if (config.form_factor.is_set()) {output.form_factor =
> config.form_factor.value(); }
> + if (config.scale.is_set()) {output.scale = config.scale.value(); }
>
> What if these values were set, and then we want to unset them? Does that make
> sense?
>
> This is all wrapped in a giant try/catch block, why?
>
>
> void PersistDisplayConfigPolicy::save_config(mg::DisplayConfiguration const&
> conf)
> {
> + if (!storage) return;
> better to throw really
>
> Big try/catch block again? Why?
>
> + // FIXME - output.edid should be std::vector<uint8_t>, not
> std::vector<uint8_t const>
> + edid.parse_data(reinterpret_cast<std::vector<uint8_t> const&>(output.edid));
> IMO std::vector<uint8_t const> is more correct, why would we want the vector
> contents to be editable? miral::Edid should take the const one instead.
>
> Another break makes sense when you find the clone output indev.