[MPlayer-dev-eng] [PATCH] -slave-idle option

[Alexander Strasser]

Didn't dive into play tree mistery, but anyway a few comments
from a quick look.

Oded Shimon wrote ( On Thu, Feb 24, 2005 at 07:04:50PM +0200 ):
> Index: cfg-mplayer.h
> ===================================================================
> RCS file: /cvsroot/mplayer/main/cfg-mplayer.h,v
> retrieving revision 1.240
> diff -u -r1.240 cfg-mplayer.h
> --- cfg-mplayer.h	30 Jan 2005 10:27:26 -0000	1.240
> +++ cfg-mplayer.h	24 Feb 2005 16:58:15 -0000
> @@ -392,6 +388,8 @@
>  #endif
>  
>  	{"slave", &slave_mode, CONF_TYPE_FLAG,CONF_GLOBAL , 0, 1, NULL},
> +	{"slave-idle", &slave_idle, CONF_TYPE_FLAG,CONF_GLOBAL , 0, 1, NULL},
> +	{"noslave-idle", &slave_idle, CONF_TYPE_FLAG,CONF_GLOBAL , 0, 0, NULL}, 

Should all be changed to idle, as mentioned in the other mail.

[skip]
> +while (slave_idle && !filename) {
> +    play_tree_t * entry = NULL;
> +    mp_cmd_t * cmd;
> +    fprintf(stderr,"got in");

No spam, madam ;)

> +    while (!(cmd = mp_input_get_cmd(0,1,0))) sleep(1);
> +    switch (cmd->id) {
> +        case MP_CMD_LOADFILE:
> +            entry = play_tree_new();
> +            play_tree_add_file(entry, cmd->args[0].v.s);
> +            break;
> +        case MP_CMD_LOADLIST:
> +            entry = parse_playlist_file(cmd->args[0].v.s);
> +            break;
> +        case MP_CMD_QUIT:    
> +            exit_player_with_rc(MSGTR_Exit_quit, (cmd->nargs > 0)? cmd->args[0].v.i : 0);
> +            break;                
> +    }

Didn't test it. But is it safe when any other command arrives, and the control flow
continues from here.

> +        
> +    while (entry) { // I use a while instead of an if so i can break at any point
> +        if (playtree) play_tree_free_list(playtree->child, 1);
> +        else playtree=play_tree_new();
> +        if (!playtree) break;
> +            
> +        play_tree_set_child(playtree, entry);
> +        playtree_iter = play_tree_iter_new(playtree, mconfig);
> +        if (!playtree_iter) break;
> +            
> +        if (play_tree_iter_step(playtree_iter,0,0) != PLAY_TREE_ITER_ENTRY) {
> +            play_tree_iter_free(playtree_iter);
> +            playtree_iter = NULL;
> +            break;
> +        }
> +        filename = play_tree_iter_get_file(playtree_iter, 1);
> +        break;
> +    }
> +}

This looks bit messy. In any case it would be better to add a few comments
about what happens in there. Maybe substituting the whole thing with a switch
or so if possible would also be cleaner.

[...]

  Alex (beastd)