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

[Alban Bedel]

Hi Andriy N. Gritsenko,

on Wed, 14 May 2003 19:15:39 +0300 you wrote:

>     Hi, Arpi!
> 
>     So I misunderstood whole thing - I thought we were about common API
> as for parser as for UIs. So we were speaking about different things, by
> different languages almost. ;)  Now I've get rid of that
> misunderstanding so I could say - you did all I wanted with your level
> 0. I think it's enough and I happy. ;)
> 
>     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.

> ------------------------- 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.
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.


> /* 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 ??????

> // 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.


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

> /*
> all below this have
>   ->priv as config_t*
> */
> // subconfig (dialog)	-		-	-	config_t*
>   CONF_TYPE_SUBCONFIG,
> // choose list		char**		-	length	config_t*
>   CONF_TYPE_CHOOSE,
> // select list		char***		-	-	config_t*
>   CONF_TYPE_LIST,
> // config group (sep.)	-		-	-	config_t*
>   CONF_TYPE_DUMMY
> };
Could pls explain these a bit more.

> /* 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.

> #define CONF_NUM_TYPES			(CONF_TYPE_DUMMY+1)
> 
> /* flags for options */
> #define CONF_MIN		(1<<0)
> #define CONF_MAX		(1<<1)
> #define CONF_RANGE		(CONF_MIN|CONF_MAX)
> // This option cannot be in config file
> #define CONF_NOCFG		(1<<2)
> // This option cannot be in command line
> #define CONF_NOCMD		(1<<3)
> // This option is global : it won't be saved and only the command line
> // parser will set it when it's parsed (ie. it won't be set later)
> // e.g options : -v, -quiet
> #define CONF_GLOBAL		(1<<4)
> // Do not save this option : it won't be saved on config save
> // or by UI on context change
> #define CONF_NOSAVE		(1<<5)
> // This subconfig has unnamed parameters list (i.e. 0.3:-1:-1 for
> example)// It looks like a regular subconfig but different in parsing a
> bit
> #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.

> /* 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.

> #endif /* __CFG_H */
> ------------------------- end of cfg.h
> 
>     With best wishes.
My best wishes too.
	Albeu