[Mplayer-cvslog] CVS: main cfg-mplayer.h,1.223,1.224

[Attila Kinali]

On Thu, Jul 29, 2004 at 01:32:30PM -0400, Nicolas Plourde wrote:
> Reimar Döffinger wrote:
> 
> >Hi,
> >
> >>+    {"rootwin", &WinID, CONF_TYPE_FLAG, 0, -1, 0, NULL},
> >> #endif
> >> 
> >>-#if defined(HAVE_X11) || defined(MACOSX)
> >>-    {"rootwin", &WinID, CONF_TYPE_FLAG, 0, -1, 0, NULL},
> >>+#ifdef MACOSX
> >>+    {"rootwin", &vo_rootwin, CONF_TYPE_FLAG, 0, 0, 1, NULL},
> >> #endif

This change is bad, very bad, there should be only one
option definition which is enabled by ifdevs.
Please fix it.

> >Is it possible that HAVE_X11 and MACOSX are both defined? In that case 
> >that probably won't work for one. I guess it would be better if both 
> >used the same variable...
> >
> 
> Yes it is possible but WinID is IMO a bad hack and should be fixed to 
> use vo_rootwin variable.
> add something like that:
> cfg-mplayer.h
> #if defined(HAVE_X11) || defined(MACOSX)
>    {"rootwin", &vo_rootwin, CONF_TYPE_FLAG, 0, 0, 1, NULL},
> #endif
> 
> in x11 code dont now where add(pre init stuff):
> if(vo_rootwin)
>    WinID = 0;
> 
> That make more sense, at least tom me. The WinID is also defined in 
> x11_common.h so
> using it in osx specific code is out of question. If someone have a 
> better idea im waiting for your feedback.

The right thing to do here is to move WinID out of x11_common.[ch]
and put it into vide_out.[ch] with proper ifdef around it.
Also a name change as already done in your code from WinID to
vo_rootwin (or something similar more expressive) should be performed.
Please dont fear to make such simple changes to other people
code, if it's bad someone will complain anyways :)


			Attila Kinali