Merge into trunk : miral-DisplayConfigurationStorage : Code : QtMir

Status:	Needs review
Proposed branch:	lp:~nick-dedekind/qtmir/miral-DisplayConfigurationStorage
Merge into:	lp:qtmir
Prerequisite:	lp:~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:~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.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:~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:~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:~nick-dedekind/qtmir/miral-DisplayConfigurationStorage into lp:qtmir

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

 === modified file 'src/platforms/mirserver/miral/CMakeLists.txt'
 --- src/platforms/mirserver/miral/CMakeLists.txt	2017-03-15 14:57:23 +0000
 +++ src/platforms/mirserver/miral/CMakeLists.txt	2017-03-31 16:11:02 +0000
@@ -2,6 +2,7 @@
  add_library(miral-prototypes OBJECT
      edid.cpp edid.h
++    display_configuration_storage.h
      persist_display_config.cpp persist_display_config.h
      mirbuffer.cpp mirbuffer.h
+ )
 === added file 'src/platforms/mirserver/miral/display_configuration_storage.h'
 --- src/platforms/mirserver/miral/display_configuration_storage.h	1970-01-01 00:00:00 +0000
 +++ src/platforms/mirserver/miral/display_configuration_storage.h	2017-03-31 16:11:02 +0000
@@ -0,0 +1,59 @@
++/*
++ * Copyright © 2016-2017 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Nick Dedekind <nick.dedekind@canonical.com>
++ */
++
++#ifndef MIRAL_DISPLAY_CONFIGURATION_STORAGE_H
++#define MIRAL_DISPLAY_CONFIGURATION_STORAGE_H
++
++#include "mir/geometry/rectangle.h"
++#include "mir/optional_value.h"
++#include "mir_toolkit/common.h"
++
++#include "edid.h"
++
++// Prototyping namespace for later incorporation in MirAL
++namespace miral
++{
++
++struct DisplayId
++{
++    Edid edid;
++    int output_id; // helps to identify a monitor if we have two of the same.
++};
++
++struct DisplayConfigurationOptions
++{
++    mir::optional_value<bool> used;
++    mir::optional_value<uint> clone_output_index;
++    mir::optional_value<mir::geometry::Size> size;
++    mir::optional_value<MirOrientation> orientation;
++    mir::optional_value<MirFormFactor> form_factor;
++    mir::optional_value<float> scale;
++};
++
++class DisplayConfigurationStorage
++{
++public:
++    virtual ~DisplayConfigurationStorage() = default;
++
++    virtual void save(const DisplayId&, const DisplayConfigurationOptions&) = 0;
++    virtual bool load(const DisplayId&, DisplayConfigurationOptions&) const = 0;
++};
++
++}
++
++#endif // MIRAL_DISPLAY_CONFIGURATION_STORAGE_H
 === modified file 'src/platforms/mirserver/miral/edid.cpp'
 --- src/platforms/mirserver/miral/edid.cpp	2017-01-06 11:47:03 +0000
 +++ src/platforms/mirserver/miral/edid.cpp	2017-03-31 16:11:02 +0000
@@ -25,7 +25,7 @@
  miral::Edid& miral::Edid::parse_data(std::vector<uint8_t> const& data)
+ {
      if (data.size() != 128 && data.size() != 256) {
--        throw std::runtime_error("Incorrect EDID structure size");
++        throw std::runtime_error(std::string("Incorrect EDID structure size:") + std::to_string(data.size()));
+     }
      // check the checksum
 === modified file 'src/platforms/mirserver/miral/persist_display_config.cpp'
 --- src/platforms/mirserver/miral/persist_display_config.cpp	2017-02-02 13:13:39 +0000
 +++ src/platforms/mirserver/miral/persist_display_config.cpp	2017-03-31 16:11:02 +0000
@@ -17,15 +17,14 @@
   */
  #include "persist_display_config.h"
++#include "display_configuration_storage.h"
++#include "edid.h"
  #include <mir/graphics/display_configuration_policy.h>
--#include <mir/server.h>
--#include <mir/version.h>
--
--#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0)
++#include <mir/graphics/display_configuration.h>
  #include <mir/graphics/display_configuration_observer.h>
  #include <mir/observer_registrar.h>
--#endif
++#include <mir/server.h>
  namespace mg = mir::graphics;
@@ -33,13 +32,16 @@
+ {
  struct PersistDisplayConfigPolicy
+ {
--    PersistDisplayConfigPolicy() = default;
++    PersistDisplayConfigPolicy(std::shared_ptr<miral::DisplayConfigurationStorage> const& storage) :
++        storage(storage) {}
      virtual ~PersistDisplayConfigPolicy() = default;
      PersistDisplayConfigPolicy(PersistDisplayConfigPolicy const&) = delete;
      auto operator=(PersistDisplayConfigPolicy const&) -> PersistDisplayConfigPolicy& = delete;
      void apply_to(mg::DisplayConfiguration& conf, mg::DisplayConfigurationPolicy& default_policy);
      void save_config(mg::DisplayConfiguration const& base_conf);
++
++    std::shared_ptr<miral::DisplayConfigurationStorage> storage;
  };
  struct DisplayConfigurationPolicyAdapter : mg::DisplayConfigurationPolicy
@@ -59,7 +61,6 @@
      std::shared_ptr<mg::DisplayConfigurationPolicy> const default_policy;
  };
--#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0)
  struct DisplayConfigurationObserver : mg::DisplayConfigurationObserver
+ {
      void initial_configuration(std::shared_ptr<mg::DisplayConfiguration const> const& /*config*/) override {}
@@ -74,21 +75,20 @@
          std::shared_ptr<mg::DisplayConfiguration const> const& /*failed_fallback*/,
          std::exception const& /*error*/) override {}
  };
--#else
--struct DisplayConfigurationObserver { };
--#endif
+ }
  struct miral::PersistDisplayConfig::Self : PersistDisplayConfigPolicy, DisplayConfigurationObserver
+ {
--    Self() = default;
--    Self(DisplayConfigurationPolicyWrapper const& custom_wrapper) :
++    Self(std::shared_ptr<DisplayConfigurationStorage> const& storage) :
++        PersistDisplayConfigPolicy(storage) {}
++    Self(std::shared_ptr<DisplayConfigurationStorage> const& storage,
++         DisplayConfigurationPolicyWrapper const& custom_wrapper) :
++        PersistDisplayConfigPolicy(storage),
          custom_wrapper{custom_wrapper} {}
      DisplayConfigurationPolicyWrapper const custom_wrapper =
          [](const std::shared_ptr<mg::DisplayConfigurationPolicy> &wrapped) { return wrapped; };
--#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0)
      void base_configuration_updated(std::shared_ptr<mg::DisplayConfiguration const> const& base_config) override
+     {
          save_config(*base_config);
@@ -97,16 +97,16 @@
      void session_configuration_applied(std::shared_ptr<mir::frontend::Session> const&,
                                         std::shared_ptr<mg::DisplayConfiguration> const&){}
      void session_configuration_removed(std::shared_ptr<mir::frontend::Session> const&)  {}
--#endif
  };
--miral::PersistDisplayConfig::PersistDisplayConfig() :
--    self{std::make_shared<Self>()}
++miral::PersistDisplayConfig::PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage) :
++    self{std::make_shared<Self>(storage)}
+ {
+ }
--miral::PersistDisplayConfig::PersistDisplayConfig(DisplayConfigurationPolicyWrapper const& custom_wrapper) :
--    self{std::make_shared<Self>(custom_wrapper)}
++miral::PersistDisplayConfig::PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage,
++                                                  DisplayConfigurationPolicyWrapper const& custom_wrapper) :
++    self{std::make_shared<Self>(storage, custom_wrapper)}
+ {
+ }
@@ -125,27 +125,99 @@
              return std::make_shared<DisplayConfigurationPolicyAdapter>(self, self->custom_wrapper(wrapped));
          });
--#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0)
      server.add_init_callback([this, &server]
          { server.the_display_configuration_observer_registrar()->register_interest(self); });
--#else
--    // Up to Mir-0.25 detecting changes to the base display config is only possible client-side
--    // (and gives a different configuration API)
--    // If we decide implementing this is necessary for earlier Mir versions then this is where to plumb it in.
--    (void)&PersistDisplayConfigPolicy::save_config; // Fake "using" the function for now
--#endif
+ }
  void PersistDisplayConfigPolicy::apply_to(
      mg::DisplayConfiguration& conf,
      mg::DisplayConfigurationPolicy& default_policy)
+ {
--    // TODO if the h/w profile (by some definition) has changed, then apply corresponding saved config (if any).
--    // TODO Otherwise...
      default_policy.apply_to(conf);
++
++    if (!storage) {
++        throw std::runtime_error("No display configuration storage supplied.");
++    }
++
++    conf.for_each_output([this, &conf](mg::UserDisplayConfigurationOutput& output) {
++        if (!output.connected) return;
++
++        try {
++            miral::DisplayId display_id;
++            // FIXME - output.edid should be std::vector<uint8_t>, not std::vector<uint8_t const>
++            display_id.edid.parse_data(reinterpret_cast<std::vector<uint8_t> const&>(output.edid));
++            display_id.output_id = output.id.as_value();
++
++            // TODO if the h/w profile (by some definition) has changed, then apply corresponding saved config (if any).
++            // TODO Otherwise...
++
++            miral::DisplayConfigurationOptions config;
++            if (storage->load(display_id, config)) {
++
++                if (config.size.is_set()) {
++                    int mode_index = output.current_mode_index;
++                    int i = 0;
++                    // Find the mode index which supports the saved size.
++                    for (auto iter = output.modes.cbegin(); iter != output.modes.cend(); ++iter, i++) {
++                        if ((*iter).size == config.size.value()) {
++                            mode_index = i;
++                            break;
++                        }
++                    }
++                    output.current_mode_index = mode_index;
++                }
++
++                uint output_index = 0;
++                conf.for_each_output([this, &output, config, &output_index](mg::DisplayConfigurationOutput const& find_output) {
++                    if (output_index == config.clone_output_index.value()) {
++                        output.top_left = find_output.top_left;
++                    }
++                    output_index++;
++                });
++
++                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(); }
++            }
++        } catch (std::runtime_error const& e) {
++            printf("Failed to parse EDID - %s\n", e.what());
++        }
++    });
+ }
--void PersistDisplayConfigPolicy::save_config(mg::DisplayConfiguration const& /*base_conf*/)
++void PersistDisplayConfigPolicy::save_config(mg::DisplayConfiguration const& conf)
+ {
--    // TODO save display config options against the h/w profile
++    if (!storage) return;
++
++    conf.for_each_output([this, &conf](mg::DisplayConfigurationOutput const& output) {
++        if (!output.connected) return;
++
++        try {
++            miral::DisplayId display_id;
++            // FIXME - output.edid should be std::vector<uint8_t>, not std::vector<uint8_t const>
++            display_id.edid.parse_data(reinterpret_cast<std::vector<uint8_t> const&>(output.edid));
++            display_id.output_id = output.id.as_value();
++
++            miral::DisplayConfigurationOptions config;
++
++            uint output_index = 0;
++            conf.for_each_output([this, output, &config, &output_index](mg::DisplayConfigurationOutput const& find_output) {
++                if (!config.clone_output_index.is_set() && output.top_left == find_output.top_left) {
++                    config.clone_output_index = output_index;
++                }
++                output_index++;
++            });
++
++            config.size = output.extents().size;
++            config.form_factor = output.form_factor;
++            config.orientation = output.orientation;
++            config.scale = output.scale;
++            config.used = output.used;
++
++            storage->save(display_id, config);
++        } catch (std::runtime_error const& e) {
++            printf("Failed to parse EDID - %s\n", e.what());
++        }
++    });
+ }
 === modified file 'src/platforms/mirserver/miral/persist_display_config.h'
 --- src/platforms/mirserver/miral/persist_display_config.h	2017-01-05 14:36:10 +0000
 +++ src/platforms/mirserver/miral/persist_display_config.h	2017-03-31 16:11:02 +0000
@@ -27,11 +27,13 @@
  // Prototyping namespace for later incorporation in MirAL
  namespace miral
+ {
++class DisplayConfigurationStorage;
++
  /// Restores the saved display configuration and saves changes to the base configuration
  class PersistDisplayConfig
+ {
  public:
--    PersistDisplayConfig();
++    PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage);
      ~PersistDisplayConfig();
      PersistDisplayConfig(PersistDisplayConfig const&);
      auto operator=(PersistDisplayConfig const&) -> PersistDisplayConfig&;
@@ -39,7 +41,9 @@
      // TODO factor this out better
      using DisplayConfigurationPolicyWrapper =
          std::function<std::shared_ptr<mir::graphics::DisplayConfigurationPolicy>(const std::shared_ptr<mir::graphics::DisplayConfigurationPolicy> &wrapped)>;
--    PersistDisplayConfig(DisplayConfigurationPolicyWrapper const& custom_wrapper);
++
++    PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage,
++                         DisplayConfigurationPolicyWrapper const& custom_wrapper);
      void operator()(mir::Server& server);
@@ -47,6 +51,7 @@
      struct Self;
      std::shared_ptr<Self> self;
  };
++
+ }
  #endif //MIRAL_PERSIST_DISPLAY_CONFIG_H
 === modified file 'src/platforms/mirserver/qmirserver_p.cpp'
 --- src/platforms/mirserver/qmirserver_p.cpp	2017-03-28 17:13:24 +0000
 +++ src/platforms/mirserver/qmirserver_p.cpp	2017-03-31 16:11:02 +0000
@@ -15,6 +15,7 @@
   */
  #include "qmirserver_p.h"
++#include "miral/display_configuration_storage.h"
  // local
  #include "logging.h"
@@ -42,9 +43,19 @@
  #include <valgrind.h>
++namespace
++{
  static int qtmirArgc{1};
  static const char *qtmirArgv[]{"qtmir"};
++struct DefaultDisplayConfigurationStorage : miral::DisplayConfigurationStorage
++{
++    void save(const miral::DisplayId&, const miral::DisplayConfigurationOptions&) override {}
++
++    bool load(const miral::DisplayId&, miral::DisplayConfigurationOptions&) const override { return false; }
++};
++} // namespace
++
  void MirServerThread::run()
+ {
      auto start_callback = [this]
@@ -138,7 +149,8 @@
              addInitCallback,
              qtmir::SetQtCompositor{screensModel},
              setTerminator,
--            miral::PersistDisplayConfig{&qtmir::wrapDisplayConfigurationPolicy}
++            miral::PersistDisplayConfig{std::make_shared<DefaultDisplayConfigurationStorage>(),
++                                        &qtmir::wrapDisplayConfigurationPolicy}
          });
+ }