After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699252 - fails to generate thumbnails for xpm files with comments in header
fails to generate thumbnails for xpm files with comments in header
Status: RESOLVED FIXED
Product: gnome-desktop
Classification: Core
Component: libgnome-desktop
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Desktop Maintainers
Desktop Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-04-29 18:42 UTC by Rodrigo Silva
Modified: 2013-09-20 10:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test file that fails (4.90 KB, image/x-xpixmap)
2013-04-29 18:42 UTC, Rodrigo Silva
  Details
gs_file_read_noatime: Always return errors (1009 bytes, patch)
2013-08-24 03:39 UTC, Bastien Nocera
committed Details | Review
thumbnailer: Always print errors from gs_file_read_noatime() (1.19 KB, patch)
2013-08-24 03:41 UTC, Bastien Nocera
committed Details | Review
thumbnailer: Correctly check for errors (1.07 KB, patch)
2013-08-24 03:41 UTC, Bastien Nocera
committed Details | Review
thumbnailer: Check gdk_pixbuf_loader_close()'s retval (1019 bytes, patch)
2013-08-24 03:42 UTC, Bastien Nocera
committed Details | Review

Description Rodrigo Silva 2013-04-29 18:42:04 UTC
Created attachment 242833 [details]
Test file that fails

If a .xpm file has a (quite unconventional, I admit) header in this form:

// This file is part of BOINC.
// http://boinc.berkeley.edu
//

/* XPM */
static const char *atiicon_xpm[] = {
...

libgnome-desktop fails to generate a thumbnail preview for it.

First I thought this could be a CRLF issue, or perhaps the '//'-style comments instead of '/* */', but even changing both, thumbnails still failed to generate.

I also assumed such files were invalid, but EOG, the default image viewer in Gnome, displays them fine. And both eog and libgnome-desktop uses GdkPixbuf, so it is not a bug in GdkPixbuf either.

Maybe eog is pre-processing the file, or using GdkPixbuf in a different way that allows such files to render? Can libgnome-desktop do the same?

Examples of such XPM files are found at http://boinc.berkeley.edu/trac/export/HEAD/boinc-v2/clientgui/res/
Comment 1 Bastien Nocera 2013-08-24 03:39:47 UTC
Created attachment 252966 [details] [review]
gs_file_read_noatime: Always return errors

When passed an invalid GFile, we should error out properly
instead of not returning an error.
Comment 2 Bastien Nocera 2013-08-24 03:41:22 UTC
Created attachment 252967 [details] [review]
thumbnailer: Always print errors from gs_file_read_noatime()
Comment 3 Bastien Nocera 2013-08-24 03:41:53 UTC
Created attachment 252968 [details] [review]
thumbnailer: Correctly check for errors

By checking the retval, not whether error was set.
Comment 4 Bastien Nocera 2013-08-24 03:42:10 UTC
Created attachment 252969 [details] [review]
thumbnailer: Check gdk_pixbuf_loader_close()'s retval
Comment 5 Bastien Nocera 2013-08-24 03:45:05 UTC
A few bug fixes above, but it seems to get thumbnailed fine:
./test-desktop-thumbnail file:///home/hadess/atiicon.xpm
shows a nice thumbnail in a separate window.

Did you pass a filepath instead of a URI?

Marking as NEW for the bug fixes.
Comment 6 Colin Walters 2013-08-24 07:49:25 UTC
Review of attachment 252966 [details] [review]:

It's not invalid, it's just non-local.  Besides that minor quibble with the commit message, looks fine to me!
Comment 7 Colin Walters 2013-08-24 07:49:59 UTC
Review of attachment 252967 [details] [review]:

Looks fine.
Comment 8 Colin Walters 2013-08-24 07:50:44 UTC
Review of attachment 252968 [details] [review]:

Checking that GError was set is valid, but this code is definitely cleaner.
Comment 9 Colin Walters 2013-08-24 07:53:22 UTC
Review of attachment 252969 [details] [review]:

Sure; the == FALSE looks odd to me...but I guess gnome-pnp-ids.c does it too.
Comment 10 Rodrigo Silva 2013-08-24 08:17:38 UTC
@Bastien: I didn't try to thumbnail it directly, I let Nautilus do so, and tested by deleting ~/.thumbnails and navigating back to the folder containing the xpm files.

That directory is local, so perhaps Nautilus is passing a filepath to thumbnailer instead of an URI?

A few helpful notes:

- The xpm test files were fixed by upstream boinc, so disregard the examples URL in my first post and use only the attachment.

- When I opened the attachment myself (Using Firefox's "Open With"), Nautilus DID generate a thumbnail for the automatically downloaded file at /tmp. But if instead I select "Save File", or copy the file somewhere else, it doesn't.

- Using Ubuntu 12.04 64 bit, Firefox 23, Nautilus 3.4.2
Comment 11 Bastien Nocera 2013-08-24 19:48:11 UTC
(In reply to comment #6)
> Review of attachment 252966 [details] [review]:
> 
> It's not invalid, it's just non-local.  Besides that minor quibble with the
> commit message, looks fine to me!

It's either invalid (try passing a local path instead of a URI to test-desktop-thumbnail) meaning that somebody passed garbage to g_file_new_for_uri(), or isn't backed by a fuse mount or native.

(In reply to comment #10)
> @Bastien: I didn't try to thumbnail it directly, I let Nautilus do so, and
> tested by deleting ~/.thumbnails and navigating back to the folder containing
> the xpm files.
> 
> That directory is local, so perhaps Nautilus is passing a filepath to
> thumbnailer instead of an URI?
> 
> A few helpful notes:
> 
> - The xpm test files were fixed by upstream boinc, so disregard the examples
> URL in my first post and use only the attachment.
> 
> - When I opened the attachment myself (Using Firefox's "Open With"), Nautilus
> DID generate a thumbnail for the automatically downloaded file at /tmp. But if
> instead I select "Save File", or copy the file somewhere else, it doesn't.
> 
> - Using Ubuntu 12.04 64 bit, Firefox 23, Nautilus 3.4.2

It might be a bug in nautilus, I have no idea what it uses to do the thumbnailing in those cases (IIRC, it will handle the fallbacks to gdk-pixbuf itself, rather than calling out to gnome-desktop, which makes sense as it would be faster).
Comment 12 Bastien Nocera 2013-09-05 16:48:12 UTC
Comment on attachment 252966 [details] [review]
gs_file_read_noatime: Always return errors

Pushed with a better commit message.

Attachment 252966 [details] pushed as 870b672 - gs_file_read_noatime: Always return errors
Comment 13 Bastien Nocera 2013-09-05 16:49:44 UTC
All pushed out. Further bugs in thumbnailing with nautilus should be filed as a nautilus bug. nautilus doesn't use libgnome-desktop's thumbnailer when thumbnailing known image types, so it's unlikely that the bug lies here.

Attachment 252967 [details] pushed as d045e36 - thumbnailer: Always print errors from gs_file_read_noatime()
Attachment 252968 [details] pushed as fef0630 - thumbnailer: Correctly check for errors
Attachment 252969 [details] pushed as 1e28ad9 - thumbnailer: Check gdk_pixbuf_loader_close()'s retval
Comment 14 André Klapper 2013-09-20 10:41:06 UTC
hadess: This seems to have created a crasher regression: see bug 708435, bug 708417.