[MPlayer-dev-eng] [PATCH] add -name and -title options for x video_outs

[Reimar Döffinger]

On Thu, Oct 22, 2009 at 02:38:15PM +0200, Paul TT wrote:
> > > -    wmClass.res_name = name;
> > > +    if (vo_winname != NULL) 
> > > +        wmClass.res_name = vo_winname;
> > > +    else
> > > +        wmClass.res_name = name;
> > 
> > Use ?:
> changing it could be useful for window identification.
> man xter
> /-name
> for example
> with the effort to implement title, name could be useful too.

I meant, write this as
wmClass.res_name = vo_winname ? vo_winname : name;

> > > +void vo_x11_settitle(char *title)
> > > +{
> > > +    if(title != NULL)
> > > +        vo_wintitle = title;
> > > +}
> > [...]
> > > +    vo_x11_settitle(vo_wintitle);
> > 
> > That really makes no sense whatsoever.
> > Also the doxy is nonsense, vo_x11_settitle only
> > sets vo_wintitle, it does not set the title
> > of an existing window.
> mhhhhh iep.
> probably the doxy is superfluous indeed and moving out from x11 code
> that one function ist name and meaning indeed varies

Not the reason it makes no sense is because you use
it like
vo_x11_settitle(vo_wintitle);
which is a very complicated way to do nothing at all.

> > > ...
> > > +#ifdef CONFIG_X11
> > > +  if (vo_wintitle != NULL)
> > > +      title = vo_wintitle;
> > > +#endif
> > 
> > I don't see the point of making this X11-specific.
> because i put it in x11_common functions and it works in x11 ;P
> probably you're right and it should be implemented in other vo's too,
> which i really can't do :(

You don't have to, not at all. But you are making it needlessly
hard to implement it in other vos by making it X11-specific, and
that's not ok IMO.
Btw. it also seems you forgot to put the cfg-mplayer.h part under
#ifdef CONFIG_X11, so it wouldn't compile at all without X11 :-)

> > > Index: mplayer.c
> > > ===================================================================
> > > --- mplayer.c	(revisione 29789)
> > > +++ mplayer.c	(copia locale)
> > > @@ -2191,6 +2191,11 @@
> > >    {
> > >      char* vf_arg[] = { "_oldargs_", (char*)mpctx->video_out ,
> > > NULL }; sh_video->vfilter=(void*)vf_open_filter(NULL,"vo",vf_arg);
> > > +#ifdef CONFIG_X11
> > > +    if(vo_wintitle == NULL)
> > > +      vo_wintitle = mp_basename2 (filename);
> > > +    vo_x11_settitle(vo_wintitle);
> > > +#endif
> > 
> > Seems to me like there's no way to get the current behaviour.
> 
> didnt' really understand what you're saying.. ????

Well, what is the title currently set to? How do you get MPlayer to set
it to exactly the same value after your patch?
I'd find it preferable if the default window title would not be changed
by your patch, in case someone relies on it.

> > > Index: cfg-mplayer.h
> > > ===================================================================
> > > --- cfg-mplayer.h	(revisione 29789)
> > > +++ cfg-mplayer.h	(copia locale)
> > > @@ -170,6 +170,9 @@
> > >  	{"screenh", &vo_screenheight, CONF_TYPE_INT,
> > > CONF_RANGE|CONF_OLD, 0, 4096, NULL}, // Geometry string
> > >  	{"geometry", &vo_geometry, CONF_TYPE_STRING, 0, 0, 0,
> > > NULL},
> > > +	// X11 Hints strings
> > > +	{"name", &vo_winname, CONF_TYPE_STRING, 0, 0, 0, NULL},
> > > +	{"title", &vo_wintitle, CONF_TYPE_STRING, 0, 0, 0, NULL},
> > 
> > I have some doubts this really works correctly with -fixed-vo and
> > different values for these for different files.
> 
> good point, my fault :(
> title should be updated at every file change, right :)

Well, if you don't want that, at least these options should be CONFIG_GLOBAL
to indicate you can not change them per-file.
But making them changeable (preferably in a way that makes it easy to add
support for changing the title via the set_property slave interface) would
be better.
Of course in that regard, implementing this via a VFCTRL/VOCTRL might
be even nicer, since it would allow video filters to easily display
it e.g. in the OSD, too.

> anyway, point to do:
> 1. move from x11 code to video_out generic code
> 2. verify fixed-vo and related stuff.

Yes, other improvements as suggested above would be possible, too, though.