Merge into trunk : miral-DisplayConfigurationStorage : Code : QtMir

Status:	Needs review
Proposed branch:	lp://staging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage
Merge into:	lp://staging/qtmir
Prerequisite:	lp://staging/~gerboland/qtmir/miral-1.3.1-compat
Diff against target:	377 lines (+182/-33) 6 files modified src/platforms/mirserver/miral/CMakeLists.txt (+1/-0) src/platforms/mirserver/miral/display_configuration_storage.h (+59/-0) src/platforms/mirserver/miral/edid.cpp (+1/-1) src/platforms/mirserver/miral/persist_display_config.cpp (+101/-29) src/platforms/mirserver/miral/persist_display_config.h (+7/-2) src/platforms/mirserver/qmirserver_p.cpp (+13/-1)
To merge this branch:	bzr merge lp://staging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot (community)	continuous-integration	2017-03-20	Approve on 2017-03-31
Gerry Boland (community)		2017-03-20	Needs Fixing on 2017-03-21
Albert Astals Cid (community)		2017-03-20	Abstain on 2017-03-21
Alan Griffiths		2017-03-20	Pending
Review via email: mp+320368@code.staging.launchpad.net

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-12-14: Posted in a previous version of this proposal

#

+ conf.for_each_output([this, &conf](mg::UserDisplayConfigurationOutput& output) {
...
default_policy.apply_to(conf);

First apply any saved configuration and then overwrite that with the default policy. Is this the right behaviour?

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-12-14: Posted in a previous version of this proposal

#

- miral::Application applicationFor(qtmir::PromptSession const &promptSession) const;
+ ::miral::Application applicationFor(qtmir::PromptSession const &promptSession) const;

This (and similar changes elsewhere) shouldn't be needed now.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-01-05: Posted in a previous version of this proposal

#

The target branch has landed, the pre-requisite has been rebased.

review: Needs Resubmitting

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-01-25: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/456/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-01-27: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/470/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-03: Posted in a previous version of this proposal

#

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

review: Needs Fixing

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-02-07: Posted in a previous version of this proposal

#

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

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-02-07: Posted in a previous version of this proposal

#

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

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

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-09: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/483/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-09: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/484/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/501/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14: Posted in a previous version of this proposal

#

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

You're right, you're in a lambda. You could "return" instead, but that does look confusing in code. Best to leave it.

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

Ok, my first instinct is to only to wrap parse_data call. But if the exception throws, then you naturally skip all the config saving bits, which is clean. So ok.

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

There is something fishy here. http://en.cppreference.com/w/cpp/container/vector says that the type in a vector (for C++11 and greater) needs to just be Erasable, which const uint8_t should be. But yeah, compiler says no. Better leave as is.

review: Approve

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14: Posted in a previous version of this proposal

#

I get this error when using Ninja, can you have a look:

FAILED: src/platforms/mirserver/miral/CMakeFiles/miral-prototypes.dir
/usr/lib/ccache/c++ -DQT_NO_DEBUG -DQT_NO_KEYWORDS -DQT_USE_QSTRINGBUILDER -DQT_USING_GL -Isrc/platforms/mirserver/miral -I../src/platforms/mirserver/miral -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/mirserver -isystem /usr/include/mirclient -isystem /usr/include/mircookie -isystem /usr/include/mirplatform -isystem /usr/include/mircommon -isystem /usr/include/uuid -isystem /usr/include/mircore -fPIC -Wall -fno-strict-aliasing -Werror -Wextra -O2 -g -DNDEBUG -std=gnu++14 -MD -MT src/platforms/mirserver/miral/CMakeFiles/miral-prototypes.dir -MF src/platforms/mirserver/miral/CMakeFiles/miral-prototypes.dir/.d -o src/platforms/mirserver/miral/CMakeFiles/miral-prototypes.dir -c /home/gerry/dev/projects/qtmir/miral-DisplayConfigurationStorage/BUILD/src/platforms/mirserver/miral/miral-prototypes_automoc.cpp
Assembler messages:
Fatal error: can't create src/platforms/mirserver/miral/CMakeFiles/miral-prototypes.dir: Is a directory

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-17: Posted in a previous version of this proposal

#

> I get this error when using Ninja, can you have a look:
>
> FAILED: src/platforms/mirserver/miral/CMakeFiles/miral-prototypes.dir
> /usr/lib/ccache/c++ -DQT_NO_DEBUG -DQT_NO_KEYWORDS -DQT_USE_QSTRINGBUILDER
> -DQT_USING_GL -Isrc/platforms/mirserver/miral
> -I../src/platforms/mirserver/miral -isystem /usr/include/x86_64-linux-
> gnu/qt5/QtCore -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem
> /usr/include/mirserver -isystem /usr/include/mirclient -isystem
> /usr/include/mircookie -isystem /usr/include/mirplatform -isystem
> /usr/include/mircommon -isystem /usr/include/uuid -isystem
> /usr/include/mircore -fPIC -Wall -fno-strict-aliasing -Werror -Wextra -O2 -g
> -DNDEBUG -std=gnu++14 -MD -MT src/platforms/mirserver/miral/CMakeFiles
> /miral-prototypes.dir -MF src/platforms/mirserver/miral/CMakeFiles/miral-
> prototypes.dir/.d -o src/platforms/mirserver/miral/CMakeFiles/miral-
> prototypes.dir -c /home/gerry/dev/projects/qtmir/miral-
> DisplayConfigurationStorage/BUILD/src/platforms/mirserver/miral/miral-
> prototypes_automoc.cpp
> Assembler messages:
> Fatal error: can't create src/platforms/mirserver/miral/CMakeFiles/miral-
> prototypes.dir: Is a directory

Not your fault, comes from iteration-0-of-miral-PersistDisplayConfig

review: Approve

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-01: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/537/rebuild

review: Approve (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2017-03-13: Posted in a previous version of this proposal

#

Text conflict in src/platforms/mirserver/qmirserver_p.cpp
1 conflicts encountered.

Was already top approved.

review: Needs Fixing

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-03-14: Posted in a previous version of this proposal

#

> > > + 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
>
> You're right, you're in a lambda. You could "return" instead, but that does
> look confusing in code. Best to leave it.
>
>
> > > + 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.
>
> Ok, my first instinct is to only to wrap parse_data call. But if the exception
> throws, then you naturally skip all the config saving bits, which is clean. So
> ok.
>
>
> > > + // 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.
>
> There is something fishy here.
> http://en.cppreference.com/w/cpp/container/vector says that the type in a
> vector (for C++11 and greater) needs to just be Erasable, which const uint8_t
> should be. But yeah, compiler says no. Better leave as is.

"Erasable, but many member functions impose stricter requirements."
Perhaps this is why constructing one is fine in mir but actually using it in our case is where the "Compiler Says No"

> > > +   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
> 
> You're right, you're in a lambda. You could "return" instead, but that does
> look confusing in code. Best to leave it.
> 
> 
> > > +                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.
> 
> Ok, my first instinct is to only to wrap parse_data call. But if the exception
> throws, then you naturally skip all the config saving bits, which is clean. So
> ok.
> 
> 
> > > +            // 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.
> 
> There is something fishy here.
> http://en.cppreference.com/w/cpp/container/vector says that the type in a
> vector (for C++11 and greater) needs to just be Erasable, which const uint8_t
> should be. But yeah, compiler says no. Better leave as is.

"Erasable, but many member functions impose stricter requirements."
Perhaps this is why constructing one is fine in mir but actually using it in our case is where the "Compiler Says No"

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-03-14: Posted in a previous version of this proposal

#

> Text conflict in src/platforms/mirserver/qmirserver_p.cpp
> 1 conflicts encountered.
>
> Was already top approved.

Fixed.

Revision history for this message

Albert Astals Cid (aacid) on 2017-03-14: Posted in a previous version of this proposal

#

review: Abstain

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-14: Posted in a previous version of this proposal

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/574/rebuild

review: Approve (continuous-integration)

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-03-20:

#

Resubmit with lp:~gerboland/qtmir/miral-1.3.1-compat dep

Reply

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-20:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/590/rebuild

review: Approve (continuous-integration)

Reply

Revision history for this message

Albert Astals Cid (aacid) on 2017-03-21:

#

review: Abstain

Reply

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-21:

#

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

Reply

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

#

FAILED: Continuous integration, rev:615
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/621/
Executed test runs:
FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4655/console
FAILURE: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4683/console

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/621/rebuild

review: Needs Fixing (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage updated on 2017-03-24

616. By Nick Dedekind on 2017-03-24: merged trunk

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/623/rebuild

review: Needs Fixing (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage updated on 2017-03-31

617. By Nick Dedekind on 2017-03-31: merged trunk
618. By Nick Dedekind on 2017-03-31: merged with trunk

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-03-31:

#

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

Fixed.

Reply

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-31:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/653/rebuild

review: Approve (continuous-integration)

Reply

QtMir

Merge lp://staging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage into lp://staging/qtmir

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers