Re: [Gnash-dev] [PATCH] Make mouse events more responsive

[Bastiaan Jacques]

On Tuesday 25 July 2006 14:54, Ivor Blockley wrote:
> This patch resolves the problem by moving some of the mouse-event
> generation/handling code outside the "movie" loop. It has been tested
> as working under GTK and SDL and should apply cleanly against CVS.

I think the patch looks good in general. Some minor comments follow 
below.

> Caveats:
> * Is this solution thread-safe?

The movie loop and the GUI events currently execute in the same thread, 
so it doesn't matter much at this point. However, if we were 
multithreaded, I think this code would be thread-safe.

> Index: gui/gtk.cpp
> ===================================================================
> RCS file: /sources/gnash/gnash/gui/gtk.cpp,v
> retrieving revision 1.13
> diff -u -p -r1.13 gtk.cpp
> --- gui/gtk.cpp       15 Jul 2006 16:02:23 -0000      1.13
> +++ gui/gtk.cpp       25 Jul 2006 12:25:57 -0000
> @@ -532,11 +532,11 @@ GtkGui::button_press_event(GtkWidget *co
>      Gui *obj = static_cast<Gui *>(data);
>      int      mask = 1 << (event->button - 1);
>      int buttons = obj->getMouseButtons();
> -    obj->setMouseButtons(buttons |= mask);
> +    buttons |= mask;
> +    obj->setMouseButtons(buttons);

Why call setMouseButtons() when you call notify_mouse_state below?

>      float scale = obj->getScale();
> -    obj->setMouseX(int(event->x / scale));
> -    obj->setMouseY(int(event->y / scale));
> -
> +    obj->notify_mouse_state(int(event->x / scale),
> +                         int(event->y / scale), buttons);
>      return true;
>  }
> @@ -549,12 +549,11 @@ GtkGui::button_release_event(GtkWidget *
>      Gui *obj = static_cast<Gui *>(data);
>      int      mask = 1 << (event->button - 1);
>      int buttons = obj->getMouseButtons();
> -
> -    obj->setMouseButtons(buttons &= ~mask);
> +    buttons &= ~mask;
> +    obj->setMouseButtons(buttons);

Same thing here.

>      float scale = obj->getScale();
> -    obj->setMouseX(int(event->x / scale));
> -    obj->setMouseY(int(event->y / scale));
> -
> +    obj->notify_mouse_state(int(event->x / scale),
> +                         int(event->y / scale), buttons);
>      return true;
>  }
>
> @@ -567,9 +566,9 @@ GtkGui::motion_notify_event(GtkWidget *c
>      Gui *obj = static_cast<Gui *>(data);
>
>      float scale = obj->getScale();
> -    obj->setMouseX(int(event->x / scale));
> -    obj->setMouseY(int(event->y / scale));
> -
> +    int buttons = obj->getMouseButtons();
> +    obj->notify_mouse_state(int(event->x / scale),
> +                         int(event->y / scale), buttons);
>      return true;
>  }
>
> Index: gui/gui.cpp
> ===================================================================
> RCS file: /sources/gnash/gnash/gui/gui.cpp,v
> retrieving revision 1.10
> diff -u -p -r1.10 gui.cpp
> --- gui/gui.cpp       15 Jul 2006 16:02:23 -0000      1.10
> +++ gui/gui.cpp       25 Jul 2006 12:25:57 -0000
> @@ -57,8 +57,6 @@ Gui::Gui() :
>      _xid(0),
>      _width(0),
>      _height(0),
> -    _mouse_x(0),
> -    _mouse_y(0),

Okay. Why not also remove _mouse_buttons?

>      _scale(1.0f),
>      _mouse_buttons(0),
>      _depth(16)
> @@ -74,8 +72,6 @@ Gui::Gui(unsigned long xid, float scale,
>      _xid(xid),
>      _width(0),
>      _height(0),
> -    _mouse_x(0),
> -    _mouse_y(0),
>      _scale(scale),
>      _mouse_buttons(0),
>      _depth(depth)
> @@ -191,6 +187,12 @@ Gui::menu_jump_backward()
>      m->goto_frame(m->get_current_frame()-10);
>  }
>
> +void
> +Gui::notify_mouse_state(int x, int y, int buttons) {
> +//   log_msg("Mouse(x,y): %d,%d", x, y);
> +    get_current_root()->notify_mouse_state(x, y, buttons);
> +}
> +
>  bool
>  Gui::advance_movie(void *data)
>  {
> @@ -199,9 +201,6 @@ Gui::advance_movie(void *data)
>      Gui *gui = reinterpret_cast<Gui*> (data);
>      gnash::movie_interface* m = gnash::get_current_root();
>
> -//   log_msg("Mouse(x,y): %d,%d", gui->getMouseX(), gui->getMouseY());
> -    m->notify_mouse_state(gui->getMouseX(), gui->getMouseY(),
> gui->getMouseButtons()); -
>      m->advance(1.0);
>      m->display();
>
> Index: gui/gui.h
> ===================================================================
> RCS file: /sources/gnash/gnash/gui/gui.h,v
> retrieving revision 1.7
> diff -u -p -r1.7 gui.h
> --- gui/gui.h 15 Jul 2006 16:02:23 -0000      1.7
> +++ gui/gui.h 25 Jul 2006 12:25:57 -0000
> @@ -70,11 +70,7 @@ public:
>      virtual bool setupEvents() = 0;
>      virtual void renderBuffer() = 0;
>
> -    void setMouseX(int x)           { _mouse_x = x; }
> -    void setMouseY(int y)           { _mouse_y= y; }
>      void setMouseButtons(int mask)  { _mouse_buttons = mask; }
> -    int getMouseX()                 { return _mouse_x; }
> -    int getMouseY()                 { return _mouse_y; }
>      int getMouseButtons()           { return _mouse_buttons; }

Same thing here.

>      float getScale()                { return _scale; }
>      bool loops()                    { return _loop; }
> @@ -92,6 +88,7 @@ public:
>      static void menu_step_backward();
>      static void menu_jump_forward();
>      static void menu_jump_backward();
> +    static void notify_mouse_state(int x, int y, int buttons);
>      static bool advance_movie(void *data);
>      static void resize_view(int width, int height);

Could you please address these comments and update your patch?

Bastiaan