Hi all, with regard to the -noconfig patch I fixed the command line preparsing to allow using options normally. I put the preparser code with the config file parser together bcs this code is also used by both mplayer and mencoder. After applying the patch command line like "foo.avi -sub -really-quiet" will behave correctly (ie. -really-quiet won't be interpreted as an option). Testing welcome, I plan to apply it tonight unless there is some problem. Albeu
Hi, On Sunday 13 April 2008 18:52, Alban Bedel wrote:
with regard to the -noconfig patch I fixed the command line preparsing to allow using options normally. I put the preparser code with the config file parser together bcs this code is also used by both mplayer and mencoder.
Good, I'll wait with my patch until yours one will be applied.
--- parser-cfg.c (revision 26439) +++ parser-cfg.c (working copy) @@ -245,4 +245,42 @@ return ret; }
+extern int mp_msg_levels[]; + +/// Parse the command line option that must be handled at startup. +int m_config_preparse_command_line(m_config_t *config, int argc, char **argv) +{ + int msg_lvl, i, r, ret = 0; + char* arg; + m_option_t* opt; + + // Hack to shutup the parser error messages. + msg_lvl = mp_msg_levels[MSGT_CFGPARSER]; + mp_msg_levels[MSGT_CFGPARSER] = -11; + + config->mode = M_COMMAND_LINE_PRE_PARSE; + + for(i = 1 ; i < argc ; i++) { + arg = argv[i]; + // Ignore non option + if(arg[0] != '-' || arg[1] == 0) continue; + arg++; + // No more options after -- + if(arg[0] == '-' && arg[1] == 0) break; + + opt = m_config_get_option(config,arg); + // Ignore invalid option + if(!opt) continue; + // Set, non-pre-parse options will be ignored + r = m_config_set_option(config,arg, + i+1 < argc ? argv[i+1] : NULL); + if(r < 0) ret = r; + else i += r; + } + + mp_msg_levels[MSGT_CFGPARSER] = msg_lvl; + + return ret; +} + ///@}
Why this is needed to be implemented as separate function instead of reusing m_config_parse_*_commandline()? Perhaps it will be better just to shut up parser errors, set appropriate config mode and run normal m_config_parse_mp_commandline() or m_config_parse_me_commandline() respectively?
Index: m_config.c ================================================================ === --- m_config.c (revision 26439) +++ m_config.c (working copy) @@ -308,6 +308,13 @@ mp_msg(MSGT_CFGPARSER, MSGL_ERR,MSGTR_InvalidCmdlineOption,arg); return M_OPT_INVALID; } + // During pre-parse don't set non-pre-parse options + // Otherwise don't set pre-parse options + if(((config->mode == M_COMMAND_LINE_PRE_PARSE) && + !(co->opt->flags & M_OPT_PRE_PARSE)) || + ((config->mode != M_COMMAND_LINE_PRE_PARSE) && + (co->opt->flags & M_OPT_PRE_PARSE)))
The problem is here. Just imagine that option is marked as CONF_PRE_PARSE and is not specified in the command line, but is specified in the config file. It such case option just will be ignored. I suggest to add another flag (e.g. M_OPT_PREPARSE_DONE) and toggle it while preparsing, and only skip option at the main parse if this flag is set.
+ set = 0;
Why not exit from m_config_parse_option() immediately?
On Sun, 13 Apr 2008 20:33:42 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hi,
On Sunday 13 April 2008 18:52, Alban Bedel wrote:
--- parser-cfg.c (revision 26439) +++ parser-cfg.c (working copy) @@ -245,4 +245,42 @@ return ret; }
+extern int mp_msg_levels[]; + +/// Parse the command line option that must be handled at startup. +int m_config_preparse_command_line(m_config_t *config, int argc, char **argv) +{
Why this is needed to be implemented as separate function instead of reusing m_config_parse_*_commandline()? Perhaps it will be better just to shut up parser errors, set appropriate config mode and run normal m_config_parse_mp_commandline() or m_config_parse_me_commandline() respectively?
The normal command line parser build a tree or list with all the filenames, that's useless at this point. IMHO it's better to just have a separate function than adding more hacks to the command line parsers.
Index: m_config.c ================================================================ === --- m_config.c (revision 26439) +++ m_config.c (working copy) @@ -308,6 +308,13 @@ mp_msg(MSGT_CFGPARSER, MSGL_ERR,MSGTR_InvalidCmdlineOption,arg); return M_OPT_INVALID; } + // During pre-parse don't set non-pre-parse options + // Otherwise don't set pre-parse options + if(((config->mode == M_COMMAND_LINE_PRE_PARSE) && + !(co->opt->flags & M_OPT_PRE_PARSE)) || + ((config->mode != M_COMMAND_LINE_PRE_PARSE) && + (co->opt->flags & M_OPT_PRE_PARSE)))
The problem is here. Just imagine that option is marked as CONF_PRE_PARSE and is not specified in the command line, but is specified in the config file. It such case option just will be ignored.
I suggest to add another flag (e.g. M_OPT_PREPARSE_DONE) and toggle it while preparsing, and only skip option at the main parse if this flag is set.
I see what you mean, I'll do that.
+ set = 0;
Why not exit from m_config_parse_option() immediately?
Because to correctly parse the command line we need to know how many parameters each option take, and that is returned by the option parser. Albeu
On Sun, 13 Apr 2008 19:13:48 +0200 Alban Bedel <albeu@free.fr> wrote:
On Sun, 13 Apr 2008 20:33:42 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hi,
On Sunday 13 April 2008 18:52, Alban Bedel wrote:
Index: m_config.c ================================================================ === --- m_config.c (revision 26439) +++ m_config.c (working copy) @@ -308,6 +308,13 @@ mp_msg(MSGT_CFGPARSER, MSGL_ERR,MSGTR_InvalidCmdlineOption,arg); return M_OPT_INVALID; } + // During pre-parse don't set non-pre-parse options + // Otherwise don't set pre-parse options + if(((config->mode == M_COMMAND_LINE_PRE_PARSE) && + !(co->opt->flags & M_OPT_PRE_PARSE)) || + ((config->mode != M_COMMAND_LINE_PRE_PARSE) && + (co->opt->flags & M_OPT_PRE_PARSE)))
The problem is here. Just imagine that option is marked as CONF_PRE_PARSE and is not specified in the command line, but is specified in the config file. It such case option just will be ignored.
I suggest to add another flag (e.g. M_OPT_PREPARSE_DONE) and toggle it while preparsing, and only skip option at the main parse if this flag is set.
I see what you mean, I'll do that.
After a few more reflections it's simpler and more logic to simply skip them when in command line mode instead of when not in command line preparsing mode. New patch attached. Albeu
Hi, On Sunday 13 April 2008 21:36, Alban Bedel wrote:
After a few more reflections it's simpler and more logic to simply skip them when in command line mode instead of when not in command line preparsing mode. New patch attached.
Ok, and thanks for explanations in your previous e-mail.
Index: m_config.c ================================================================ === --- m_config.c (revision 26439) +++ m_config.c (working copy) @@ -308,6 +308,13 @@ mp_msg(MSGT_CFGPARSER, MSGL_ERR,MSGTR_InvalidCmdlineOption,arg); return M_OPT_INVALID; } + // During command line preparse set only pre-parse options + // During command line parse don't set pre-parse options + if(((config->mode == M_COMMAND_LINE_PRE_PARSE) && + !(co->opt->flags & M_OPT_PRE_PARSE)) || + ((config->mode == M_COMMAND_LINE) && + (co->opt->flags & M_OPT_PRE_PARSE)))
With currently available preparsed options this will work fine: -really-quiet has no options and -noconfig is command line only. But in the case when preparsed option has an argument and is specified in both command line and config file, the config's value will override the console line's one, and this is not a desirable behaviour. So in order to make preparse routine all-purpose this issue should be solved. Who know what kind of options will be there in future? Sincerely yours, Andrew
On Sun, 13 Apr 2008 21:44:01 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hi,
On Sunday 13 April 2008 21:36, Alban Bedel wrote:
After a few more reflections it's simpler and more logic to simply skip them when in command line mode instead of when not in command line preparsing mode. New patch attached.
Ok, and thanks for explanations in your previous e-mail.
Index: m_config.c ================================================================ === --- m_config.c (revision 26439) +++ m_config.c (working copy) @@ -308,6 +308,13 @@ mp_msg(MSGT_CFGPARSER, MSGL_ERR,MSGTR_InvalidCmdlineOption,arg); return M_OPT_INVALID; } + // During command line preparse set only pre-parse options + // During command line parse don't set pre-parse options + if(((config->mode == M_COMMAND_LINE_PRE_PARSE) && + !(co->opt->flags & M_OPT_PRE_PARSE)) || + ((config->mode == M_COMMAND_LINE) && + (co->opt->flags & M_OPT_PRE_PARSE)))
With currently available preparsed options this will work fine: -really-quiet has no options and -noconfig is command line only.
But in the case when preparsed option has an argument and is specified in both command line and config file, the config's value will override the console line's one, and this is not a desirable behaviour.
Yes, that could happen, so here is a patch implementing this. Albeu
Hi, On Sunday 13 April 2008 22:57, Alban Bedel wrote: [...]
With currently available preparsed options this will work fine: -really-quiet has no options and -noconfig is command line only.
But in the case when preparsed option has an argument and is specified in both command line and config file, the config's value will override the console line's one, and this is not a desirable behaviour.
Yes, that could happen, so here is a patch implementing this.
Fine, I'm ok with this patch. Sincerely yours, Andrew
On Sun, 13 Apr 2008 22:40:41 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hi,
On Sunday 13 April 2008 22:57, Alban Bedel wrote: [...]
With currently available preparsed options this will work fine: -really-quiet has no options and -noconfig is command line only.
But in the case when preparsed option has an argument and is specified in both command line and config file, the config's value will override the console line's one, and this is not a desirable behaviour.
Yes, that could happen, so here is a patch implementing this.
Fine, I'm ok with this patch.
Committed Albeu
participants (2)
-
Alban Bedel -
Andrew Savchenko