Merge lp:~nick-dedekind/qtmir/miral-DisplayConfigurationStorage into lp:qtmir
- miral-DisplayConfigurationStorage
- Merge into trunk
Status: | Superseded |
---|---|
Proposed branch: | lp:~nick-dedekind/qtmir/miral-DisplayConfigurationStorage |
Merge into: | lp:qtmir |
Prerequisite: | lp:~nick-dedekind/qtmir/edid |
Diff against target: |
398 lines (+185/-32) 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 (+4/-1) src/platforms/mirserver/miral/persist_display_config.cpp (+100/-26) src/platforms/mirserver/miral/persist_display_config.h (+7/-2) src/platforms/mirserver/qmirserver_p.cpp (+14/-3) |
To merge this branch: | bzr merge lp:~nick-dedekind/qtmir/miral-DisplayConfigurationStorage |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Unity8 CI Bot (community) | continuous-integration | Approve | |
Albert Astals Cid (community) | Abstain | ||
Gerry Boland (community) | Approve | ||
Alan Griffiths | Pending | ||
Review via email: mp+315571@code.launchpad.net |
This proposal supersedes a proposal from 2016-12-07.
This proposal has been superseded by a proposal from 2017-03-20.
Commit message
Added interface for saving/loading display configuration output options.
Description of the change
Pass responsibility for display configuration storage to an overridable interface.
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
- miral::Application applicationFor(
+ ::miral:
This (and similar changes elsewhere) shouldn't be needed now.
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
The target branch has landed, the pre-requisite has been rebased.
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:604
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:605
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
Gerry Boland (gerboland) wrote : | # |
+++ src/platforms/
Copyright 2017 now
+struct DisplayOutputOp
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/
+ for(auto iter = output.
+ if ((*iter).size == config.
+ mode_index = i;
break?
+ if (output_index == config.
+ output.top_left = find_output.
break again?
+ if (config.
+ if (config.
+ if (config.
+ if (config.
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 PersistDisplayC
{
+ if (!storage) return;
better to throw really
Big try/catch block again? Why?
+ // FIXME - output.edid should be std::vector<
+ edid.parse_
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.
Nick Dedekind (nick-dedekind) wrote : | # |
> +++ src/platforms/
> Copyright 2017 now
Done
>
> +struct DisplayOutputOp
> 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/
> + for(auto iter = output.
> i++) {
> + if ((*iter).size == config.
> + mode_index = i;
> break?
>
> + if (output_index == config.
> + output.top_left = find_output.
> break again?
>
> + if (config.
> config.
> + if (config.
> }
> + if (config.
> config.
> + if (config.
>
> 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 PersistDisplayC
> conf)
> {
> + if (!storage) return;
> better to throw really
>
> Big try/catch block again? Why?
>
> + // FIXME - output.edid should be std::vector<
> std::vector<uint8_t const>
> + edid.parse_
> 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.
Nick Dedekind (nick-dedekind) wrote : | # |
> +++ src/platforms/
> Copyright 2017 now
Done
>
> +struct DisplayOutputOp
> 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/
> + for(auto iter = output.
> i++) {
> + if ((*iter).size == config.
> + mode_index = i;
> break?
Done.
>
> + if (output_index == config.
> + output.top_left = find_output.
> break again?
Can't break out of a for_each_output callback
>
> + if (config.
> config.
> + if (config.
> }
> + if (config.
> config.
> + if (config.
>
> 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 PersistDisplayC
> conf)
> {
> + if (!storage) return;
> better to throw really
done.
>
> Big try/catch block again? Why?
Same again.
>
> + // FIXME - output.edid should be std::vector<
> std::vector<uint8_t const>
> + edid.parse_
> 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
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:609
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:611
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:612
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
Gerry Boland (gerboland) wrote : | # |
> > + if (output_index == config.
> > + output.top_left = find_output.
> > 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.
> > config.
> > + if (config.
> config.
> > }
> > + if (config.
> > config.
> > + if (config.
> >
> > 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<
> > std::vector<uint8_t const>
> > + edid.parse_
> const&>
> > 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://
Gerry Boland (gerboland) wrote : | # |
I get this error when using Ninja, can you have a look:
FAILED: src/platforms/
/usr/lib/ccache/c++ -DQT_NO_DEBUG -DQT_NO_KEYWORDS -DQT_USE_
Assembler messages:
Fatal error: can't create src/platforms/
Gerry Boland (gerboland) wrote : | # |
> I get this error when using Ninja, can you have a look:
>
> FAILED: src/platforms/
> /usr/lib/ccache/c++ -DQT_NO_DEBUG -DQT_NO_KEYWORDS -DQT_USE_
> -DQT_USING_GL -Isrc/platforms
> -I../src/
> gnu/qt5/QtCore -isystem /usr/include/
> /usr/include/
> /usr/include/
> /usr/include/
> /usr/include/
> -DNDEBUG -std=gnu++14 -MD -MT src/platforms/
> /miral-
> prototypes.dir/.d -o src/platforms/
> prototypes.dir -c /home/gerry/
> DisplayConfigur
> prototypes_
> Assembler messages:
> Fatal error: can't create src/platforms/
> prototypes.dir: Is a directory
Not your fault, comes from iteration-
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:613
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Albert Astals Cid (aacid) wrote : | # |
Text conflict in src/platforms/
1 conflicts encountered.
Was already top approved.
- 614. By Nick Dedekind
-
merged trunk
Nick Dedekind (nick-dedekind) wrote : | # |
> > > + if (output_index == config.
> > > + output.top_left = find_output.
> > > 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.
> > > config.
> > > + if (config.
> > config.
> > > }
> > > + if (config.
> > > config.
> > > + if (config.
> > >
> > > 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<
> > > std::vector<uint8_t const>
> > > + edid.parse_
> > const&>
> > > 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://
> 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"
Nick Dedekind (nick-dedekind) wrote : | # |
> Text conflict in src/platforms/
> 1 conflicts encountered.
>
> Was already top approved.
Fixed.
Albert Astals Cid (aacid) : | # |
Unity8 CI Bot (unity8-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:614
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
- 615. By Nick Dedekind
-
miral 1.3.1 compat
- 616. By Nick Dedekind
-
merged trunk
- 617. By Nick Dedekind
-
merged trunk
- 618. By Nick Dedekind
-
merged with trunk
Unmerged revisions
- 618. By Nick Dedekind
-
merged with trunk
- 617. By Nick Dedekind
-
merged trunk
- 616. By Nick Dedekind
-
merged trunk
- 615. By Nick Dedekind
-
miral 1.3.1 compat
- 614. By Nick Dedekind
-
merged trunk
- 613. By Nick Dedekind
-
merged with trunk
- 612. By Nick Dedekind
-
more info for identifying a display
- 611. By Nick Dedekind
-
print error on failed EDID parse
- 610. By Nick Dedekind
-
added check for ouput.connected to load/save config
- 609. By Nick Dedekind
-
merged parent
Preview Diff
1 | === modified file 'src/platforms/mirserver/miral/CMakeLists.txt' |
2 | --- src/platforms/mirserver/miral/CMakeLists.txt 2017-01-05 14:36:10 +0000 |
3 | +++ src/platforms/mirserver/miral/CMakeLists.txt 2017-03-20 14:53:59 +0000 |
4 | @@ -2,5 +2,6 @@ |
5 | |
6 | add_library(miral-prototypes OBJECT |
7 | edid.cpp edid.h |
8 | + display_configuration_storage.h |
9 | persist_display_config.cpp persist_display_config.h |
10 | ) |
11 | |
12 | === added file 'src/platforms/mirserver/miral/display_configuration_storage.h' |
13 | --- src/platforms/mirserver/miral/display_configuration_storage.h 1970-01-01 00:00:00 +0000 |
14 | +++ src/platforms/mirserver/miral/display_configuration_storage.h 2017-03-20 14:53:59 +0000 |
15 | @@ -0,0 +1,59 @@ |
16 | +/* |
17 | + * Copyright © 2016-2017 Canonical Ltd. |
18 | + * |
19 | + * This program is free software: you can redistribute it and/or modify it |
20 | + * under the terms of the GNU General Public License version 3, |
21 | + * as published by the Free Software Foundation. |
22 | + * |
23 | + * This program is distributed in the hope that it will be useful, |
24 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
25 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
26 | + * GNU General Public License for more details. |
27 | + * |
28 | + * You should have received a copy of the GNU General Public License |
29 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
30 | + * |
31 | + * Authored by: Nick Dedekind <nick.dedekind@canonical.com> |
32 | + */ |
33 | + |
34 | +#ifndef MIRAL_DISPLAY_CONFIGURATION_STORAGE_H |
35 | +#define MIRAL_DISPLAY_CONFIGURATION_STORAGE_H |
36 | + |
37 | +#include "mir/geometry/rectangle.h" |
38 | +#include "mir/optional_value.h" |
39 | +#include "mir_toolkit/common.h" |
40 | + |
41 | +#include "edid.h" |
42 | + |
43 | +// Prototyping namespace for later incorporation in MirAL |
44 | +namespace miral |
45 | +{ |
46 | + |
47 | +struct DisplayId |
48 | +{ |
49 | + Edid edid; |
50 | + int output_id; // helps to identify a monitor if we have two of the same. |
51 | +}; |
52 | + |
53 | +struct DisplayConfigurationOptions |
54 | +{ |
55 | + mir::optional_value<bool> used; |
56 | + mir::optional_value<uint> clone_output_index; |
57 | + mir::optional_value<mir::geometry::Size> size; |
58 | + mir::optional_value<MirOrientation> orientation; |
59 | + mir::optional_value<MirFormFactor> form_factor; |
60 | + mir::optional_value<float> scale; |
61 | +}; |
62 | + |
63 | +class DisplayConfigurationStorage |
64 | +{ |
65 | +public: |
66 | + virtual ~DisplayConfigurationStorage() = default; |
67 | + |
68 | + virtual void save(const DisplayId&, const DisplayConfigurationOptions&) = 0; |
69 | + virtual bool load(const DisplayId&, DisplayConfigurationOptions&) const = 0; |
70 | +}; |
71 | + |
72 | +} |
73 | + |
74 | +#endif // MIRAL_DISPLAY_CONFIGURATION_STORAGE_H |
75 | |
76 | === modified file 'src/platforms/mirserver/miral/edid.cpp' |
77 | --- src/platforms/mirserver/miral/edid.cpp 2017-01-06 11:47:03 +0000 |
78 | +++ src/platforms/mirserver/miral/edid.cpp 2017-03-20 14:53:59 +0000 |
79 | @@ -19,13 +19,16 @@ |
80 | #include "edid.h" |
81 | |
82 | #include <cstring> |
83 | +#include <sstream> |
84 | #include <numeric> |
85 | #include <stdexcept> |
86 | |
87 | miral::Edid& miral::Edid::parse_data(std::vector<uint8_t> const& data) |
88 | { |
89 | if (data.size() != 128 && data.size() != 256) { |
90 | - throw std::runtime_error("Incorrect EDID structure size"); |
91 | + std::ostringstream stringStream; |
92 | + stringStream << "Incorrect EDID structure size {" << data.size() << "}"; |
93 | + throw std::runtime_error(stringStream.str()); |
94 | } |
95 | |
96 | // check the checksum |
97 | |
98 | === modified file 'src/platforms/mirserver/miral/persist_display_config.cpp' |
99 | --- src/platforms/mirserver/miral/persist_display_config.cpp 2017-02-02 13:13:39 +0000 |
100 | +++ src/platforms/mirserver/miral/persist_display_config.cpp 2017-03-20 14:53:59 +0000 |
101 | @@ -17,15 +17,16 @@ |
102 | */ |
103 | |
104 | #include "persist_display_config.h" |
105 | +#include "display_configuration_storage.h" |
106 | +#include "edid.h" |
107 | |
108 | #include <mir/graphics/display_configuration_policy.h> |
109 | +#include <mir/graphics/display_configuration.h> |
110 | #include <mir/server.h> |
111 | #include <mir/version.h> |
112 | |
113 | -#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0) |
114 | #include <mir/graphics/display_configuration_observer.h> |
115 | #include <mir/observer_registrar.h> |
116 | -#endif |
117 | |
118 | namespace mg = mir::graphics; |
119 | |
120 | @@ -33,13 +34,16 @@ |
121 | { |
122 | struct PersistDisplayConfigPolicy |
123 | { |
124 | - PersistDisplayConfigPolicy() = default; |
125 | + PersistDisplayConfigPolicy(std::shared_ptr<miral::DisplayConfigurationStorage> const& storage) : |
126 | + storage(storage) {} |
127 | virtual ~PersistDisplayConfigPolicy() = default; |
128 | PersistDisplayConfigPolicy(PersistDisplayConfigPolicy const&) = delete; |
129 | auto operator=(PersistDisplayConfigPolicy const&) -> PersistDisplayConfigPolicy& = delete; |
130 | |
131 | void apply_to(mg::DisplayConfiguration& conf, mg::DisplayConfigurationPolicy& default_policy); |
132 | void save_config(mg::DisplayConfiguration const& base_conf); |
133 | + |
134 | + std::shared_ptr<miral::DisplayConfigurationStorage> storage; |
135 | }; |
136 | |
137 | struct DisplayConfigurationPolicyAdapter : mg::DisplayConfigurationPolicy |
138 | @@ -59,7 +63,6 @@ |
139 | std::shared_ptr<mg::DisplayConfigurationPolicy> const default_policy; |
140 | }; |
141 | |
142 | -#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0) |
143 | struct DisplayConfigurationObserver : mg::DisplayConfigurationObserver |
144 | { |
145 | void initial_configuration(std::shared_ptr<mg::DisplayConfiguration const> const& /*config*/) override {} |
146 | @@ -74,21 +77,20 @@ |
147 | std::shared_ptr<mg::DisplayConfiguration const> const& /*failed_fallback*/, |
148 | std::exception const& /*error*/) override {} |
149 | }; |
150 | -#else |
151 | -struct DisplayConfigurationObserver { }; |
152 | -#endif |
153 | } |
154 | |
155 | struct miral::PersistDisplayConfig::Self : PersistDisplayConfigPolicy, DisplayConfigurationObserver |
156 | { |
157 | - Self() = default; |
158 | - Self(DisplayConfigurationPolicyWrapper const& custom_wrapper) : |
159 | + Self(std::shared_ptr<DisplayConfigurationStorage> const& storage) : |
160 | + PersistDisplayConfigPolicy(storage) {} |
161 | + Self(std::shared_ptr<DisplayConfigurationStorage> const& storage, |
162 | + DisplayConfigurationPolicyWrapper const& custom_wrapper) : |
163 | + PersistDisplayConfigPolicy(storage), |
164 | custom_wrapper{custom_wrapper} {} |
165 | |
166 | DisplayConfigurationPolicyWrapper const custom_wrapper = |
167 | [](const std::shared_ptr<mg::DisplayConfigurationPolicy> &wrapped) { return wrapped; }; |
168 | |
169 | -#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0) |
170 | void base_configuration_updated(std::shared_ptr<mg::DisplayConfiguration const> const& base_config) override |
171 | { |
172 | save_config(*base_config); |
173 | @@ -97,16 +99,16 @@ |
174 | void session_configuration_applied(std::shared_ptr<mir::frontend::Session> const&, |
175 | std::shared_ptr<mg::DisplayConfiguration> const&){} |
176 | void session_configuration_removed(std::shared_ptr<mir::frontend::Session> const&) {} |
177 | -#endif |
178 | }; |
179 | |
180 | -miral::PersistDisplayConfig::PersistDisplayConfig() : |
181 | - self{std::make_shared<Self>()} |
182 | +miral::PersistDisplayConfig::PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage) : |
183 | + self{std::make_shared<Self>(storage)} |
184 | { |
185 | } |
186 | |
187 | -miral::PersistDisplayConfig::PersistDisplayConfig(DisplayConfigurationPolicyWrapper const& custom_wrapper) : |
188 | - self{std::make_shared<Self>(custom_wrapper)} |
189 | +miral::PersistDisplayConfig::PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage, |
190 | + DisplayConfigurationPolicyWrapper const& custom_wrapper) : |
191 | + self{std::make_shared<Self>(storage, custom_wrapper)} |
192 | { |
193 | } |
194 | |
195 | @@ -125,27 +127,99 @@ |
196 | return std::make_shared<DisplayConfigurationPolicyAdapter>(self, self->custom_wrapper(wrapped)); |
197 | }); |
198 | |
199 | -#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 26, 0) |
200 | server.add_init_callback([this, &server] |
201 | { server.the_display_configuration_observer_registrar()->register_interest(self); }); |
202 | -#else |
203 | - // Up to Mir-0.25 detecting changes to the base display config is only possible client-side |
204 | - // (and gives a different configuration API) |
205 | - // If we decide implementing this is necessary for earlier Mir versions then this is where to plumb it in. |
206 | - (void)&PersistDisplayConfigPolicy::save_config; // Fake "using" the function for now |
207 | -#endif |
208 | } |
209 | |
210 | void PersistDisplayConfigPolicy::apply_to( |
211 | mg::DisplayConfiguration& conf, |
212 | mg::DisplayConfigurationPolicy& default_policy) |
213 | { |
214 | - // TODO if the h/w profile (by some definition) has changed, then apply corresponding saved config (if any). |
215 | - // TODO Otherwise... |
216 | default_policy.apply_to(conf); |
217 | + |
218 | + if (!storage) { |
219 | + throw std::runtime_error("No display configuration storage supplied."); |
220 | + } |
221 | + |
222 | + conf.for_each_output([this, &conf](mg::UserDisplayConfigurationOutput& output) { |
223 | + if (!output.connected) return; |
224 | + |
225 | + try { |
226 | + miral::DisplayId display_id; |
227 | + // FIXME - output.edid should be std::vector<uint8_t>, not std::vector<uint8_t const> |
228 | + display_id.edid.parse_data(reinterpret_cast<std::vector<uint8_t> const&>(output.edid)); |
229 | + display_id.output_id = output.id.as_value(); |
230 | + |
231 | + // TODO if the h/w profile (by some definition) has changed, then apply corresponding saved config (if any). |
232 | + // TODO Otherwise... |
233 | + |
234 | + miral::DisplayConfigurationOptions config; |
235 | + if (storage->load(display_id, config)) { |
236 | + |
237 | + if (config.size.is_set()) { |
238 | + int mode_index = output.current_mode_index; |
239 | + int i = 0; |
240 | + // Find the mode index which supports the saved size. |
241 | + for(auto iter = output.modes.cbegin(); iter != output.modes.cend(); ++iter, i++) { |
242 | + if ((*iter).size == config.size.value()) { |
243 | + mode_index = i; |
244 | + break; |
245 | + } |
246 | + } |
247 | + output.current_mode_index = mode_index; |
248 | + } |
249 | + |
250 | + uint output_index = 0; |
251 | + conf.for_each_output([this, &output, config, &output_index](mg::DisplayConfigurationOutput const& find_output) { |
252 | + if (output_index == config.clone_output_index.value()) { |
253 | + output.top_left = find_output.top_left; |
254 | + } |
255 | + output_index++; |
256 | + }); |
257 | + |
258 | + if (config.orientation.is_set()) {output.orientation = config.orientation.value(); } |
259 | + if (config.used.is_set()) {output.used = config.used.value(); } |
260 | + if (config.form_factor.is_set()) {output.form_factor = config.form_factor.value(); } |
261 | + if (config.scale.is_set()) {output.scale = config.scale.value(); } |
262 | + } |
263 | + } catch (std::runtime_error const& e) { |
264 | + printf("Failed to parse EDID - %s\n", e.what()); |
265 | + } |
266 | + }); |
267 | } |
268 | |
269 | -void PersistDisplayConfigPolicy::save_config(mg::DisplayConfiguration const& /*base_conf*/) |
270 | +void PersistDisplayConfigPolicy::save_config(mg::DisplayConfiguration const& conf) |
271 | { |
272 | - // TODO save display config options against the h/w profile |
273 | + if (!storage) return; |
274 | + |
275 | + conf.for_each_output([this, &conf](mg::DisplayConfigurationOutput const& output) { |
276 | + if (!output.connected) return; |
277 | + |
278 | + try { |
279 | + miral::DisplayId display_id; |
280 | + // FIXME - output.edid should be std::vector<uint8_t>, not std::vector<uint8_t const> |
281 | + display_id.edid.parse_data(reinterpret_cast<std::vector<uint8_t> const&>(output.edid)); |
282 | + display_id.output_id = output.id.as_value(); |
283 | + |
284 | + miral::DisplayConfigurationOptions config; |
285 | + |
286 | + uint output_index = 0; |
287 | + conf.for_each_output([this, output, &config, &output_index](mg::DisplayConfigurationOutput const& find_output) { |
288 | + if (!config.clone_output_index.is_set() && output.top_left == find_output.top_left) { |
289 | + config.clone_output_index = output_index; |
290 | + } |
291 | + output_index++; |
292 | + }); |
293 | + |
294 | + config.size = output.extents().size; |
295 | + config.form_factor = output.form_factor; |
296 | + config.orientation = output.orientation; |
297 | + config.scale = output.scale; |
298 | + config.used = output.used; |
299 | + |
300 | + storage->save(display_id, config); |
301 | + } catch (std::runtime_error const& e) { |
302 | + printf("Failed to parse EDID - %s\n", e.what()); |
303 | + } |
304 | + }); |
305 | } |
306 | |
307 | === modified file 'src/platforms/mirserver/miral/persist_display_config.h' |
308 | --- src/platforms/mirserver/miral/persist_display_config.h 2017-01-05 14:36:10 +0000 |
309 | +++ src/platforms/mirserver/miral/persist_display_config.h 2017-03-20 14:53:59 +0000 |
310 | @@ -27,11 +27,13 @@ |
311 | // Prototyping namespace for later incorporation in MirAL |
312 | namespace miral |
313 | { |
314 | +class DisplayConfigurationStorage; |
315 | + |
316 | /// Restores the saved display configuration and saves changes to the base configuration |
317 | class PersistDisplayConfig |
318 | { |
319 | public: |
320 | - PersistDisplayConfig(); |
321 | + PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage); |
322 | ~PersistDisplayConfig(); |
323 | PersistDisplayConfig(PersistDisplayConfig const&); |
324 | auto operator=(PersistDisplayConfig const&) -> PersistDisplayConfig&; |
325 | @@ -39,7 +41,9 @@ |
326 | // TODO factor this out better |
327 | using DisplayConfigurationPolicyWrapper = |
328 | std::function<std::shared_ptr<mir::graphics::DisplayConfigurationPolicy>(const std::shared_ptr<mir::graphics::DisplayConfigurationPolicy> &wrapped)>; |
329 | - PersistDisplayConfig(DisplayConfigurationPolicyWrapper const& custom_wrapper); |
330 | + |
331 | + PersistDisplayConfig(std::shared_ptr<DisplayConfigurationStorage> const& storage, |
332 | + DisplayConfigurationPolicyWrapper const& custom_wrapper); |
333 | |
334 | void operator()(mir::Server& server); |
335 | |
336 | @@ -47,6 +51,7 @@ |
337 | struct Self; |
338 | std::shared_ptr<Self> self; |
339 | }; |
340 | + |
341 | } |
342 | |
343 | #endif //MIRAL_PERSIST_DISPLAY_CONFIG_H |
344 | |
345 | === modified file 'src/platforms/mirserver/qmirserver_p.cpp' |
346 | --- src/platforms/mirserver/qmirserver_p.cpp 2017-02-21 18:53:48 +0000 |
347 | +++ src/platforms/mirserver/qmirserver_p.cpp 2017-03-20 14:53:59 +0000 |
348 | @@ -15,6 +15,7 @@ |
349 | */ |
350 | |
351 | #include "qmirserver_p.h" |
352 | +#include "miral/display_configuration_storage.h" |
353 | |
354 | // local |
355 | #include "logging.h" |
356 | @@ -28,7 +29,6 @@ |
357 | |
358 | // miral |
359 | #include <miral/add_init_callback.h> |
360 | -#include <miral/set_command_line_hander.h> |
361 | #include <miral/set_terminator.h> |
362 | #include <miral/set_window_managment_policy.h> |
363 | |
364 | @@ -36,9 +36,19 @@ |
365 | #include <QCoreApplication> |
366 | #include <QOpenGLContext> |
367 | |
368 | +namespace |
369 | +{ |
370 | static int qtmirArgc{1}; |
371 | static const char *qtmirArgv[]{"qtmir"}; |
372 | |
373 | +struct DefaultDisplayConfigurationStorage : miral::DisplayConfigurationStorage |
374 | +{ |
375 | + void save(const miral::DisplayId&, const miral::DisplayConfigurationOptions&) override {} |
376 | + |
377 | + bool load(const miral::DisplayId&, miral::DisplayConfigurationOptions&) const override { return false; } |
378 | +}; |
379 | +} |
380 | + |
381 | void MirServerThread::run() |
382 | { |
383 | auto start_callback = [this] |
384 | @@ -125,12 +135,13 @@ |
385 | m_sessionAuthorizer, |
386 | m_openGLContextFactory, |
387 | m_mirServerHooks, |
388 | - miral::set_window_managment_policy<WindowManagementPolicy>(m_windowModelNotifier, m_windowController, |
389 | + miral::set_window_management_policy<WindowManagementPolicy>(m_windowModelNotifier, m_windowController, |
390 | m_appNotifier, screensModel), |
391 | addInitCallback, |
392 | qtmir::SetQtCompositor{screensModel}, |
393 | setTerminator, |
394 | - miral::PersistDisplayConfig{&qtmir::wrapDisplayConfigurationPolicy} |
395 | + miral::PersistDisplayConfig{std::make_shared<DefaultDisplayConfigurationStorage>(), |
396 | + &qtmir::wrapDisplayConfigurationPolicy} |
397 | }); |
398 | } |
399 |
+ conf.for_ each_output( [this, &conf]( mg::UserDisplay ConfigurationOu tput& output) { policy. apply_to( conf);
...
default_
First apply any saved configuration and then overwrite that with the default policy. Is this the right behaviour?