GNOME Bugzilla – Bug 395738
support for OS X icons (.icns files)
Last modified: 2010-07-10 04:07:47 UTC
A simple loader for OS X icons is available at http://ezix.org/project/wiki/MacOSXIcons It can read all formats of 32-bit icons. Write support is planned.
Please attach the sources of your loader as a patch to gtk+.
Created attachment 80263 [details] [review] Patch for GTK+ 2.10.7
Can you provide a few test image to use with this code ?
Just a few easily downloadable files (I just searched for icns and cvs): http://dev.w3.org/cvsweb/~checkout~/Amaya/resources/bundle/amaya.icns?rev=1.2&content-type=text/plain http://bioinformatics.org/cgi-bin/cvsweb.cgi/~checkout~BioCocoa/BC.icns?rev=1.3&content-type=text/plain http://tktoolkit.cvs.sourceforge.net/*checkout*/tktoolkit/tk/macosx/Wish.icns?revision=1.2 http://vc.thauvin.net/*checkout*/cvs/mac/SherlockRSS/Channel.icns?rev=1.1
Some comments from just looking over the patch (sorry these are not in order, I've read the patch bottom-up...) + if (error != NULL) + *error = NULL; No need to do that, callers are required to pass a cleared GError* +static void +pixbuf_destroy_function (IN guchar * pixels, gpointer data) +{ + if (pixels) + free (pixels); +} No need for the NULL check here either - you know you passed non-NULL data when you constructed the pixbuf ! +_error: /* wipe the channel before bailing out */ + for (i = 0; i < size * size; i++) + wipeit[i * 4] = 0; We don't normally do this kind of cleanup in the error paths in loaders. It is just unnecessary extra work, imo. uncompress assumes that source has size * size bytes, but the 3 consecutive calls of uncompress are moving the source pointer forward, so that is not true for the subsequent calls. uncompress() should take an int *remaining and update it, I'd say. while (current - bytes < icnslen) + { + header = (IcnsBlockHeader *) current; + blocklen = GUINT32_FROM_BE (header->size); This probably needs to check that icnslen - (current - bytes) >= sizeof (IcnsBlockBeader) before accessing blocklen. Also, the code in the switch needs to check that blocklen is not a fantasy number going beyond the end of the data. There is no need to implement incremental callbacks when all you are doing is piling up the data for the acutal work in stop_load. I would prefer actual incremental loading, but if it is not there, just implement load().
Created attachment 82738 [details] Sample .icns file Sample .icns file containing all usual sizes and colours
Created attachment 94260 [details] [review] gdk-pixbuf-icns-loader.patch Updated patch. - Against SVN trunk - Don't use incremental load if we don't support it - Load the biggest size possible - Simplify some code - Add support for late-Tiger .icns files (256x256 icons, most can't be loaded because we don't have a JPEG2000 loader, see bug 469901)
(In reply to comment #5) > Some comments from just looking over the patch (sorry these are not in > order, I've read the patch bottom-up...) > > > + if (error != NULL) > + *error = NULL; > > No need to do that, callers are required to pass a cleared GError* Fixed. > +static void > +pixbuf_destroy_function (IN guchar * pixels, gpointer data) > +{ > + if (pixels) > + free (pixels); > +} > > No need for the NULL check here either - you know you passed non-NULL > data when you constructed the pixbuf ! Fixed. > +_error: /* wipe the channel before bailing out */ > + for (i = 0; i < size * size; i++) > + wipeit[i * 4] = 0; > > We don't normally do this kind of cleanup in the error paths in loaders. > It is just unnecessary extra work, imo. Fixed as well. > uncompress assumes that source has size * size bytes, but the 3 > consecutive calls of uncompress are moving the source pointer forward, > so that is not true for the subsequent calls. uncompress() should > take an int *remaining and update it, I'd say. > > > while (current - bytes < icnslen) > + { > + header = (IcnsBlockHeader *) current; > + blocklen = GUINT32_FROM_BE (header->size); > > This probably needs to check that > icnslen - (current - bytes) >= sizeof (IcnsBlockBeader) > before accessing blocklen. Also, the code in the switch needs > to check that blocklen is not a fantasy number going beyond the end of > the data. Fixed. > There is no need to implement incremental callbacks when all you are doing is > piling up the data for the acutal work in stop_load. I would prefer actual > incremental loading, but if it is not there, just implement load(). I did this as well, although it seems to break a lot of applications that just use the incremental loading and never fallback to the non-incremental one. You can find a 256x256 icns icon at: http://svn.igniterealtime.org/svn/repos/spark/trunk/src/resources/images/message.icns http://publicsvn.songbirdnest.com/browser/trunk/app/branding/songbird.icns?format=raw
Note that there's 512x512 support in Leopard, but I couldn't find any icons or docs about it. The only thing missing is knowing the resource name (like "ic08" is for 256x256 icons).
Looks reasonable to me and does survive some fuzz testing with pixbuf-randomly-modified. The one thing you should correct is using g_try_malloc0 for the pixbuf data and return NULL if that fails. pixbuf loaders are supposed to handle this kind of oom failure. Feel free to commit to trunk with that fix.
Done the fix, and committed to trunk. Thanks! 2007-11-20 Bastien Nocera <hadess@hadess.net> * configure.in: add support for conditional icns gdk-pixbuf loader (Closes: #395738) 2007-11-20 Bastien Nocera <hadess@hadess.net> * Makefile.am: * io-icns.c: Add icns (MacOS X icons) loader, based on work by Lyonel Vincent <lyonel@ezix.org> (Closes: #395738) 2007-11-20 Bastien Nocera <hadess@hadess.net> * POTFILES.in: Add icns loader to the files to translate