[MPlayer-dev-eng] [PATCH] Apple Remote support

[Ulion]

2007/8/13, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> Hello,
> On Mon, Aug 13, 2007 at 03:38:41PM +0800, Ulion wrote:
> > 2007/8/13, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > >
> > > > +  cc_check -framework IOKit && tmp_run && _apple_remote=yes
> > >
> > > configure checks should not execute anything unless absolutely
> > > necessary.
> > I think this run check is necessary when apple-remote support is setted 'auto'.
>
> I do not really like it because I think it causes problems for everyone
> compiling for a different machine.
> But configure is Diego's domain and if he is fine with it I won't
> complain.
> But the function you use to test is a copy-and-paste of the FindHIDDevices
> function you updated, so it would be good to update this as well so they
> match again.
for machine has Apple Remote support it run return 0 and _apple_remote
setted to yes.
for machine not support Apple Remote, it run return non-zero,
_apple_remote setted to no.
this is exactly what the 'auto' mean, check whether current machine
support Apple Remote.

>
> > > > +#ifdef HAVE_APPLE_REMOTE
> > > > +  { { AR_PLAY, 0}, "pause" },
> > > > ...
> > > > +  { { AR_VDOWN, 0 }, "volume -1"},
> > > > +#endif
> > >
> > > I think the #ifdef is not needed.
> > in the context, other codes all have this '#ifdef' such as joystick's,
> > so we follow the style.
>
> Actually only joystick is comparable, the other ifdefs are not for input
> devices but for MPlayer features, so their bindings make no sense (and
> actually print an error) when the feature is not available.
> But I admit this is a separate issue.
I prefer keep the #ifdef because since the compile does not support
Apple Remote, the key to function mapping is useless.

>
> > > > +static IOHIDDeviceInterface **hidDeviceInterface = NULL;
> > > > +static int inited = 0;
> > > > +static int hidDeviceIsOpen = 0;
> > > > +static io_iterator_t hidObjectIterator = (io_iterator_t)NULL;
> > >
> > > Useless initializations.
> > hidObjectIterator moved into init function.
> > other init maybe useless, but for make sure.
>
> I don't really mind, but the reason I call them useless is because the C
> standard explicitly says that this kind of variables (not allocated dynamically on
> the stack) will be initialized to zero.
yes, I known, init some static variables explicit just for notice.

> Btw. I think you forgot to set inited = 0 in mp_input_ar_close.
> While I probably as always could find more nits to pick, I don't have
> much time for reviewing and the code seems good enough for me.
>
fixed.


-- 
Ulion
-------------- next part --------------
A non-text attachment was scrubbed...
Name: apple_remote_grammer_fixed3.patch
Type: application/octet-stream
Size: 28626 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070813/0cf7dcec/attachment.obj>