Re: [Banshee-List] banshee r3781 - in trunk/banshee: . buildsrc/Core/Banshee.Services/Banshee.Playlistsrc/Core/Banshee.Services/Banshee.Sourcessrc/Extensions/Banshee.Daap src/Extensions/Banshee.Daap/Banshee.Daapsrc/Extensions/Banshee.Daap/Daap

[Alex Hixon <hixon alexander mediati org>]

On Wed, 16 Apr 2008 14:39:49 -0500, "Gabriel Burt" <gabriel burt gmail com>
wrote:
> Hi Alex,
> 
> A couple minor and a couple major issues with this commit
> 
> 1) (Major) Please, do not commit to outside of Daap or Audioscrobbler
> code (eg in particular Core/) without following the patch guidelines
> at http://banshee-project.org/Developers/Patches  You actually broke
> playlists with this commit, and I would have caught this if we had
> done a proper review.  Especially as 1.0 comes closer, we cannot
> afford to have bugs like this (and more subtle) slip into Core/.

*Really* sorry about this.
/me hides in a hole.

> 
> 3) (Major) The entire point of Mono.ZeroConf is to be an abstraction
> layer around Avahi and other backends.  Please remove all
> Avahi-specific code and target the MZC API only.

At the moment, Mono.Zeroconf has several issues related to stability
when using the Avahi backend. I haven't had the chance to file a bug
on MZ itself (I'll do that later today), but I honestly doubt that
the bug will be fixed relatively soon (especially since for the most
part we're unaware why its even happening, other than it's probably
related to threading). I would much rather have stable, working code
than code that causes Banshee to crash unexpectedly and not always
work as expected.

Also, if we do decide to use both, we can still target platforms that
don't provide avahi-sharp, we can fallback to using MZ as the backend
of choice.

I can still drop the avahi-sharp optional dependency if we want, but
as I said above, I'd rather have something that works and works well
than something that's half-broken.

Obviously, once the bug is fixed in MZ, I'll remove all the code 
associated with avahi-sharp here.

> 4) (Minor) Use playlist_id instead of playlistid.  Use track_id
> instead of track, if the variable is actually the id.  Sounds like we
> need to get you a copy of the Framework Design Guidelines.  There is
> an online copy referenced at the bottom of HACKING you can start
> reviewing for now.

Yeah, that'd probably be useful. :)
I'll try and go through and review some of my code shortly.

Cheers,
Alex Hixon