[MPlayer-dev-eng] [PATCH] double click to switch to full screen in GUI

[Reimar Döffinger]

Hello,
On Mon, Oct 02, 2006 at 12:23:22AM +0200, Diego Biurrun wrote:
> On Fri, Sep 29, 2006 at 06:16:56PM +0200, Reimar Döffinger wrote:
> > On Thu, Sep 14, 2006 at 11:11:07PM +0200, laurent wozniak wrote:
> > >   gtkInit();
> > > +// --- initialize some input constants 
> > > + g_object_get ( G_OBJECT ( gtk_settings_get_default() ), 
> > > +    "gtk-double-click-time", &double_click_time, NULL );
> > > + mp_input_set_double_click_time(double_click_time);
> > 
> > I don't care much, but it should be tested if this works with GTK1
> 
> Apparently it does not, I get linking errors 'undefined reference to
> gtk_settings_get_default'.  Some sort of solution is needed.  This part
> of the code can be dropped, protected by #ifdef or support for GTK 1.x
> dropped.

ifdef is fine with me, though I also don't find it particularly
important and actually has the disadvantage of the Gui and non-Gui
versions behaving differently in yet another case.


> > > @@ -305,6 +307,7 @@
> > >  
> > >  static mp_cmd_bind_t def_cmd_binds[] = {
> > >  
> > > +  { {  MOUSE_BTN0_DBL, 0 }, "vo_fullscreen" },
> > 
> > I'd prefer it not to be bound by default.
> 
> I strongly disagree.  Pretty much every player goes into fullscreen on
> doubleclick.  I'm not aware of any that doesn't...

Well, I guess I did not exactly express what I really wanted. I first
and foremost wanted this double-click patch do really only implement the
double-click detection.
I would not want this in a first patch, because I'd also prefer not to
have the next-discussed part - but then the single-click command should
be bound as well.


> > > +  // When binding a command to a mouse button double click,
> > > +  // if the corresponding single click is not bound,
> > > +  // bind it to the command which does nothing:
> > > +  // Avoid logging command not found on the first click of the double click.
> > > +  for(j = 0 ; (key = keys[j]) != 0 ; j++) {
> > > +    if (MOUSE_BTN0_DBL <= key && key <= MOUSE_BTN_LAST_DBL) {
> > > +      key = MOUSE_BTN0 + key - MOUSE_BTN0_DBL;
> > > +      other_cmd = mp_input_find_bind_for_key(1, &key);
> > > +      if (other_cmd == NULL) {
> > > +        int other_cmd_keys[2];      	
> > > +        other_cmd_keys[0] = key;
> > > +        other_cmd_keys[1] = 0;      	
> > > +        mp_input_bind_keys(other_cmd_keys, MP_CMD_TXT_NO_COMMAND);
> > > +      }
> > > +    }
> > > +  }
> > > +
> > 
> > I find the (IMHO doubtful since possibly confusing) convenience of not having
> > to add a no_command keybind not worth even that few lines of code.
> 
> I'm a bit sceptical myself here, but the convenience is real ..

It only avoids a somewhat annoying message... If this was a simple
3-liner I'd be far more okay. But apart from that this is an only
slightly related part that can be discussed and applied without major
drawbacks at a later time, instead of delaying the real feature this
thread started about.


> Index: Gui/wm/ws.h
> ===================================================================
> --- Gui/wm/ws.h	(revision 19814)
> +++ Gui/wm/ws.h	(working copy)
> @@ -29,11 +29,15 @@
>  #define  wsPRMouseButton 3
>  #define  wsP4MouseButton 4
>  #define  wsP5MouseButton 5
> -#define  wsRLMouseButton 1 + 128
> -#define  wsRMMouseButton 2 + 128
> -#define  wsRRMouseButton 3 + 128
> -#define  wsR4MouseButton 4 + 128
> -#define  wsR5MouseButton 5 + 128
> +#define  wsP0MouseButton wsPLMouseButton
> +#define  wsPLastMouseButton 10
> +#define  wsRLMouseButton (1 + 128)
> +#define  wsRMMouseButton (2 + 128)
> +#define  wsRRMouseButton (3 + 128)
> +#define  wsR4MouseButton (4 + 128)
> +#define  wsR5MouseButton (5 + 128)

These 1 + 128 => (1 + 128) changes although correct are independent and
IMHO can and should be applied first and separately.

> @@ -877,9 +884,14 @@
>    
>  
>  static char*
> -mp_input_find_bind_for_key(mp_cmd_bind_t* binds, int n,int* keys) {
> +mp_input_find_bind_for_key(int n, int* keys) {
> +  mp_cmd_bind_t* binds;
>    int j;
>  
> +  binds = cmd_binds;
> +  if (binds == NULL)
> +    return NULL;
> +
>    for(j = 0; binds[j].cmd != NULL; j++) {
>      if(n > 0) {
>        int found = 1,s;
> @@ -903,24 +915,27 @@
>  
>  static mp_cmd_t*
>  mp_input_get_cmd_from_keys(int n,int* keys, int paused) {
> -  char* cmd = NULL;
> +  char* cmd;
>    mp_cmd_t* ret;
>  
> -  if(cmd_binds)
> -    cmd = mp_input_find_bind_for_key(cmd_binds,n,keys);
> -  if(cmd == NULL)
> -    cmd = mp_input_find_bind_for_key(def_cmd_binds,n,keys);
> +  for(;;) {
> +    cmd = mp_input_find_bind_for_key(n,keys);
> +    // Found a command.
> +    if(cmd != NULL) {
> +      if (strcmp(cmd, MP_CMD_TXT_NO_COMMAND) == 0)
> +        return NULL;
> +      break;
> +    }
>  
> -  if(cmd == NULL) {
> +    // Ignore unbound key.
>      mp_msg(MSGT_INPUT,MSGL_WARN,MSGTR_NoBindFound,mp_input_get_key_name(keys[0]));
> -    if(n > 1) {
> -      int s;
> -      for(s=1; s < n; s++)
> -	mp_msg(MSGT_INPUT,MSGL_WARN,"-%s",mp_input_get_key_name(keys[s]));
> -    }
>      mp_msg(MSGT_INPUT,MSGL_WARN,"                         \n");
> -    return NULL;
> +    keys++;
> +    n--;
> +    if(n<=0)
> +      return NULL;
>    }
> +

As discussed before, this part is to fix a bug that has nothing to do
with double click (at least that's how I understood it). This IMO definitly
needs to be a separate patch.

> +// This puts key layer filter events.
> +// It can translate the event into another one.

Should be made doxygen-compatible.

>  				break;
>  			int x = GET_WHEEL_DELTA_WPARAM(wParam);
>  			if (x > 0)
> -				mplayer_put_key(MOUSE_BTN3);
> +				mplayer_put_key(MOUSE_WHL_UP);
>  			else
> -				mplayer_put_key(MOUSE_BTN4);
> +				mplayer_put_key(MOUSE_WHL_DOWN);
>  			break;

The mouse wheel changes should be discussed separately as well, esp.
since this new systems is also broken, even if less so than the current
one.

> Index: libvo/x11_common.c
> ===================================================================
> --- libvo/x11_common.c	(revision 19814)
> +++ libvo/x11_common.c	(working copy)
> @@ -1098,17 +1098,8 @@
>                      mouse_waiting_hide = 1;
>                      mouse_timer = GetTimerMS();
>                  }
> -                // Ignore mouse whell press event
> -                if (Event.xbutton.button > 3)
> -                {
> -                    mplayer_put_key(MOUSE_BTN0 + Event.xbutton.button - 1);
> -                    break;
> -                }
>  #ifdef HAVE_NEW_GUI
> -                // Ignor mouse button 1 - 3 under gui 
> -                if (use_gui && (Event.xbutton.button >= 1)
> -                    && (Event.xbutton.button <= 3))
> -                    break;
> +                if ( use_gui ) { break; }
>  #endif
>                  mplayer_put_key((MOUSE_BTN0 + Event.xbutton.button -
>                                   1) | MP_KEY_DOWN);
> @@ -1121,10 +1112,7 @@
>                      mouse_timer = GetTimerMS();
>                  }
>  #ifdef HAVE_NEW_GUI
> -                // Ignor mouse button 1 - 3 under gui 
> -                if (use_gui && (Event.xbutton.button >= 1)
> -                    && (Event.xbutton.button <= 3))
> -                    break;
> +                if ( use_gui ) { break; }
>  #endif

Double-click stuff would probably still work without this stuff, too.
Mind, I do not say I'm against this, but I don't like do-everything-(and
in most cases nothing-properly-)patches.

Greetings,
Reimar Döffinger