GNOME Bugzilla – Bug 352899
winicon plugin can't create or open windows vista icons
Last modified: 2008-01-15 13:11:00 UTC
Windows vista now use a 256x256 pixels layer in icons that he could scale at any size, for more informations see http://www.rw-designer.com/vista-icon http://www.axialis.com/tutorials/tutorial-vistaicons.html . It also seem that thoses icons could be compressed using PNG format inside the .ico file. I think that GIMP should support import and export of this new format for icons. The actual status is : - GIMP show an error if you try to create an icon with a layer inside that have a width or a height of more than 255x255. - The PNG compression isn't implemented. - The plugin generate an access violation when trying to open an icon that contain a PNG compressed layer.
PS: The access violation is an access to a null pointer (0x00000008)
Created attachment 71619 [details] Icon that produce an access violation when loaded
Fixed the crash in both branches. For now, the large icon is silently ignored: 2006-08-28 Sven Neumann <sven@gimp.org> * plug-ins/winicon/icoload.c: avoid crashing on newer versions of the winicon file format (bug #352899).
Created attachment 73921 [details] [review] Winicon patch This patch introduces some bug fixes, enhancements, and (partial) code cleanup. fixes: * Bug 346016 – file-ico-save ignores run-mode parameter * indexed & grayscale are not saved correctly in 1 or 4 bit formats * preview is displayed in more colors than saved image has (in paletted modes) enhancements: * Bug 352899 – winicon plugin can't create or open windows vista icons * Bug 342885 – Icon - save as 24-bit
It would have been a lot easier if you could have submitted this in several patches. One for the cleanup and bug fixes and one for each of the enhancement requests. This combined patch is a lot more difficult to apply now.
The main problem with the patch is that you cannot rely on the presence of libpng. Build of the PNG plug-in is optional and we would have to compile a winicon plug-in without support the Vista icons then. Can you please try to split this up into at least two patches? It shouldn't be too difficult to make PNG support a compile-time option if we had a patch that did nothing but adding support for Vista icons.
Hm, since GTK+ depends on libpng for a while now, maybe we should make libpng mandatory too. The patch should still be split up though.
Or we could simply not build the winicon plug-in in case someone insists on using --without-png.
That sounds fair enough.
After all that has been said it's not clear to me whether it is still worth splitting the patch into two.
Every comment agrees that the patch should be split in two, regardless of how the png situation is handled.
All done. I've split up all the patches into 3 parts. I submit them all here so that we don't loose context. The first patch is the largest and contains bug fixes and code cleanup. One of the main points of it was to set a more maintainable code structure so that following enhancements could be applied. Some code movement had to occur, however, and much more lines appeared in diff than have been actually changed. Enhancement patches contain mostly additions. They expect the first patch to be applied. Hopefully, they could be applied independently.
Created attachment 74083 [details] [review] Winicon - fixes * Bug 346016 – file-ico-save ignores run-mode parameter * indexed & grayscale are not saved correctly in 1 or 4 bit formats * preview is displayed in more colors than saved image has (in paletted modes)
Created attachment 74085 [details] [review] Winicon - 24bit icon saving Bug 342885 – Icon - save as 24-bit
Created attachment 74086 [details] [review] Winicon - vista Bug 352899 – winicon plugin can't create or open windows vista icons
I've committed the first patch, will look at the others later. 2006-10-06 Sven Neumann <sven@gimp.org> * plug-ins/winicon/icodialog.[ch] * plug-ins/winicon/icoload.[ch] * plug-ins/winicon/icosave.[ch] * plug-ins/winicon/main.[ch]: applied patch from Aurimas Juška with code cleanup and fixes for bug #346016 and other issues (see bug #352899).
Created attachment 74632 [details] [review] 24 bit saving patch with small cleanups This is the 24-bit saving patch that I was about to commit. It's basically the submitted patch plus some minor coding style cleanups. Unfortunately though, the plug-in reproducibly crashes on me in g_free() called from ico_write_icon(). Cause is either a double free or other memory corruption. This will need to be sorted out before this patch can be applied.
Created attachment 74647 [details] [review] 24bit icon fix Fix eliminates problem that Sven has found. Moreover, it fixes wrong handling or rowstride while loading, which was inherited from original code. The patch should be applied after Sven's patch.
2006-10-16 Sven Neumann <sven@gimp.org> * plug-ins/winicon/icodialog.c * plug-ins/winicon/icoload.c * plug-ins/winicon/icosave.c: applied patches from Aurimas Juška that add support for saving 24 bit files and fix a bug in the load routines for this format (bug #352899).
2006-10-16 Sven Neumann <sven@gimp.org> * plug-ins/winicon/Makefile.am * plug-ins/winicon/icodialog.c * plug-ins/winicon/icoload.c * plug-ins/winicon/icosave.c * plug-ins/winicon/main.h: applied patch from Aurimas Juška that adds support for the loading and saving Vista 256x256 PNG Compressed Icons (bug #352899). * configure.in * plug-ins/Makefile.am: don't build the winicon plug-in if PNG support has been explicitely disabled.