Re: [cp-patches] Patch: start of ALSA MIDI provider code

[Anthony Green]

On Sun, 2005-10-02 at 19:45 +0200, Mark Wielaard wrote:
> Cool stuff.
> You call the jni library "gjsmalsa", what does that stand for?

Gnu Javax Sound Midi ALSA.   I also have a "gjsmdssi" for Gnu Javax
Sound Midi DSSI.  

> >  dnl -----------------------------------------------------------
> > +dnl ALSA code (enabled by default)
> > +dnl -----------------------------------------------------------
> > +AC_ARG_ENABLE([alsa],
> > +              [AS_HELP_STRING(--disable-alsa,compile ALSA providers 
> > (disabled by --disable-alsa) [default=yes])],
> > +              [case "${enableval}" in
> > +                yes) COMPILE_ALSA=yes ;;
> > +                no) COMPILE_ALSA=no ;;
> > +                *) COMPILE_ALSA=yes ;;
> > +              esac],
> > +              [COMPILE_ALSA=yes])
> > +if test "x$COMPILE_ALSA" = "xyes"; then
> > +  AC_CHECK_HEADERS([alsa/asoundlib.h],,COMPILE_ALSA=no)
> > +fi
> > +AM_CONDITIONAL(CREATE_ALSA_LIBRARIES, test "x${COMPILE_ALSA}" = xyes)
> 
> I think it should say AS_HELP_STRING(--enable-alsa,... reading
> --disable-alsa in the configure help output here is a but confusing.

Yes, I think so also.  I was just copying what others had done.

> Also when running configure with --disable-alsa I get:
>       configure: error: conditional "AMDEP" was never defined.
> I couldn't figure out why.

I'll try to figure this out.

> Feel free to call the new native directory just 'alsa' or 'midialsa'
> instead of the super long gnu-javax-sound-midi-alsa. That might get
> boring and we don't have that much jni code that we really need to use
> package namespacing to disambiguate things.

Ok, I'll go with midi-alsa.

> In your .c files please use the copyright boilerplate as is. So don't
> reindent it. It should be similar to how you did the .java file
> boilerplate.

Hmmm.. I copied that boilerplate from other .c files.  Maybe I just
picked a bad one.

> Instead of things like   /* Nothing yet.  */ please use FIXME to help
> people find unimplemented things. Although in general you should just
> not stub things but leave them out for now.
> 
> Please don't use abort() or printf() but do something like:
>         
>         JCL_ThrowException (env, "java/lang/InternalError",
>                             snd_strerror(error));
>         return;

Right.

> The .java source files don't compile with gcj (4.0.2).
> /usr/bin/gcj -Wno-deprecated --encoding=UTF-8 --bootclasspath '' --classpath 
> ..:../vm/reference:..:../external/w3c_dom:../external/sax:.: -C -d . -MD -MF 
> lists/gnu-javax-sound.deps -MT lists/gnu-javax-sound.stamp -MP 
> @lists/gnu-javax-sound.list
> ../gnu/javax/sound/midi/alsa/AlsaPortDevice.java:91: error: Type 'Info' not 
> found in the declaration of the return type of method 'getDeviceInfo'.
>      public Info getDeviceInfo()
>             ^
> It does compile with jikes though. So this might be a gcj or build
> infrastructure bug.

This is probably a gcj bug.  I'm building it with ecj.  I'll tweak it to
build with gcj.

> There is lots and lots of stuff that look autogenerated:

Yes, Eclipse is wonderful.

>   /* (non-Javadoc)
>    * @see javax.sound.midi.MidiDevice#getTransmitter()
>    */
>   public Transmitter getTransmitter() throws MidiUnavailableException
>   {
>     // TODO Auto-generated method stub
>     return null;
>   }
> 
> Please don't include that (non-Javadoc) comment.
> And are you sure you really need all this stuff already?

Actually this is correct code.  I just need to remove the comments.

> I really don't want to have so many stubs in our code. So if at all
> possible just leave things out that are not necessary yet.
> 
> And please try to keep your lines < 80 characters for readability.

I've tried my best.  I'm not sure how to break up some of the deeply
nested code.

So, OK with these changes or do you want me to resubmit?

AG