Mir

MirMotionEvent.action needs stronger typing (to MirMotionAction etc)

Bug #1311699 reported by Daniel d'Andrada on 2014-04-23
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Mir
Fix Released
Medium
Robert Carr
mir (Ubuntu)
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;
""

Related branches

summary: - MirEvent::action is not defined by the API
+ 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;

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)
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 }

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.

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)
Changed in mir:
milestone: none → 0.10.0
Changed in mir:
status: In Progress → Fix Committed
Changed in mir:
status: Fix Committed → Fix Released
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  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers