Mir

Merge lp://qastaging/~andreas-pokorny/mir/fix-1531517 into lp://qastaging/mir

  1. fix-1531517
  2. Merge into development-branch
Proposed by Andreas Pokorny
Status: Merged
Approved by: Alexandros Frantzis
Approved revision: no longer in the source branch.
Merged at revision: 3243
Proposed branch: lp://qastaging/~andreas-pokorny/mir/fix-1531517
Merge into: lp://qastaging/mir
Diff against target: 362 lines (+241/-44)
2 files modified
src/server/input/android/input_sender.cpp (+69/-28)
tests/unit-tests/input/android/test_android_input_sender.cpp (+172/-16)
To merge this branch: bzr merge lp://qastaging/~andreas-pokorny/mir/fix-1531517
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Mir CI Bot continuous-integration Approve
Brandon Schaefer (community) Approve
Kevin DuBois (community) Approve
Alexandros Frantzis (community) Approve
Review via email: mp+282439@code.qastaging.launchpad.net

Commit message

Never encode more than one action per event

Android InputTransport has only one action parameter per event. Within that the contact that caused the action is also encoded. So until we replace or extend the input transport protocol, we have to split up MirEvents, to not lose touch up/down changes.

Description of the change

This change splits a MirEvent into several input transport messages when more than one contact was changed.

Alternatively to this approach we could 'just' change the InputTransport protocol... But I would wait with that when bschaefer gets around to add the cookie.. Additionally Daniel D'Andrada indicated that qt might not like seeing multiple state changes, at least there was some uncertainty.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:3241
http://jenkins.qa.ubuntu.com/job/mir-ci/6001/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5514/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4421
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5470/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/325
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/325/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/325
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/325/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5467/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

I find it hard to follow the logic in the main loop, although this could just be a result of my unfamiliarity with the domain. Perhaps breaking up the code into multiple distinct passes over the touch points, each pass performing a simple task and setting up state for the next pass, would make the code clearer.

I don't see anything wrong with the code, but I am not confident enough to "approve" either, so "abstain" :)

review: Abstain
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3241
https://mir-jenkins.ubuntu.com/job/mir-ci/37/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/37/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/37/rebuild

review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> I find it hard to follow the logic in the main loop, although this could just
> be a result of my unfamiliarity with the domain. Perhaps breaking up the code
> into multiple distinct passes over the touch points, each pass performing a
> simple task and setting up state for the next pass, would make the code
> clearer.
>
> I don't see anything wrong with the code, but I am not confident enough to
> "approve" either, so "abstain" :)

fair enough.
Hm I cannot see a simple solution .. I have three types up:
* up contacts have to be sent as change events until they have been processed then they have to be ignored.
* change contact are added always
* down contacts have to be omitted until they get processed.. then they have to be sent as change contacts..

ah.. yeah i guess that could turn into simpler code. Will give it a try.

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3242
https://mir-jenkins.ubuntu.com/job/mir-ci/38/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/38/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/38/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:3242
http://jenkins.qa.ubuntu.com/job/mir-ci/6002/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5515
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4422
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5471
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/267
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/326
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/326/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/326
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/326/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5468
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5468/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7942
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26607
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/263
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/263/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/121
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26611

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

review: Approve (continuous-integration)
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3243
https://mir-jenkins.ubuntu.com/job/mir-ci/50/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/50/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/50/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

OK. I am still missing some domain knowledge about why things are done this way, but the code is clearer. Thanks.

review: Approve
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3244
https://mir-jenkins.ubuntu.com/job/mir-ci/51/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/51/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/51/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:3243
http://jenkins.qa.ubuntu.com/job/mir-ci/6015/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5531
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4438
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5487
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/276
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/339
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/339/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/339
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/339/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5484
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5484/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7955
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26643
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/272
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/272/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/130
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26646

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

review: Approve (continuous-integration)
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

LGTM. For my changes ill just be hard coding an uint8_t[20] for the mac, sooo this wont cause me any issues. Which will just give me a operator= copy/move for the mac anyway soo no fancy changes needed on my end here.

Sooo really depends on whos branch lands first :), should be a simple conflict (if any)

One things:
+#include <unordered_set>
+ std::unordered_set<size_t> sent_indices;

Doesnt seem to be used?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:3244
http://jenkins.qa.ubuntu.com/job/mir-ci/6016/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5532
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4439
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5488
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/277
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/340
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/340/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/340
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/340/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5485
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5485/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7956
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26653
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/273
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/273/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/131
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26666

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

review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

lgtm

review: Approve
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> LGTM. For my changes ill just be hard coding an uint8_t[20] for the mac, sooo
> this wont cause me any issues. Which will just give me a operator= copy/move
> for the mac anyway soo no fancy changes needed on my end here.
>
> Sooo really depends on whos branch lands first :), should be a simple conflict
> (if any)
>
> One things:
> +#include <unordered_set>
> + std::unordered_set<size_t> sent_indices;
>
> Doesnt seem to be used?

yes removed that

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

LGTM

review: Approve
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3245
https://mir-jenkins.ubuntu.com/job/mir-ci/62/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/62/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/62/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:3245
http://jenkins.qa.ubuntu.com/job/mir-ci/6026/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5542
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4449
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5498
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/282
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/350
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/350/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/350
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/350/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5495
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5495/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7965
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26692
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/278
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/278/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/136
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26693

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

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