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