[vlc-devel] [PATCH] Global Hotkeys for windows

[Laurent Aimar]

Thanks for your work.
Here are some comments on your patch:

On Wed, Jan 07, 2009, Hannes Domani wrote:
> Hello
> 
> new variant:
> - use latest vlc threading technology
> - change add_key() so that for every hotkey a global hotkey is created automatically (so i don't have to copy all hotkey texts), this is not ideal since i think i18n won't work (and i have not experience with that)

> +#ifndef WIN32
>  #define add_key( name, value, p_callback, text, longtext, advc ) \
>      add_int_inner( CONFIG_ITEM_KEY, name, text, longtext, advc, p_callback, \
>                     value )
> +#else
> +#define add_key( name, value, p_callback, text, longtext, advc ) \
> +    add_int_inner( CONFIG_ITEM_KEY, name, text, longtext, advc, p_callback, \
> +                   value ); \
> +    { \
> +        char *psz_global_text = \
> +            malloc( strlen( "Global " ) + strlen( text ) + 1 ); \
> +        strcpy( psz_global_text, "Global " ); \
> +        strcat( psz_global_text, text ); \
> +        add_int_inner( CONFIG_ITEM_KEY, "global-" name, psz_global_text, \
> +                       longtext, advc, p_callback, KEY_UNSET ); \
> +        free( psz_global_text ); \
> +    }
> +#endif
 I am not sure how to fix the add_key modification to avoid having code in it
as requested by courmish.

 Maybe the best way would be to modify it to have 2 text arguments, one for
global, one for normal key (even on OS that does not have it).
Like:
+#ifndef WIN32
 #define add_key( name, value, p_callback, text, text_global, longtext, advc ) \
     add_int_inner( CONFIG_ITEM_KEY, name, text, longtext, advc, p_callback, \
                    value )
+#else
+#define add_key( name, value, p_callback, text, text_global, longtext, advc ) \
+    add_int_inner( CONFIG_ITEM_KEY, name, text, longtext, advc, p_callback, \
+                   value ); \
+    add_int_inner( CONFIG_ITEM_KEY, "global-" name, text_global, \
+                       longtext, advc, p_callback, KEY_UNSET ); \
+#endif

(It will probably fix translation issues too).

If anyone has a better suggestion...

> +        psz_hotkey = realloc( psz_hotkey,
> +                strlen( "global-" ) + strlen( p_hotkey->psz_action ) + 1 );
 Checking the realloc return value is prefered.

You may also use asprintf like this:

if( asprintf( &psz_hotkey, "global-%s", p_hotkey->psz_action ) < 0 )
    break; (It is the error case)

(Do not forget to free psz_hotkey everytimes)

> +    SetWindowLong( (HWND)p_intf->p_sys, GWL_WNDPROC, (LONG)WMHOTKEYPROC );
 Is it safe even on win 64 ? (not a big deal, but if not, something has to be
done to avoid at least compilation).
> +    SetWindowLongPtr( (HWND)p_intf->p_sys, GWL_USERDATA, (LONG_PTR)p_intf );
Same here.

> +
> +    /* Registering of Hotkeys */
> +    for( struct hotkey *p_hotkey = p_intf->p_libvlc->p_hotkeys;
> +            p_hotkey->psz_action != NULL;
> +            p_hotkey++ )
> +    {
> +        psz_hotkey = realloc( psz_hotkey,
> +                strlen( "global-" ) + strlen( p_hotkey->psz_action ) + 1 );
> +        strcpy( psz_hotkey, "global-" );
> +        strcat( psz_hotkey, p_hotkey->psz_action );
 You could use asprintf to simplify (or at least check the realloc return value).

> +        i_key = config_GetInt( p_intf, psz_hotkey );
I think var_CreateGetInteger is prefered here.

> +                /* force uppercase */
> +                if( i_vk >= 'a' && i_vk <= 'z' )
> +                    i_vk -= 32;
Is the standard function "toupper()" what you are trying to do ?
(You have to include ctype.h for it).

> +    /* Main loop */
> +    while( 1 )
> +    {
> +        if( !PeekMessage( &message, NULL, 0, 0, 0 ) )
> +        {
> +            vlc_restorecancel( canc );
> +
> +            /* Sleep a bit */
> +            msleep( INTF_IDLE_SLEEP );
> +
> +            canc = vlc_savecancel();
> +
> +            continue;
> +        }
> +
> +        if( !GetMessage( &message, NULL, 0, 0 ) ) break;
> +        DispatchMessage( &message );
> +    }
 Mhh I really don't like the msleep part (useless thread wakeup).

 Would there be a way from another thread to make GetMessage return
a special value, or a special indentifiable message or at least to stop waiting ?

If so, a clean way can be found:
 - Open create the thread itself and does *not* set pf_run.
 - Close will do what is needed for the GetMessage to behave as above, and will
 terminate the thread.
 - RunIntf will then be changed to have something like:
 for( ;; )
 {
    GetMessage();
    if( condition in which we terminate )
       break;
    DispatchMessage();
 } 

Regards,

-- 
fenrir