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

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

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