On Fri, 2005-08-05 at 12:16 -0400, Charles Schmidt wrote: > > Can you explain what this is for? Why do you need to disconnect from > > shares? > > iTunes itself has a limit of 5 connections at a time/per day (can't > recall off hand). So, to "play nice" it seems that being able to > disconnect when you're not using the source would be needed (not that > you necessarily /would/, but so that you could). Urgh. Well...if it's just 5 connections at at time, why don't we disconnect from the share when we're not playing something from it? What would be the UI impact of that? If it's per-day...ugh. Hopefully it's not =) > The disconnect command > could be extended to the ipod source as well, telling the ipod source to > save changes, unmount & eject. I happened to overhear a discussion between Bryan Clark (GNOME interaction designer) and David Zeuthen (HAL maintainer) about removable media in general, and one of Bryan's suggestions was to let users pull media out when they want; if an application has unsaved data on the media it should pop up a big warning dialog (maybe have an aural clue for this as well) that it has unsaved data, asking the user to please reinsert the media. When they do, each app automatically saves, and then tells the user it's safe to remove the device. In the ipod case, i wonder if there's a technical reason we can't just allow unplugging at any time; if it's in the middle of a sync, then then user just doesn't get all their songs synced. Providing a way to save state is probably useful too, but it's a lot nicer for people to be able to just remove devices and go. We're all forgetful, I'm sure I'd often e.g. suspend my computer and forget to do things like disconnect from remote music shares. > > Don't forget the space before parens :) Most of the code is good here, but there's still quite a few places where there's a missing space in your latest patch. You might try something like: grep -nHE -e '_[A-Za-z]\(' foo.c Also the function declarations are inconsistent, some functions are: +static gboolean add_playlist_entry_to_mlcl (GtkTreeModel *model, But Rhythmbox style is static gboolean add_playlist_entry_to_mlcl (GtkTreeModel *model, I hate to be a nag about this coding style stuff but it is really a good thing to have the codebase be consistent, it makes it a lot easier to maintain and understand :) Looking over the patch: > + rb_daap_structure_add (mlog, RB_DAAP_CC_MSTT, (gint32)200); > + rb_daap_structure_add (mlog, RB_DAAP_CC_MLID, (gint32)42); Are these just fixed magic values? Or should they really be generated from something? > + buf = g_malloc (file_size); > + > + result = gnome_vfs_read (handle, buf, file_size, > NULL); Hmm...this is potentially pretty large, and remember GLib will just abort() if malloc fails. Is the GMappedFile or whatever interface to mmap() in GLib yet? We should probably use that. > + g_hash_table_insert (share->priv->id_to_entry, > GINT_TO_POINTER (share->priv->num_songs), entry); What about just adding a share id to the RhythmDBEntry? In line with the previous comment about #ifdef, you should feel free to make whatever changes you want to the rest of Rhythmbox's code if it makes things easier. > + share->priv->server = soup_server_new (SOUP_SERVER_PORT, 3689, > NULL); Does this have to be port 3689? What if multiple users are on the machine? I would think this was all dynamically bound and announced with Rendezvous or whatever it's called today :) > + -I$(top_srcdir)/sources \ > + source = rb_daap_source_find_for_uri (uri); > + headers = rb_daap_source_get_headers (source, uri, 0); Yeah, I guess at this point we really have to bite the bullet and accept that the various subdirectories like player/ and sources/ are not independent. Probably trying to keep them that way has led to some of the complexity in the code. > + result = gnome_vfs_dns_sd_resolve_sync (name, Does this potentially take a long time? For example does it involve broadcasting and waiting for a network response? If so we probably want to make it async and display "Loading..." or something in the source until the data is available. This isn't the kind of thing that needs to be fixed before the patch goes in, but a /* FIXME */ would be good. > + headers = g_string_new ("Accept: */*\r\nCache-Control: > no-cache\r\nUser-Agent: iTunes/4.6 (Windows; N)\r Eek, do we really need to say iTunes? I'm going to have a closer look at the RPC code and rhythmdb integration soon, this is kind of a first pass. But generally it looks like you've done a kickass job here integrating with the rest of Rhythmbox. I'm really excited about getting this in.
Attachment:
signature.asc
Description: This is a digitally signed message part