Mir

MirMotionEvent.action needs stronger typing (to MirMotionAction etc)

Bug #1311699 reported by Daniel d'Andrada
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Mir
Fix Released
Medium
Robert Carr
Mir 0.10.0
mir (Ubuntu)
Fix Released
Medium
Unassigned

Bug Description

MirMotionEvent.action is currently an int for legacy android compatibility reasons. We need to clean this up and use MirMotionAction properly, unlike in examples...

MirMotionAction action = static_cast<MirMotionAction>(event.motion.action & ~0xff00);

OLD DESCRIPTION:
MirEvent::action is currently effectively an opaque value, as Mir headers do not specify its meaning.

So event.h must either define counterparts to the android AMOTION_EVENT_ACTION_* values below or split up the action and pointer index into separate variables.

"""
/* Bit shift for the action bits holding the pointer index as
 * defined by AMOTION_EVENT_ACTION_POINTER_INDEX_MASK.
 */
#define AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT 8

enum {
    /* Bit mask of the parts of the action code that are the action itself.
     */
    AMOTION_EVENT_ACTION_MASK = 0xff,

    /* Bits in the action code that represent a pointer index, used with
     * AMOTION_EVENT_ACTION_POINTER_DOWN and AMOTION_EVENT_ACTION_POINTER_UP. Shifting
     * down by AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT provides the actual pointer
     * index where the data for the pointer going up or down can be found.
     */
    AMOTION_EVENT_ACTION_POINTER_INDEX_MASK = 0xff00,
"""

Right now in the "Qt compositor" code I had to resort to locally defining those values, which is a hack:

""
// from android-input AMOTION_EVENT_ACTION_*, hidden inside mir bowels
// mir headers should define them
const int QtEventFeeder::MirEventActionMask = 0xff;
const int QtEventFeeder::MirEventActionPointerIndexMask = 0xff00;
const int QtEventFeeder::MirEventActionPointerIndexShift = 8;
""

See original description

Related branches

lp://staging/~mir-team/mir/introduce-mir-event-2.0
Chris Halse Rogers: Approve
Alberto Aguirre (community): Needs Information
PS Jenkins bot (community): Approve (continuous-integration)
Alexandros Frantzis (community): Approve
Andreas Pokorny (community): Approve
Cemil Azizoglu (community): Approve
Diff: 1174 lines (+963/-22)
14 files modified
client-ABI-sha1sums (+4/-1)
common-ABI-sha1sums (+4/-1)
examples/fingerpaint.c (+23/-16)
include/common/mir_toolkit/event.h (+22/-1)
include/common/mir_toolkit/input/input_event.h (+101/-0)
include/common/mir_toolkit/input/key_input_event.h (+113/-0)
include/common/mir_toolkit/input/touch_input_event.h (+141/-0)
platform-ABI-sha1sums (+4/-1)
server-ABI-sha1sums (+4/-1)
src/common/input/CMakeLists.txt (+1/-0)
src/common/input/input_event.cpp (+330/-0)
src/common/symbols.map (+1/-1)
tests/unit-tests/input/CMakeLists.txt (+1/-0)
tests/unit-tests/input/test_input_event.cpp (+214/-0)
lp://staging/ubuntu/vivid-proposed/mir
Daniel d'Andrada (dandrader)
summary: - MirEvent::action is not defined by the API
+ MirMotionEvent::action is not defined by the API
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Re: MirMotionEvent::action is not defined by the API

Just a comment in mir_toolkit/event.h

    /*
     * TODO(racarr): We would like to store this as a MirMotionAction but the android input stack
     * encodes some non enumerable values in it. It's convenient to keep things
     * this way for now until we can drop SF/Hybris support in QtUbuntu.
     */
    int action;

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Part of the problem description is incorrect. It does have a local definition: MirMotionAction

And a while ago "action" was defined as a MirMotionAction. However for compatibility reasons only racarr changed it to an int. You can still use MirMotionAction so long as you remember the compatibility hack like in examples/demo-shell/window_manager.cpp:

         // FIXME: https://bugs.launchpad.net/mir/+bug/1197108
        MirMotionAction action = static_cast<MirMotionAction>(event.motion.action & ~0xff00);

But obviously we need to retire such hacks. So it's a bug.

Changed in mir:
importance: Undecided → Medium
summary: - MirMotionEvent::action is not defined by the API
+ MirMotionEvent::action needs stronger typing (to MirMotionAction etc)
Changed in mir:
status: New → Triaged
description: updated
tags: added: clientapi input
summary: - MirMotionEvent::action needs stronger typing (to MirMotionAction etc)
+ MirMotionEvent.action needs stronger typing (to MirMotionAction etc)
Revision history for this message
Sylvain Becker (sylvain-becker) wrote :

FYI

http://developer.android.com/reference/android/view/MotionEvent.html

you should handle
ACTION_CANCEL like ACTION_UP for all FingerId
ACTION_UP, ACTION_DOWN
POINTER_ACTION_UP and POINTER_ACTION_DOWN requires the use of getActionIndex()

mir/3rd_party/android-input/android/frameworks/base/include/androidfw/Input.h :
324 inline int32_t getActionMasked() const { return mAction & AMOTION_EVENT_ACTION_MASK; }
325
326 inline int32_t getActionIndex() const {
327 return (mAction & AMOTION_EVENT_ACTION_POINTER_INDEX_MASK)
328 >> AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT;
329 }

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Yes, the Android approach obviously works. Although the Mir API is free to be different and I think we would like to avoid masking in Mirs APIs.

Revision history for this message
Robert Carr (robertcarr) wrote :

In progress as part of MirEvent 2.0 (a.k.a. MirInputEvent)

Changed in mir:
status: Triaged → In Progress
assignee: nobody → Robert Carr (robertcarr)
Daniel van Vugt (vanvugt)
Changed in mir:
milestone: none → 0.10.0
Daniel van Vugt (vanvugt)
Changed in mir:
status: In Progress → Fix Committed
Daniel van Vugt (vanvugt)
Changed in mir:
status: Fix Committed → Fix Released
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

mir (0.10.0+15.04.20150107.2-0ubuntu1) vivid; urgency=medium

Changed in mir (Ubuntu):
importance: Undecided → Medium
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.