Mir

Merge lp://qastaging/~vanvugt/mir/eliminate-configure-output into lp://qastaging/mir

eliminate-configure-output
Merge into development-branch

Proposed by Daniel van Vugt on 2014-02-13

Status:	Merged
Approved by:	Daniel van Vugt on 2014-02-28
Approved revision:	no longer in the source branch.
Merged at revision:	1438
Proposed branch:	lp://qastaging/~vanvugt/mir/eliminate-configure-output
Merge into:	lp://qastaging/mir
Diff against target:	1421 lines (+447/-388) 32 files modified examples/demo-shell/window_manager.cpp (+10/-24) examples/pixel_format_selector.cpp (+2/-7) examples/server_configuration.cpp (+15/-26) include/platform/mir/graphics/display_configuration.h (+27/-7) include/test/mir_test_doubles/null_display_configuration.h (+1/-1) include/test/mir_test_doubles/stub_display_configuration.h (+6/-1) src/platform/graphics/android/android_display.cpp (+8/-0) src/platform/graphics/android/android_display_configuration.cpp (+4/-5) src/platform/graphics/android/android_display_configuration.h (+1/-4) src/platform/graphics/display_configuration.cpp (+50/-0) src/platform/graphics/mesa/display.cpp (+6/-0) src/platform/graphics/mesa/real_kms_display_configuration.cpp (+6/-34) src/platform/graphics/mesa/real_kms_display_configuration.h (+1/-4) src/server/frontend/session_mediator.cpp (+22/-16) src/server/graphics/default_display_configuration_policy.cpp (+9/-10) src/server/graphics/nested/nested_display.cpp (+6/-0) src/server/graphics/nested/nested_display_configuration.cpp (+59/-39) src/server/graphics/nested/nested_display_configuration.h (+1/-5) src/server/graphics/offscreen/display.cpp (+6/-0) src/server/graphics/offscreen/display_configuration.cpp (+5/-3) src/server/graphics/offscreen/display_configuration.h (+1/-5) src/server/scene/mediating_display_changer.cpp (+2/-7) tests/mir_test/display_config_matchers.cpp (+6/-3) tests/unit-tests/frontend/test_session_mediator.cpp (+1/-9) tests/unit-tests/graphics/android/test_android_fb.cpp (+21/-29) tests/unit-tests/graphics/mesa/test_cursor.cpp (+6/-3) tests/unit-tests/graphics/mesa/test_display_multi_monitor.cpp (+27/-37) tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp (+1/-3) tests/unit-tests/graphics/nested/test_nested_display_configuration.cpp (+20/-68) tests/unit-tests/graphics/test_default_display_configuration_policy.cpp (+42/-38) tests/unit-tests/graphics/test_display.cpp (+34/-0) tests/unit-tests/graphics/test_display_configuration.cpp (+41/-0)
To merge this branch:	bzr merge lp://qastaging/~vanvugt/mir/eliminate-configure-output
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-02-13	Approve on 2014-02-28
Alberto Aguirre (community)		2014-02-13	Approve on 2014-02-28
Daniel van Vugt			Abstain on 2014-02-17
Alan Griffiths			Abstain on 2014-02-14
Andreas Pokorny (community)		2014-02-13	Needs Information on 2014-02-14
Alexandros Frantzis		2014-02-13	Pending
Review via email: mp+206127@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2014-02-12.

Commit message

Eliminate "configure_output()" and its long parameter list that has caused us
so much grief recently.

Replaced by a mutable version of for_each_output(), where you can observe
and modify fields of each DisplayConfigurationOutput directly.
This not only eliminates the coupling problem, but simplifies typical
display config changes that only touch a small number of fields (as
demonstrated in examples/*).

Validation checking that used to exist in some configure_output()
implementations has been reimplemented as common to all platforms now, and
moved to the Display::configure() stage.

Description of the change

In case you've forgotten, here are where configure_output() has caused grief
recently:
https://code.launchpad.net/~andreas-pokorny/mir/add-pixel-format-to-display-configuration/+merge/200294
https://code.launchpad.net/~vanvugt/mir/rotate-output/+merge/201887

You could argue such changes aren't that common. But I think the maintenance
benefits and reduced likelihood for mistakes to arise from parameter list
confusion (some of which I have fixed here) is worth it.

The required server ABI bump is here:
https://code.launchpad.net/~vanvugt/mir/server-abi-16/+merge/206842

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-12: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/825/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-02-13: Posted in a previous version of this proposal

jenkins failure looks unrelated, retriggering:
bzr: ERROR: Invalid http response for https://xmlrpc.launchpad.net/bazaar/: Unable to handle http code 502: Bad Gateway

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-02-13: Posted in a previous version of this proposal

somewhat on the fence about this one, it feels better to not have to program a long parameter list to change one item, but it also introduces the problem of how to check the request.

If we do go with this as-is, I think we're delaying when we throw and putting the burden of checking validity on the mg::Display. The execute-around pattern of for_each_output() would let us easy check valid() and throw right after the user mis-programs the struct. mg::DisplayConfiguration::valid could then be protected: or a free standing function

review: Abstain

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-13: Posted in a previous version of this proposal

Interesting idea to call valid() as part of for_each_output(). Although consistency checking could theoretically become based on the whole config, and therefore not enough information might be available if you're only looking at an individual output. This could happen, for example, if you're creating some complex config that requires multiple iterations to build. You don't want an exception happening in the middle when you're only half finished.

It's important to remember the current design is misleading. configure_output() doesn't ever actually configure anything. It just assigns struct fields to a configuration object that is entirely private and benign (as it's a unique_ptr). No actual configuration happens until Display::configure(). And I think it's better practice in general to only verify and throw exceptions when the caller has finally requested that an action be taken.

The final reason for moving validation into Display::configure() is because that's the action function which can fail and throw exceptions for other hardware/platform reasons. As all the other config failure exceptions already exist in our configure() implementations, I think it's cleanest to keep that as the only place from which you can expect to get a config failure exception.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-13: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/829/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-02-13: Posted in a previous version of this proposal

The idea behind configure_output() was to allow the user to only change particular values of the DisplayConfigurationOutput struct (e.g., it's not valid to change the id). That is, an interface that makes it harder for the user to mess up.

If we want to go this path, I would prefer it if we gave the user a copy of the struct, and only copied back to the real/internal DisplayConfigurationOutput struct only the fields that are valid to change, throwing a logic_error if the user tries to change one of the "immutable" values.

So, "Needs fixing", because we can't let the users just change the DisplayConfigurationOutput any way they want.

Also:

576 + std::vector<MirPixelFormat> formats;
577 + formats.reserve(mir_output.num_output_formats);
578 + for (auto p = mir_output.output_formats;
579 + p != mir_output.output_formats+mir_output.num_output_formats;
580 + ++p)
581 + {
582 + formats.push_back(*p);
583 + }

std::vector<MirPixelFormat> formats(mir_output.output_formats,mir_output.output_formats+mir_output.num_output_formats

// as some test cases change immutable fields like output_id
...
622 + mir_output.output_id = output.id.as_value();

As stated above, I think we should throw in such cases.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-13: Posted in a previous version of this proposal

I have been undecided about how or if to solve that problem at all. I don't think it's important enough to justify the overhead of copying structures, but if it's a blocker will do so... Copying structures isn't really much worse than passing around long parameter lists.

An alternative solution is to separate the mutable and immutable fields into different structures:
   struct Output
   {
      // immutable fields
      ...
      OutputConfiguration config;
   }

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-13:

OK, I've made immutable fields of the output structure read-only. So the compiler will prevent you from changing them. No need for run-time checking.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-13:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/835/rebuild

review: Approve (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2014-02-14:

I do not object to the current status of the MP.

But: Have you considered having two structures, splitting the current one into two?

One passed to the policy as a constant/value - i.e. called OutputHardwareInformation - and another one passed in by referencs - maybe called UserConfigration or just OutputConfigurationSomthing?

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-14:

Yes, I've considered that a few times, but don't think it's any better. We already have too many structures representing the same thing:
    MirDisplayOutput (client API)
    protobuf::DisplayOutput (protobuf class autogenerated)
    DisplayConfigurationOutput (server API)
    UserDisplayConfigurationOutput (this proposal)

I was reluctant to add the last one yesterday, except to resolve the issue of making some fields read only.

Although we do have lots of structs representing the same thing at different layers, I think that's preferable to further confusing the reader by making one of the layers two structs :)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2014-02-14:

Yet another suggestion:
    MirDisplayOutput (client API)
    protobuf::DisplayOutput (protobuf class autogenerated)
    struct DisplayConfigurationOutput
    {
       Output fixed;
       Configuration current;
    };

Then pass fixed and a copy of current by ref to the callback. From the users point of few it would like dealing with the same thing. But yet larger change to the code base.

Other opinions?

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-02-14:

170 +struct UserDisplayConfigurationOutput
171 +{
172 + DisplayConfigurationOutputId const& id;
173 + DisplayConfigurationCardId const& card_id;
174 + DisplayConfigurationOutputType const& type;
175 + std::vector<MirPixelFormat> const& pixel_formats;
176 + std::vector<DisplayConfigurationMode> const& modes;
177 + size_t const& preferred_mode_index;
178 + geometry::Size const& physical_size_mm;
179 + bool const& connected;
180 + bool& used;
181 + geometry::Point& top_left;
182 + size_t& current_mode_index;
183 + MirPixelFormat& current_format;
184 + MirPowerMode& power_mode;
185 + MirOrientation& orientation;
186 +
187 + UserDisplayConfigurationOutput(DisplayConfigurationOutput& master);
188 };

I'm not saying this is worse than the status quo (I'm not keen on that either), but structures like this make me feel we're missing the right abstractions.

review: Abstain

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-17:

Andreas:
Answered by my previous comment :). Also, shell code would have to change syntax to something like:
output.current.current_mode_index = output.fixed.preferred_mode_index;
That's ugly having to know and explicitly mention "current" or "fixed" in front of everything.

Alan:
I agree it feels like we're missing the right abstractions. But I'm only trying to fix one problem at a time. I suspect part of the greater solution is to reduce the number of structs we have representing the same thing. Trying to unify client and server structs might help, albeit difficult.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-17:

Strangely this branch seems to fix bug 1277343. Although I don't fully understand why.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-17:

The session_mediator.cpp logic is mixing up output ID with protobuf vector indices.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) on 2014-02-17:

review: Abstain

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/861/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/930/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-26:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/939/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-02-28:

Looks good to me. The config api had a bit too many arguments.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-28:

After two weeks, I'm happy to do with a single Approve :)

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-02-28:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Daniel van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team