Code review comment for lp:~mir-team/mir/introduce-mir-event-2.0

179 +MirInputEvent* mir_event_get_input_event(MirEvent* ev);

Const? Why is the MirEvent and MirInputEvent mutable?

---

229 +typedef struct _MirInputEvent MirInputEvent;
344 +typedef struct _MirKeyInputEvent MirKeyInputEvent;
460 +typedef struct _MirTouchInputEvent MirTouchInputEvent;

We've traditionally not used the typedef struct _Foo Foo idiom. Any particular reason to not make this “typedef struct Mir*InputEvent Mir*InputEvent”?

---

271 +MirInputEventTime mir_input_event_get_event_time(MirInputEvent const* ev);

Is there any particular reason to typedef int64_t to MirInputEventTime here? We're documenting it's nanoseconds-since-the-epoch, and this is not really useful if the type is opaque.

---

467 +typedef int64_t MirTouchInputEventTouchId;

We'll probably not have a full 2^63 touch ids at any point; int32_t maybe? :)

---

479 + /* This touch point was cancelled, an down should have been
480 + received but an up will never come. */
481 + mir_touch_input_event_action_cancel = 3,

Comment seems a bit off, and not just the an/down mismatch :)
Should mention that this is opt-in behaviour, “a down should have been received” is a bit wishy-washy.

“This touch point was cancelled. Any actions taken in response to this touch should be undone. No further events will be delivered for this touch point.

A cancelled event will only be sent to a client that has explicitly enabled cancellation, via mir_input_break_me_harder()”

Maybe?

---

482 + /* This touch point is unchanged but is reported as part of
483 + a full state update. */
484 + mir_touch_input_event_action_none = 4

Why do we have this? Is there an ability to request a full-state update?

---

518 +// Touch is made with a mouse through touch emulation
519 + mir_touch_input_tool_type_mouse = 3,

Are we going to do this?

---

549 +MirTouchInputEventTouchAction mir_touch_input_event_get_touch_action(MirTouchInputEvent const* event, size_t touch_index);

Hm. Would it be nice for clients if instead of get_touch_id(..., index) and get_*(..., index) we just had.
get_*(MirTouchInputEvent const* event, MirTouchInputEventTouchId id)?

That would seem be a more natural API to me, but we'd need toolkit input as to whether or not that's better.

---

655 + default:
656 + return MirInputEventType();

I'm a fan of abort() when faced with egregious client API abuse. Likewise for the DeviceId just afterwards.

---

761 +MirTouchInputEventTouchAction mir_touch_input_event_get_touch_action(MirTouchInputEvent const* event, size_t touch_index)

Oooh, ugh. Maybe we need an actual MirTouchInputEvent to pass in here, so that we can maintain the state that lets us implement this properly?