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

Revision history for this message
Gerry Boland (gerboland) wrote :

+++ src/platforms/mirserver/miral/edid.cpp
+ std::ostringstream stringStream;
+ stringStream << "Incorrect EDID structure size {" << data.size() << "}";
+ throw std::runtime_error(stringStream.str());

This would do:
throw std::runtime_error(std::string("Incorrect EDID structure size:") + std::to_string(data.size()));

++ src/platforms/mirserver/miral/persist_display_config.cpp
since you remove the #if MIR_SERVER_VERSION block, please merge the list of mir headers so it is neater.

+++ src/platforms/mirserver/miral/persist_display_config.cpp
Nitpick: space after the "for":
+ for(auto iter = output.modes.cbegin(); iter != output.modes.cend(); ++iter, i++) {

+++ src/platforms/mirserver/qmirserver_p.cpp
Nitpick: please add "// namespace" after the anonymous namespace closing brace

Rest looks good

review: Needs Fixing

« Back to merge proposal