Created attachment 68455 [details] [review]
missing-art-icon.patch

Use desaturated version of "gnome-dev-cdrom-audio". Scaled to fit; it's in Tango so there will be a SVG version available.

I was going to suggest this too.

We could also consider adding a "searching" place holder image to display while trying to locate the correct image.

Maybe a generic CD with an ellipsis (...) added.  Or just the same image as for "missing cover"?

However, this would complicate the code which fades between albums if it had to do:

Missing/CoverA --> Searching --> Missing/CoverB

A good idea.

One issue with adding a "searching" image is that the cover db doesn't currently report whether it is searching or not. It wouldn't be a good idea to display for "searching" image all the time, as the art may just be about to get loaded from the cache/on disk.

I guess we could make the db emit a signal when it switching from local to remote searches, which the art widget could listen to.

What about i18n with those images? Should we use cairo/pango to render images instead of using fixed ones? If it's impossible, I'd rather SVG images.

I agree with James (comment 4) that if we did include a "searching for cover" placeholder, it would be best to show it when switching from the fast local search to the slow online search (or slower file search on a remote shared server).

The image Amamok uses for "Missing Cover" is a tilted CD jewel case with a large purple question mark on it.

As Nguyễn pointed out in comment 5 using any text/symbol like "?" for missing or "..." for searching might raise an issue with il8n (internationalization).

For example, maybe in Spanish we should use "¿?" to mean unknown. This example may be wrong, I do know that questions in Spanish are written with an opening upside down question mark, and close with a normal question mark.  It may be that using just ? would be perfectly OK for Spanish.

My suggestion of using an ellipsis (...) for the searching icon is probably a bad idea due to i18n concerns.  I was using it in the sense of "speachless", which while a common usage is not the official meaning of "missing text" in a quotation.

We might be safer using a simple blank CD image in both cases - either just the disc, or a disc in a plain transparent case - like Amamok's without the questionmark?

Created attachment 69431 [details] [review]
art.patch

Fairly comprehensive rewrite.

Creates a custom widget that handles:
- Fading in/out between album covers and "missing" image
- Overlays throbber while searching
- Delayed start to displaying throbber and "missing" image

Oh - it probably won't apply against latest CVS. Meh.

Re comment 4: this patch has a half-second delay before displaying the "searching" image or throbber. Probably better than a signal when changing between local and remote searches; the user doesn't care whether the search is local or remote.

Created attachment 69433 [details]
rhythmbox-missing-artwork.svg

Suggested artwork. (Blank CD in a blank jewel case)

Created attachment 69434 [details]
rhythmbox-missing-artwork.png

200x200 render of missing artwork image

Created attachment 69525 [details] [review]
missing-art-icon.patch

Improve resizing, efficiency.

Created attachment 69529 [details] [review]
missing-art-icon.patch

rm unused code

Created attachment 69536 [details] [review]
missing-art-icon.patch

To CVS. Minor fixes.

When I first started rhythmbox after applying the patch, I got the following:

Traceback (most recent call last):

+ Trace 69603

Still yet to test the actual functionality because my sound card is blocking playing.

News is not good I'm afraid, this patch seems to break cover art for me completely.  Is it using some recent pygtk functionality or expecting certain icon themes?    If there are some other files needed, can you please include them in a self-contained patch?

Something in that cluster of things it is breaking.  I couldn't find any file called "process-working" on my box.

$ rhythmbox

Traceback (most recent call last):

+ Trace 69605

sys:1: Warning: g_date_set_julian: assertion `g_date_valid_julian (j)' failed
sys:1: Warning: g_date_get_year: assertion `g_date_valid (d)' failed
sys:1: Warning: g_date_set_dmy: assertion `g_date_valid_dmy (day, m, y)' failed
sys:1: Warning: g_date_get_julian: assertion `g_date_valid (d)' failed
Traceback (most recent call last):
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 248, in playing_changed
    self.set_entry(sp.get_playing_entry ())
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 254, in set_entry
    if entry != self.current_entry:
AttributeError: 'ArtDisplayPlugin' object has no attribute 'current_entry'
Traceback (most recent call last):
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 248, in playing_changed
    self.set_entry(sp.get_playing_entry ())
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 254, in set_entry
    if entry != self.current_entry:
AttributeError: 'ArtDisplayPlugin' object has no attribute 'current_entry'
Traceback (most recent call last):
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 251, in playing_entry_changed
    self.set_entry(entry)
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 254, in set_entry
    if entry != self.current_entry:
AttributeError: 'ArtDisplayPlugin' object has no attribute 'current_entry'
Traceback (most recent call last):
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 248, in playing_changed
    self.set_entry(sp.get_playing_entry ())
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 254, in set_entry
    if entry != self.current_entry:
AttributeError: 'ArtDisplayPlugin' object has no attribute 'current_entry'
Traceback (most recent call last):
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 248, in playing_changed
    self.set_entry(sp.get_playing_entry ())
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 254, in set_entry
    if entry != self.current_entry:
AttributeError: 'ArtDisplayPlugin' object has no attribute 'current_entry'
Traceback (most recent call last):
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 248, in playing_changed
    self.set_entry(sp.get_playing_entry ())
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 254, in set_entry
    if entry != self.current_entry:
AttributeError: 'ArtDisplayPlugin' object has no attribute 'current_entry'
Traceback (most recent call last):
  File "/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py", line 242, in deactivate
    shell.remove_widget (self.art_widget, rb.SHELL_UI_LOCATION_SIDEBAR)
AttributeError: 'ArtDisplayPlugin' object has no attribute 'art_widget'

"process-working" is the Tango name for the Gnome throbber. I guess I need to change that to the older Gnome name and put some error handling in.

Any updates to the patches pending?

Created attachment 70351 [details] [review]
missing-art-icon.patch

Yeah, this is against current CVS.

Should be a bit more robust (complains if can't load images from theme, instead of crashing).

Created attachment 70353 [details] [review]
missing-art-icon.patch

Oops. This one works.

Where does the plugin expect the rhythmbox-missing-artwork file to be found?

I put it in 

/usr/local/share/rhythmbox/art/rhythmbox-missing-artwork.png

and

/usr/local/share/rhythmbox/art/rhythmbox-missing-artwork.svg

but I keep getting:


/usr/local//lib/rhythmbox/plugins/artdisplay/__init__.py:136: Warning: Missing artwork icon not found: Icon 'rhythmbox-missing-artwork' not present in theme
  warn ("Missing artwork icon not found: %s" % e, Warning)

Can you make a self-contained patch that includes the artwork file and/or adds the appropriate automake stuff so it will install the file in the correct place?

OK, once I installed rhythmbox-missing-artwork.png in /usr/local/share/icons it works nicely.   The throbber appears when artwork is being looked up on the net.

Since all the other rhythmbox-specific art is in /usr/local/share/rhythmbox/art/ perhaps it should also look for the icon there?  In either case, the patch should include the commands necessary to install the file in the appropriate place.

Well, the stuff that's in /usr/local/share/rhythmbox/art/ isn't actually read from there; it's inlined. That wouldn't be appropriate for rhythmbox-missing-artwork.png; it's big, and it's an optional plugin. But it still needs to be themeable...

OK, either way the patch should include the Makefile/automake commands needed to put it in the icondir.  I assume that the png file would be added to CVS in the plugins/artdisplay/artdisplay directory?

Hm... if I install it to $(prefix)/share/rhythmbox/art/, the plugin has no way to find it (because $(prefix) isn't passed to python plugins), and no way to add it to the icon theme (since placing icons directly on the search path is deprecated, and loading it and adding to the theme is inefficient).

I guess that leaves:
a) install it to $(GTK_PREFIX)/share/pixmaps/
b) install it to $(GTK_PREFIX)/share/icons/hicolor/192x192/apps/?

Probably b) is better; I can also install the .svg to $(GTK_PREFIX)/share/icons/hicolor/scalable/apps/. Need to resize it to 192x192, of course.

Oops, collision. Yeah, I guess it wants to go there in the distribution. I'll look at the correct autofoo.

(In reply to comment #24)
> Hm... if I install it to $(prefix)/share/rhythmbox/art/, the plugin has no way
> to find it (because $(prefix) isn't passed to python plugins), and no way to
> add it to the icon theme (since placing icons directly on the search path is
> deprecated, and loading it and adding to the theme is inefficient).
> 
> I guess that leaves:
> a) install it to $(GTK_PREFIX)/share/pixmaps/
> b) install it to $(GTK_PREFIX)/share/icons/hicolor/192x192/apps/?

If I specify /usr/local as my prefix, it should put it /usr/local/share/icons/... not in /usr/share/icons.  Python appeared to find the artwork correctly in the /usr/local/ case.

It should also work if an arbitrary prefix is chosen, e.g. /opt/rhythmbox as a prefix.  What it should never do is put them in /usr/share area unless that prefix is chosen explicitly.

Well, /usr/local/ works because g_get_system_data_dirs defaults to "/usr/local/share/:/usr/share/". I guess any other prefix you're expected to set XDG_DATA_DIRS yourself. Hm... seems other apps that install into $(prefix)/share/icons/... Just Do It. So, we just install the icons into appropriate locations inside $(datadir)/icons/hicolor/ and expect it to work.

Created attachment 70433 [details]
rhythmbox-missing-artwork.png

192x192 version

Created attachment 70434 [details] [review]
missing-art-icon.patch

With Makefile.am patch.

rhythmbox-missing-artwork-{svg,png} go in plugins/artdisplay/artdisplay/.

Works for me.  Works also with prefix=/usr/local/

Ready to go into CVS, I think.

It would be much better to put the art files in plugins/artdisplay/, install them to $plugindir/artdisplay/, and find them with "path = plugin.find_file (filename)". The current version doesn't work for uninstalled builds, installs in a prefix that isn't searched for by GTK, and won't work if install to the usrt-plugin directory.

Created attachment 70788 [details] [review]
missing-art-icon.patch

Uh, like so?

(In reply to comment #32)
> Created an attachment (id=70788) [edit]
> missing-art-icon.patch
> 
> Uh, like so?

Just tested.  Works fine for me. 

Ping?  Can this be committed now that 0.9.6 is out?  This has been working fine for me for a couple of months.

Created attachment 74056 [details] [review]
move svg file

Moves the SVG file to be in plugins/artdisplay/ not plugins/artdisplay/artdisplay/, so that it will be found when running uninstalled.


One small issue I've noticed is that when you first play a track, the widget will appear with the placeholder art and then fade to the real one. Perhaps we should always show the art widget, even when a track isn't playing?

(In reply to comment #35)
> Created an attachment (id=74056) [edit]
> move svg file
> 
> Moves the SVG file to be in plugins/artdisplay/ not
> plugins/artdisplay/artdisplay/, so that it will be found when running
> uninstalled.

Works fine for me.

Any reason why this can't be committed now?

Committed to cvs, thanks.