Mir

Merge lp://qastaging/~vanvugt/mir/resize-events into lp://qastaging/mir

resize-events
Merge into development-branch

Proposed by Daniel van Vugt on 2013-11-26

Status:

Merged

Approved by:

Alan Griffiths on 2013-11-27

Approved revision:

no longer in the source branch.

Merged at revision:

1259

Proposed branch:

lp://qastaging/~vanvugt/mir/resize-events

Merge into:

lp://qastaging/mir

Diff against target:

275 lines (+131/-8)

7 files modified

examples/eglapp.c (+19/-4)
examples/fingerpaint.c (+12/-0)
include/shared/mir_toolkit/event.h (+12/-1)
src/server/frontend/event_sender.cpp (+1/-1)
src/server/scene/surface_impl.cpp (+9/-0)
tests/unit-tests/frontend/test_event_sender.cpp (+36/-0)
tests/unit-tests/scene/test_surface_impl.cpp (+42/-2)

To merge this branch:

bzr merge lp://qastaging/~vanvugt/mir/resize-events

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2013-11-27
Alan Griffiths		2013-11-26	Approve on 2013-11-27
Alexandros Frantzis (community)			Approve on 2013-11-27
Review via email: mp+196679@code.qastaging.launchpad.net

Commit message

Add mir_event_type_resize / MirResizeEvent, allowing clients to react
immediately to surface resizing. (LP: #1227744)

Description of the change

You will see FIXME comments in the example code. I spent several days prototyping solutions to bug 1194384 to avoid the FIXMEs, before realizing it's a much bigger can of worms and beyond this scope of this work. Most importantly, the design of resize events is not affected by the presence of a fix for bug 1194384. So any sufficiently powerful toolkit (one with its own event queue and/or locking primitives) will be able to utilize MirResizeEvent immediately, without needing a fix for bug 1194384.

I admit, this was not my first choice of design. I started with "size" being a MirSurfaceAttrib with value:
int value = (width << 16) | height;
However as nice as that was reusing the existing logic and maintaining "size" as an "attribute", it was a bit kludgy and I feared reviewers would disapprove of the shifting. Not to mention the 4-gigapixel limitation ;)

I also prototyped a solution where size was a multi-value MirSurfaceAttrib. However modifying all the code to support multi-value MirSurfaceAttrib's resulted in a mess that I did not like at all.

And then there were several other possible designs I disliked so much I did not even prototype. So this is a compromise, which I think more people will approve of.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-26:

PASSED: Continuous integration, rev:1260
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/448/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/299
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/177/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/177/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2013-11-26:

157 +TEST(TestEventSender, sends_all_but_input_events)

I don't think this test does what you think. E.g. you can comment out the following lines and it still passes:

168 + EXPECT_CALL(*msg_sender, send(_))
169 + .Times(0);

It would be better written as a series of individual (or parameterized) tests

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2013-11-27:

Alan,

I don't think that's a valid argument. You can comment out asserts/expectations from any tests and they will of course pass.

The point is to verify send is never called at all for those event types. And that it is called for other event types. I just re-tested changing "Times(0)" to "Times(1)" and the test then fails. So the test is correct.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-27:

PASSED: Continuous integration, rev:1262
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/456/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/310
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/185/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/185/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2013-11-27:

> Alan,
>
> I don't think that's a valid argument. You can comment out
> asserts/expectations from any tests and they will of course pass.
>
> The point is to verify send is never called at all for those event types. And
> that it is called for other event types. I just re-tested changing "Times(0)"
> to "Times(1)" and the test then fails. So the test is correct.

Sorry, I wasn't clear.

The canonical way to write a test is /1/ setup, /2/ expectations, /3/ execution and /4/ validation.

You've mixed expectations and execution - but that doesn't change the way the fact that validation only occurs at the end. Essentially you've written:

TEST(TestEventSender, sends_all_but_input_events)
{
using namespace testing;

    // Setup
    auto msg_sender = std::make_shared<MockMsgSender>();
    mfd::EventSender event_sender(msg_sender);
    MirEvent event;
    memset(&event, 0, sizeof event);

    // Expectations
    InSequence seq;
    EXPECT_CALL(*msg_sender, send(_))
        .Times(2);

    // Execution
    event.type = mir_event_type_surface;
    event_sender.handle_event(event);
    event.type = mir_event_type_resize;
    event_sender.handle_event(event);
    event.type = mir_event_type_key;
    event_sender.handle_event(event);
    event.type = mir_event_type_motion;
    event_sender.handle_event(event);
}