Code review comment for lp:~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?

This is used for both saving & loading options which the shell will need.
For saving, all the options will be set by qtmir from the mir display config and passed to the shell. The shell can decide what it want's to save.
For loading, the shell loads and sets whatever value is found in the saved config (if any). Qtmir will apply everything that has been set to the mir display config.

>
> +++ 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?

Done.

>
> + if (output_index == config.clone_output_index.value()) {
> + output.top_left = find_output.top_left;
> break again?

Can't break out of a for_each_output callback

>
> + 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?

edid.parse_data throws an exception if it encounters bad data.
Would log out something, but this is qtmir::miral so wasn't sure what we do.

>
>
> void PersistDisplayConfigPolicy::save_config(mg::DisplayConfiguration const&
> conf)
> {
> + if (!storage) return;
> better to throw really

done.

>
> Big try/catch block again? Why?

Same again.

>
> + // 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.

Apparently vector elements can't be const :/ not sure why it's constructable, but it's not usable. Get a compiler error. something to do with the allocator.

>
> Another break makes sense when you find the clone output indev.

Can't break out of a for_each_output callback