[MPlayer-G2-dev] Re: g2 config - restart...

[Arpi]

Hi,

> >     BTW, did you wrote new config_t and module_info_t structures (I mean
> > API layer 0) yet? If you didn't then there is a draft here (from yours
> > API description, of course - you really fine generalized all). :)  Also
> > I typed here some #define because they have values of fields of
> > config_t. I hope I did all correctly now. :)  BTW, I'm not sure if
> > filename must be in module_info_t - plugin subsystem can have it in
> > internal arrays. That subsystem anyway has to keep bad filenames list to
> > avoid trying its (in cache also, I think) further. Bad filenames - if
> > plugins are in plugin's subdirectory but are not plugins or have
> > incompatible version.
> 
> I like the cache idea but imho the it should use more than the filename
> to identify a file. Smthg like filename + date (a hash would be better
> but that's probaly overkill) so we could detect if the file changed
> since last run.

Of course filename is not enough, maybe filename+date also not, but dunno.
But these infos don't belong to the module_info_t at all.
The filename is there for the dlopen() only, to allow any filename, but i'm
not sure we really want to allow it. Esp., that CLI interfaces would load
plugins by filename, without scanning the plugin dir first.


> > ------------------------- begin of cfg.h
> > #ifndef __CFG_H
> > #define __CFG_H 1
> > 
> > typedef struct {
> >   char *name;
> >   void *p; /* variable pointer, type specific */
> >   int type;
> >   unsigned int flags;
> >   float min, max; /* range */
> >   void *priv; /* additional pointer, type specific */
> >   char *desc; /* short description for help and gui */
> > } config_t;
> 
> I liked the idea you had of putting some hint for GUI widget type.

Yes, I also like that idea.

> It would avoid having different options type wich are in fact the
> same C type and it would also make things nicer for GUI writers.
> Also type specific flags would be nice. Ok there is priv but
> if a type only need a few flags priv is overkill. We could either
> add another flag field or use the lower bit for common flags and
> the higher for type specific ones.

Yes, we could add more flags, to more exactly describe how it should look
like on the GUI. Including step units (for sliders), and others.
So a clever GUI coder could write an 'interpreter' which dinamically build
gui panels based on these info. I've seen such thing in xine, they build Xv
setup panel runtime based on the attribute list returned by xvinfo.

> > /* note: version has to be first in ModuleInfo struct in loadable module
> >    to allow us always do validation/version check of module */
> > typedef struct {
> >   unsigned char version[4]; // { PLUGIN_VERSION_MAJOR,
> >   PLUGIN_VERSION_MINOR, 'M', 'P' } 
> >   int type; // for loadable module identification
> >   char *name;
> >   char *desc; // short description for help and gui
> >   char *maintainer;
> >   char *author;
> >   char *comment;
> >   config_t *opts;
> >   void *param_defaults;
> >   size_t *param_size;
> >   char *filename; // filled by plugin subsystem, NULL for internal
> > } module_info_t;
> > 
> > /* type (menu field)	->p		->min	->max	->priv */
> > enum {
> > // flag (checkbox)	int*		reset	set	-
> >   CONF_TYPE_FLAG=0,
> > // integer (slider)	int*		min	max	-
> >   CONF_TYPE_INT,
> > // float (slider)	float*		min	max	-
> >   CONF_TYPE_FLOAT,
> > // position (slider)	off_t*		min	max	-
> >   CONF_TYPE_POSITION,
> > // string (input box)	char**		min	max	-
> >   CONF_TYPE_STRING,
> > // string as list - must be obsoleted to CONF_TYPE_LIST
> >   CONF_TYPE_STRING_LIST,
> Obsolated stuff in a draft ??????

I can't see why is string list is obsolete? It's a good thing and is used a
lot by g1 and in g2 too.

> 
> > // function		cfg_func*	-	-	revert-cfg_func*
> >   CONF_TYPE_FUNC,
> >   CONF_TYPE_FUNC_PARAM,
> >   CONF_TYPE_FUNC_FULL,
> I think we should avoid callback based stuff. These were nice in the
> old days for very specific stuff. But now the only place in G1 where
> they are really needed is for the -aa* options of aalib. But as in G2
> each module instance have it's own context it doesn't really make
> sense as options parsing append before instanciation.
> For options global to a layer that would make some sense but then the
> callback also need to get the struct/context on wich it should
> operate. So this context should be static otherwise you can't put
> it in the config_t->priv field. But we don't want static stuff so
> for G2 lib that's anyway completly unusefull.
> We may let that for app level stuff but i doubt it's really usefull.

it's usefull. think of an external plugin (wirtten by third party) which
requires special parsing for some parameters. Ok, they could receive string
and do parsing at open(), but implementing a parsing function is better, as
the UI can notice syntax errors before getting to open().

but imho we don't need 3 of them, only one such type is enough.
but we should define how to pass struct offset/pointer and number of args.

> > // text (none)		char**		-	-	-
> >   CONF_TYPE_PRINT,
> That's also only usefull at app level.

this one is really obsolete, imho.
it was used in g1 to abort UI at parsing level with error... (for obsolete
parameters or when requested not compiled-in things)

or dunno, maybe it's still usefull for something...
but should be handled differently.

> > /* these macros are very useful for parsers or UI */
> > #define CONF_TYPE_HAS_CHILD(x)		(x>=CONF_TYPE_SUBCONFIG)
> > #define CONF_TYPE_ALLOW_WILDCARD(x)
> > #(x==CONF_TYPE_LIST||x==CONF_TYPE_CHAIN||x==CONF_TYPE_FUNC_FULL)
> I don't like this way for wildcard. Wildcard support depend on the
> string 2 C parser so imho it should be queried from it.

i also don't like these

> > #define CONF_SEQUENT		(1<<6)
> Dunno if that's really usefull as it's possible to have a parser
> how handle both mode (ie named and unamed suboptions) and even
> a mix of the 2.
> 
> > // It used for lists to reverse order of arguments in char***
> > #define CONF_REVERSE		(1<<7)
> Is that really useful ??? I mean you need code to support that, on
> the other hand you can only just reorder your list instead of
> adding this flag.

agree

> > /* macro to get relative address of x.y in struct x */
> > #define STRUCT_OFF(x,y)		((void *)(&x.y-x))
> I doubt that this macro really work. And even if it work it need an
> instance to apply it to. Why don't you just take the one in m_struct.h
> (wich is from glib.h) wich just need the declaration of the struct.

agree

btw i defined in my layer 0 draft, that you should make a static stuct,
filled with the defaults. it also serevs as target of pointers in config_t,
but that static instance is read-only, must not be modified.

but you're maybe right, if defaulting everything to zero is ok, then we
could left this out (optinally).


A'rpi / Astral & ESP-team

--
Developer of MPlayer, the Movie Player for Linux - http://www.MPlayerHQ.hu