Code review comment for lp:~samuel-buffet/entertainer/new-scrollmenu

Hi Matt,

> Samuel,
>
> It seems like you have been very busy making modifications to this branch. Now
> that thing have stabilized a bit, let me throw another few comments at you.
> After this round of comments, I really only have to review the MotionBuffer
> class (I don't the time to do that tonight). Again, thanks for the good work.
>
> main.py:
> * _move_menu is now a dead unused method that can be removed.

Indeed, I've removed it.

> scroll_menu.py:
> * _on_release_event has more idx variables
> * line 301 could use some spacing around the less than character for clarity

Fixed.

> * _on_motion_event: why is it showing the cursor? That is done by the
> UserInterface.
> * _on_scroll_event: why is it showing the cursor? That is done by the
> UserInterface.

Yeah that's done by the UserInterface but if for instance you place your cursor on the menu and scroll it (no motion only scroll) then after 4 seconds the cursor will be hidden without the cursor.show in _on_scroll_event because there's no motion event grabbed by the stage. Just try to comment that line out, it looks strange from the user experience point of view (I think).

Same kind of thing with _on_motion_event if you want to click on the menu and, keeping the pressure on the button you play with it up and down. It will hide the cursor after 4 seconds but here the answer to "why?" is more subtle. Because here we have motion-events but they are grabbed only by the menu because of the clutter.grab_pointer(self) placed on _on_button_press_event. This redirection of all events to the menu is only released in _on_button_release_event by clutter.ungrab_pointer().
So until the button is released the stage is blind. It see no event at all so the timeout is not reset and hides the cursor after 4 seconds.

Now, after having explain that, maybe I should explain why I've done this grab/ungrab stuff. Simply it's because without clutter.grab_pointer(self) event are seen by the menu only if the cursor is over the menu. And when we move it to give it an impulse to scroll we can easily finish the cursor's motion (= release the mouse's button) anywhere with no guaranty to be over the menu. So in this case, the _on_button_release_event event will never be seen by the menu and it will be in a unstable weird state.

I've tried another way to detect the button's release using the ClutterModifierType field of a motion event which is supposed to provide the state of button (if I understand correctly). But unfortunately, it looks like it doesn't work (maybe after more tests I'll file a bug upstream).

Cheers,
Samuel-