Code review comment for lp:~andreas-pokorny/mir/fix-1546324

>
> 20 +MirPointerButtons to_button_state(int button)
>
> Should be named to_mir_button(int button)
> --------------------------------------------
>
> 487 + auto const up = 4, down = 5, left = 6, right = 7;
>
> Better to use Button4 for 4, Button5 for 5 as those are standard. For 6 and 7,
> my understanding is that not every left/right button may be reported as 6 & 7
> (though, I'm okay leaving this as you've implemented it). Is this also your
> understanding? That can also be said for up & down I guess.

Yeah not entirely sure - but Gtk seems to be sure about that and defines those constants
in its X11 backend. At least my mouse has about 10 buttons - but no horizontal scrolling
wheels and the additional buttons begin with 9 ...

> --------------------------------------------
>
> 114 +void mix::XInputDevice::pointer_motion(std::chrono::nanoseconds
> event_time, mir::geometry::Point const& pos, mir::geometry::Displacement
> scroll)
> 115 +{
> 116 + std::cout << "pos " << pos.x.as_float() << std::endl;
> 117 + auto const movement = pos - pointer_pos;
> 118 + pointer_pos = pos;
> 119 + sink->handle_input(
> 120 + *builder->pointer_event(
> 121 + event_time,
> 122 + mir_pointer_action_motion,
> 123 + button_state,
> 124 + scroll.dx.as_float(),
> 125 + scroll.dy.as_float(),
> 126 + movement.dx.as_float(),
> 127 + movement.dy.as_float()
> 128 + )
> 129 + );
> 130 +}
>
> button_state is not set to the proper value here. Unfortunately, this function
> is called at two different times; i) during scrolling, which doesn't require
> this setting, and ii) mouse motion, which does. So you may have to clone this
> function (pointer_scroll()?).
>
> Note also that for (ii) you can't simply call to_button_state() function as it
> has to be extracted from, not 'button' field of the Xevent struct, but from
> the 'state' field. (You can look at the existing code for that case).

Ack I should pull it from the most recent event. And remove the iostream mess..

> --------------------------------------------
>
> Superluous lines
> 34 +
> 60 +
> 111 +
>
> --------------------------------------------
>
> Hmm, you're right implementing it this way, I think.
>
> 77 + button_state |= to_button_state(button);
> 96 + button_state = button_state & ~(to_button_state(button));
>
> Though can you change the latter to
> button_state &= ~(to_button_state(button));
> for consistency?

sure..