Mir

Merge lp://qastaging/~mir-team/mir/cursor-spike-lightning-round-fix-input-area-contains into lp://qastaging/mir

cursor-spike-lightning-round-fix-input-area-contains
Merge into development-branch

Proposed by Robert Carr on 2014-05-19

Status:

Merged

Approved by:

Alan Griffiths on 2014-06-04

Approved revision:

no longer in the source branch.

Merged at revision:

1678

Proposed branch:

lp://qastaging/~mir-team/mir/cursor-spike-lightning-round-fix-input-area-contains

Merge into:

lp://qastaging/mir

Diff against target:

87 lines (+16/-7)

3 files modified

src/server/input/android/android_input_window_handle.cpp (+1/-0)
src/server/scene/basic_surface.cpp (+11/-4)
tests/unit-tests/scene/test_basic_surface.cpp (+4/-3)

To merge this branch:

bzr merge lp://qastaging/~mir-team/mir/cursor-spike-lightning-round-fix-input-area-contains

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-05-19	Approve on 2014-06-04
Alan Griffiths			Approve on 2014-06-03
Chris Halse Rogers			Approve on 2014-05-28
Kevin DuBois (community)		2014-05-19	Needs Information on 2014-05-20
Review via email: mp+220111@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2014-05-19.

Commit message

Input area contains was broken when surfaces moved due to the initial input_rectangles being copied from global coordinates. Fix input_area_contains to work properly in global coordinates even when input_rectangles is updated.

Description of the change

Revision history for this message

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

PASSED: Continuous integration, rev:1637
http://jenkins.qa.ubuntu.com/job/mir-ci/1663/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/242
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/243
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/1
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/1/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-armhf-ci/1
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-armhf-ci/1/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/1663/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-19:

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-05-20:

50,80: (nonblocking) I wouldn't mind Points having a operator+ and operator-

nits:
12, 41: whitespace

50: why not just geom::Point point{}

mostly a needs info around this question though:
if the input stack is talking in different coordinates from the mir stack, should the transform be done in the input stack? (eg, have touchableRegionContainsPoint do the transform by Surface::top_left) Seems to have the advantage that we don't have BasicSurface with two coord systems.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-27:

> 50,80: (nonblocking) I wouldn't mind Points having a operator+ and operator-

Something like this?

inline Point operator+(Point const& lhs, Displacement const& rhs)
...

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-28:

> 50,80: (nonblocking) I wouldn't mind Points having a operator+ and operator-

#include "mir/geometry/displacement.h"
...

static const geom::Point origin{};
auto surface_displacement = surface_rect.top_left - origin;

    // TODO: Perhaps creates some issues with transformation.
    auto local_point = point - surface_displacement;
...
            auto test_pt = rect.top_left + geom::Displacement{{x}, {y}};

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-28:

> if the input stack is talking in different coordinates from the mir stack,
> should the transform be done in the input stack? (eg, have
> touchableRegionContainsPoint do the transform by Surface::top_left) Seems to
> have the advantage that we don't have BasicSurface with two coord systems.

The transformation of co-ordinate system is part of a larger issue: what parts of the data model are in the scene and which are in compositing and input.

If we want to support clicking on surfaces projected onto a "cube" (or walls, or a spread or...) then either input has to understand these transformations or the transformations should be modeled in the scene. I think that argues for transformations being part of the model.

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-05-28:

I concur; input needs access to a scenegraph-like interface to do transformations (and targetting, and such).

However, that interface is not going to happen in the immediate future, and this fixes something annoying now. I don't mind committing this knowing that it's going to be thrown away when we do the right thing (after deciding what the right thing actually is ☺).

Under the expectation that this code will die in the forseeable future, I don't care too much about the Displacement cleanup, but do it if you need to push again.

review: Approve

Revision history for this message

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

Nits

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-06-03:

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-03:

FAILED: Continuous integration, rev:1638
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1801/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/436
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/437/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/435/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/320/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/319/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1216/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-03:

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-04:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-06-04:

Failure was bug 1325602 - and the fix has landed. Reapproving.

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-06-04:

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:

Emanuele Antonio Faraone

Gerry Boland

Mir development team