[MPlayer-dev-eng] Re: [PATCH] one input key more commands

[Alban Bedel]

On Sat, 31 Mar 2007 19:35:24 +0200
Ötvös Attila <oattila at chello.hu> wrote:

> Hi All!
> 
> I modified input key bind so can bind more commands to one key.
> 
> Whith mp_input_set_section() can set which section of key command will use.

That sound like a good idea.
 
> Example settings in mplayer.c:
> 
> mp_input_set_section(NULL);
> if(mpctx->stream->type==STREAMTYPE_DVDNAV) mp_input_set_section("dvdnav");
> if(mpctx->stream->type==STREAMTYPE_TV) mp_input_set_section("tv");
> 
> Example input.conf:
> 
> RIGHT seek +10
> RIGHT {tv} tv_step_channel 1
> RIGHT {teletext} vbi_step_page 1
> RIGHT {dvdnav} dvdnav 4
> RIGHT {menu} menu right

I know it's just an example, but settings keyboard binds for the menu is
useless as libmenu have its own key handling.

> +static mp_cmd_bind_section_t*
> +mp_input_get_bind_section(char *section) {
> +  mp_cmd_bind_section_t* bind_section = cmd_binds_section;
> +
> +  if (section==NULL) section="default";
> +  while (bind_section) {
> +    if(strcmp(section,bind_section->section)==0) {
> +      return bind_section;
> +      }

Could you please just use the identation style used in the rest of the
file? I know you can use your own style for new functions. However it
is preferable if we avoid having different styles in the same file.

> -  if(cmd_binds)
> -    cmd = mp_input_find_bind_for_key(cmd_binds,n,keys);
> +  if(cmd_binds_section) {
> +    bind_section = mp_input_get_bind_section(section);
> +    if(bind_section)
> +    cmd = mp_input_find_bind_for_key(bind_section->cmd_binds,n,keys);
> +    }

Wrong identation, you _must_ respect the original indentation style when
extending an existing function.

> +  if (NULL==(bind_section=mp_input_get_bind_section(section))) {
> +    mp_msg(MSGT_INPUT,MSGL_INFO,"Bind section alloc error.\n");
> +    return;
> +    }

Imho those kind of test are useless, if we can't allocate a couple bytes
on a modern OS, then the system will be so hoosed that a sig11 is just
as good.

> +      if(keys[j] == 0 && bind_section->cmd_binds[i].input[j] == 0 ) {
> +    bind = &bind_section->cmd_binds[i];
>  	break;
>        }

Just plain wrong indentation.

+void
+mp_input_set_section(char *name)
+{
+if(section) free(section);
+if(name) section=strdup(name); else section=NULL;
+}
+

Same here using the style from the rest of the file would be better.
Also why not directly lookup the bind section here instead of doing it
at every keypress ?

> +struct mp_cmd_bind_section_st {
> +  mp_cmd_bind_t* cmd_binds;
> +  char* section;
> +  mp_cmd_bind_section_t* next;
> +};
[...]
> +  mp_cmd_bind_t* binds;
[...]
> +  while (!cmd_binds_section) {

You meant while(cmd_binds_section) I suppose.

> +    mp_input_free_binds(cmd_binds_section->cmd_binds);
> +    free(cmd_binds_section->section);
> +    binds=cmd_binds_section->next;

Wrong pointer type! Sure a pointer is a pointer but no need to add
useless warnings.

	Albeu