Re: [Rhythmbox-devel] Patch proposal: Show album cover as iconnext to the song tiltle



Am Sa, den 07.02.2004 schrieb Colin Walters um 20:47:
> On Sat, 2004-02-07 at 10:48, Joergen Scheibengruber wrote:
> 
> > That sounds like a good idea :-) The maybe is probably not necessary, I
> > think having it default to "if available" is fine.
> 
> Ok - actually, having thought about this a bit more, let's just go with
> "No" and "Yes".  The UI resizing caused by "if available" would be
> pretty ugly.

Ok, fine with me :-)

> > Regarding the icon: imho it's ok, but if someone comes up with a nice
> > "no cover available", this would of course be better :-)
> 
> I think we'll really need to do that - I'd like for clicking on the "no
> cover available" icon to download an album cover.

I'll see what I can do. Maybe i find a good icon on jimmacs page or we
could even convince him to draw one.

> if (album_icon->priv->uri)
> 	g_free(album_icon->priv->uri);
> 
> g_free already tests if it's NULL, so you can just say:
> g_free (album_icon->priv->uri);

Ah, how handy :-) I didn't know that....

> Next, you need to hold the GDK lock in all gnome-vfs async callbacks. 
> Basically just run GDK_THREADS_ENTER () when the function starts, and
> GDK_THREADS_LEAVE () before it returns (be sure to handle all return
> paths!).

No problem. I'll fix that ASAP.

> Regarding style, you have spaces between the function name and call in
> some places, but not others.  Also, the indentation around the function
> headers is...odd:
> http://web.verbum.org/files/rb-indentation.png
> I'm guessing your tab width is set to 4 or so?  It should be 8.  

Yep, it's 4. I'll change that. Or should i use spaces instead in those
cases?
Regarding the spaces before the '(': I noticed it, and really tried to
use it, but it's always difficult to use a new style. But I'll try to
fix this, too, of course...

> I also get this assertion on startup:
> 
> (rhythmbox:5969): Gtk-CRITICAL **: file gtkwidget.c: line 1911
> (gtk_widget_hide_all): assertion `GTK_IS_WIDGET (widget)' failed

Hmm, I don't get this, but I'll try to figure out what could be causing
it...

> I'm uneasy about doing this in a mouseover - it's a cool hack, but it's
> a little intrusive, especially because it's near the playback control
> buttons.  Maybe changing the timeout to a second would be enough.  Or we
> could just make clicking on it open it in an image viewer.  Also, you
> need to hold the GDK lock in those idle handlers too.

Yeah, I can understand that. I was a little uncertain about the whole
thing, too. But I simply felt that you really don't see much on 48x48
pixels. On the other hand, the most important thing for me is
recognizing to album quickly, so if there are strong objections I could
remove this again (or we could make it a gconf-preference).

Regards,
Jörgen




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]