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

[laurent wozniak]

Hello,

Before we continue the discussion, just as a reminder, this patch is about:
1: refactoring mouse events
1.1: code factorization
1.2: bugs corrections
2: double click handling

Reimar Döffinger wrote:
> Hello,
> 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
>
>   
Do we really want to continue supporting the obsolete GTK1 ?

>> @@ -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.
>
>   
Well, most other media players have this binding.
As a user, I prefer to have a well know binding activated by default.

>> +// Mouse button double click time
>> +// Default value may be overridden during the gui initialization 
>> +static unsigned int double_click_time = 300;
>> +
>>     
>
> probably should be a command line option.
>
>   
A reasonable default value is mandatory.
If using the GUI, it will be overridden by the correct value from the 
window system.
Then, a user preference could be added, from a configuration file or 
from the command line.
But I can't imagine a user changing the default value for this...

>> @@ -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;
>>     
>
>
> Why does a no_command key bind have to behave completely than other key
> binds?
> I would simply add the
>
>   
>> +      if (strcmp(cmd, MP_CMD_TXT_NO_COMMAND) == 0)
>> +        return NULL;
>>     
>
> And leave everything else as is.
>
>   
You're right, the "no_command" is only handled by the 2 lines you're citing.
The other changes correct the bug:
Unbound key was blocking next key if sent nearly simultaneously (wheel 
up/down mouse buttons, for wheel emulation, not the wheel itself) in 
command processing.

>> +  // 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.
>
>   
Well, in a user perspective, the more you have to configure, the less 
you're happy.
I'm working in a software company, we can spend months to write code if 
it reduces the need of user configuration.
That's not stupid since configuration represent a cost for each client 
when code is a one time cost.
I already had my ass kicked for having put in configuration things that 
could have been somehow computed ;)

>> +// This put key layer filter events
>> +// It can translate the event into another one 
>>     
>
> Should be doxygen-compatible comments.
> Also I think that translating is not such a great idea, won't it break
> thinks for people who have bound a mouse button and click real fast?
> They would only get half the events compared to before.
> IMHO instead the double click event should be generated in addition.
>
>   
Events translation was already implemented and necessary before this 
path, in several places.
For example, a button press is converted to a button release if the 
button is the wheel mouse.
This patch is factorizing this behavior in a single place.

Not all mouse button events can be bound: only button release, not 
button press.
The double click event replace a key press (not bound).
In term of command execution, you add a command hook, you don't suppress 
one.

The sequence:
(1) Press, (2) Release, (3) Press, (4) Release
generates:
(2) Click, (3) Double click, (4) Click.

>> +    // Translate mouse wheel: Interpret press event as mouse button release event
>> +    if (btn == 3 || btn == 4) {
>> +      code = MOUSE_BTN0 + btn;
>> +    }
>>     
>
> I don't really like all these mouse wheel changes, first because the do
> not seem absolutely necessary for now (just BTN0-BTN2 double clicks is probably
> enough for now), and some mice do have more than one mouse wheel (though
> the old code didn't behave entirely right in that case either).
>
>   
Currently, the mouse wheel is not working correctly, especially the 
mouse wheel emulation buttons.
So, I added the correction since I'm using it: depending on the action 
to do I use either the real wheel or the wheel emulation.
Logitech (and probably others) has added this emulation for ergonomic 
purpose and it's really cool.

The old code is not handling a second wheel correctly at all, since, 
excluding the 3 first buttons, it double all button clicks ...

>> Index: libvo/vo_sdl.c
>> ===================================================================
>> --- libvo/vo_sdl.c	(revision 19814)
>> +++ libvo/vo_sdl.c	(working copy)
>> @@ -1189,10 +1189,7 @@
>>  			case SDL_MOUSEBUTTONDOWN:
>>  				if(vo_nomouse_input)
>>  				    break;
>> -				if(event.button.button == 4 || event.button.button == 5)
>> -					mplayer_put_key(MOUSE_BASE+event.button.button-1);
>> -				else
>> -					mplayer_put_key((MOUSE_BASE+event.button.button-1) | MP_KEY_DOWN);
>> +				mplayer_put_key((MOUSE_BTN0+event.button.button-1) | MP_KEY_DOWN);
>>  				break;			    
>>  		
>>  			case SDL_MOUSEBUTTONUP:
>> @@ -1198,7 +1195,7 @@
>>  			case SDL_MOUSEBUTTONUP:
>>  				if(vo_nomouse_input)
>>  				    break;
>> -				mplayer_put_key(MOUSE_BASE+event.button.button-1);
>> +				mplayer_put_key(MOUSE_BTN0+event.button.button-1);
>>  				break;
>>  	
>>  			/* graphics mode selection shortcuts */
>>     
>
> Why this MOUSE_BASE -> MOUSE_BTN0 change?
>   
To be homogeneous with other vo drivers.
And that's really useful when you perform a find reference on the entire 
source code.

Cheers,
Laurent