[MPlayer-dev-eng] [PATCH] -noconfig option

[Andrew Savchenko]

Hello,
and thank you for reply.

On Saturday 12 April 2008 12:57, Alban Bedel wrote:
> >  int m_config_parse_config_file(m_config_t* config, char
> > *conffile) {
> > +        if (noconfig)
> > +                return 1;
>
> I don't like that very much. Having a global simply disable a
> function is ugly. Better check before looking for config files.
> It's a bit more work but you could also allow finer disabling
> (to only use the user's config file for ex.).

Done. Some code lookup shows that there is 5 different types of 
config files:
1) system-wide;
2) user-specific;
3) include;
4) user-filedir;
5) gui.

So I introduced 4 new options:
-noconfig, -disable-system-conf, -disable-user-conf, and 
-disable-gui-conf (gui only). Option -noconfig is equivalent to 
all -disable-*-conf specified simaltaneously.

Behaviour of options -include and -user-filedir-conf is not 
changed. The reason is simple: if they are specified in the config 
file and -noconfig is used, they will be obviously ignored; if 
these options are specified in the command line together with 
-noconfig, than I assume that user knows what he/she is doing.
This behaviour is documented in the manual.

> >  // defined in mplayer.c and mencoder.c
> >  extern int verbose;
> > +extern int noconfig;
>
> This has nothing to do in mp_msg.h.

Fixed.

> > The second patch is just cosmetics required after first patch
> > applying.
>
> No need to split that lonely blank line. Added code is allowed
> to contain black lines :)

Very good. I am aware that mplayer developers are very strict about 
any cosmetics, so I was afraid newline will be treated as 
cosmetics.

> > +void preparse_command_line(int argc, char* argv[])
> > +{
> > +    int i;
> > +
> > +    for(i=1; i<argc; i++) {
> > +        if(!strcmp(argv[i], "-really-quiet"))
> > +            verbose= -10;
> > +        if(!strcmp(argv[i], "-noconfig"))
> > +            noconfig= 1;
> > +    }
> > +}
>
> With respect to this patch, this is fine. But it is quiet a hack
> that will catch stuff which would not normally be treated as
> command line options. Ideally this should latter be replaced by
> something robuster.

I do not understand you. What do you want me to do?

The problem is that some command line options must be analyzed 
before configuration files will be. If I'll just move command line 
parsing before config files one, it will break overriding 
precedence (sysmem<-user<-CLI).

Yes, currently used approach is not the best. Moreover it creates 
overhead: these options are parsed twice: in preparse and in 
normal parse routines, though second parsing is absolutely 
useless, but without it mplayer will complain about unknown 
options.

Do you have some ideas how to fix this? I can see three different 
posibilities:
1) Leave it as is.
2) Parse all CLI options before configs, and either :
2a) save them in temporary storage and reassign after config files 
parsing;
2b) mark these options somehow and modify config files parser in 
order to not override marked options.
3) Modify parse subsystem to parse some subset of CLI options 
before config files and forbid their further modification whilst 
config files are parsed.

Variant 2b) sounds as the best for me, but even it will create too 
much overhead. So perhaps our best is to leave things as is?

Well, lets talk about new patches.

The first patch moves some common code from mplayer.c and 
mencoder.c to mpcommon.[ch].

The second patch fixes a bug or, precisely speaking, missed feature 
of mencoder. Look at the man page: it says that mencoder read both 
system-wide and user-specific config files, but currently it reads 
only user-specific one. Anyone can easy check this by 
creating /etc/mplayer/mencoder.conf (or whatever is your's 
MPLAYER_CONFDIR). 

The reason is easly seen in comparison between parse_cfgfiles() in 
mplayer.c and mencoder.c. The last one use only get_path() which 
checks only for home directory. The second patch fixes this, it may 
be applied with or without the first patch.

The third patch adds new functionality as described above. It 
requires both previous patches to be applied.

The fourth patch is cosmetics required after previous patches, real 
cosmetics: not newlines, but indentation shifts.

And finally the fifth patch is the manual page update.

With best regards,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: noconfig_mpcommon.patch
Type: text/x-diff
Size: 3249 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080412/1b2d5c7e/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: noconfig_mencoder_sysconf.patch
Type: text/x-diff
Size: 456 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080412/1b2d5c7e/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: noconfig.patch
Type: text/x-diff
Size: 5158 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080412/1b2d5c7e/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: noconfig_cosmetics.patch
Type: text/x-diff
Size: 1955 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080412/1b2d5c7e/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: noconfig_manual.patch
Type: text/x-diff
Size: 957 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080412/1b2d5c7e/attachment-0004.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080412/1b2d5c7e/attachment.pgp>