Merge pull request #439 from mono-soc-2012/garyb/iconfix
authorJérémie Laval <jeremie.laval@gmail.com>
Mon, 10 Sep 2012 12:28:33 +0000 (05:28 -0700)
committerJérémie Laval <jeremie.laval@gmail.com>
Mon, 10 Sep 2012 12:28:33 +0000 (05:28 -0700)
Fix for corruption of vista icons on save

mcs/class/System.Drawing/System.Drawing/Icon.cs
mcs/class/System.Drawing/Test/System.Drawing/TestIcon.cs
mcs/class/System.Drawing/Test/System.Drawing/bitmaps/only256.ico [new file with mode: 0644]

index 2638031963a4bf6716332ac6a5ab92a0e3e896b5..7c07c2bf6c0eaf7ab20cb1132cac726b60418c17 100644 (file)
@@ -2,6 +2,7 @@
 // System.Drawing.Icon.cs
 //
 // Authors:
+//   Gary Barnett (gary.barnett.mono@gmail.com)
 //   Dennis Hayes (dennish@Raytek.com)
 //   Andreas Nahr (ClassDevelopment@A-SoftTech.com)
 //   Sanjay Gupta (gsanjay@novell.com)
@@ -53,7 +54,7 @@ namespace System.Drawing
        public sealed class Icon : MarshalByRefObject, ISerializable, ICloneable, IDisposable
        {
                [StructLayout(LayoutKind.Sequential)]
-               internal struct IconDirEntry {
+               internal struct IconDirEntry {          
                        internal byte   width;          // Width of icon
                        internal byte   height;         // Height of icon
                        internal byte   colorCount;     // colors in icon 
@@ -62,6 +63,7 @@ namespace System.Drawing
                        internal ushort bitCount;       // Bits per pixel
                        internal uint   bytesInRes;     // bytes in resource
                        internal uint   imageOffset;    // position in file 
+                       internal bool   ignore;         // for unsupported images (vista 256 png)
                }; 
 
                [StructLayout(LayoutKind.Sequential)]
@@ -87,19 +89,28 @@ namespace System.Drawing
                        internal uint   biClrImportant; 
                };
 
+               [StructLayout(LayoutKind.Sequential)]   // added baseclass for non bmp image format support
+               internal abstract class ImageData {
+               };
+
                [StructLayout(LayoutKind.Sequential)]
-               internal struct IconImage {
+               internal class IconImage : ImageData {
                        internal BitmapInfoHeader       iconHeader;     //image header
                        internal uint []                iconColors;     //colors table
                        internal byte []                iconXOR;        // bits for XOR mask
                        internal byte []                iconAND;        //bits for AND mask
-               };      
+               };
+
+               [StructLayout(LayoutKind.Sequential)]
+               internal class IconDump : ImageData {
+                       internal byte []                data;
+               };
 
                private Size iconSize;
                private IntPtr handle = IntPtr.Zero;
                private IconDir iconDir;
                private ushort id;
-               private IconImage [] imageData;
+               private ImageData [] imageData;
                private bool undisposable;
                private bool disposed;
                private Bitmap bitmap;
@@ -152,28 +163,43 @@ namespace System.Drawing
 
                                for (ushort i=0; i < count; i++) {
                                        IconDirEntry ide = iconDir.idEntries [i];
-                                       if ((ide.height == size.Height) || (ide.width == size.Width)) {
+                                       if (((ide.height == size.Height) || (ide.width == size.Width)) && !ide.ignore) {
                                                id = i;
                                                break;
                                        }
                                }
 
                                // if a perfect match isn't found we look for the biggest icon *smaller* than specified
-                               if (id == UInt16.MaxValue) {
+                               if (id == UInt16.MaxValue) { 
                                        int requested = Math.Min (size.Height, size.Width);
-                                       IconDirEntry best = iconDir.idEntries [0];
-                                       for (ushort i=1; i < count; i++) {
+                                       // previously best set to 1st image, as this might not be smallest changed loop to check all
+                                       IconDirEntry? best = null; 
+                                       for (ushort i=0; i < count; i++) {
                                                IconDirEntry ide = iconDir.idEntries [i];
-                                               if ((ide.height < requested) || (ide.width < requested)) {
-                                                       if ((ide.height > best.height) || (ide.width > best.width))
+                                               if (((ide.height < requested) || (ide.width < requested)) && !ide.ignore) {
+                                                       if (best == null) {
+                                                               best = ide;
+                                                               id = i;
+                                                       } else if ((ide.height > best.Value.height) || (ide.width > best.Value.width)) {
+                                                               best = ide;
                                                                id = i;
+                                                       }
                                                }
                                        }
                                }
 
                                // last one, if nothing better can be found
+                               if (id == UInt16.MaxValue) {
+                                       int i = count;
+                                       while (id == UInt16.MaxValue && i > 0) {
+                                               i--;
+                                               if (!iconDir.idEntries [i].ignore)
+                                                       id = (ushort) i;
+                                       }
+                               }
+
                                if (id == UInt16.MaxValue)
-                                       id = (ushort) (count - 1);
+                                       throw new ArgumentException ("Icon", "No valid icon image found");
 
                                iconSize.Height = iconDir.idEntries [id].height;
                                iconSize.Width = iconDir.idEntries [id].width;
@@ -353,6 +379,11 @@ namespace System.Drawing
                        writer.Write (ii.iconAND);
                }
 
+               private void SaveIconDump (BinaryWriter writer, IconDump id)
+               {
+                       writer.Write (id.data);
+               }
+
                private void SaveIconDirEntry (BinaryWriter writer, IconDirEntry ide, uint offset)
                {
                        writer.Write (ide.width);
@@ -377,10 +408,20 @@ namespace System.Drawing
                        }
 
                        for (int i=0; i < (int)count; i++) {
-                               SaveIconImage (writer, imageData [i]);
+
+                               //FIXME: HACK: 1 (out of the 8) vista type icons had additional bytes (value:0)
+                               //between images. This fixes the issue, but perhaps shouldnt include in production?
+                               while (writer.BaseStream.Length < iconDir.idEntries[i].imageOffset)
+                                       writer.Write ((byte) 0);
+
+                               if (imageData [i] is IconDump)
+                                       SaveIconDump (writer, (IconDump) imageData [i]);
+                               else
+                                       SaveIconImage (writer, (IconImage) imageData [i]);
                        }
                }
-
+               // TODO: check image not ignored (presently this method doesnt seem to be called unless width/height 
+               // refer to image)
                private void SaveBestSingleIcon (BinaryWriter writer, int width, int height)
                {
                        writer.Write (iconDir.idReserved);
@@ -401,7 +442,7 @@ namespace System.Drawing
                        }
 
                        SaveIconDirEntry (writer, iconDir.idEntries [best], 22);
-                       SaveIconImage (writer, imageData [best]);
+                       SaveIconImage (writer, (IconImage) imageData [best]);
                }
 
                private void SaveBitmapAsIcon (BinaryWriter writer)
@@ -490,7 +531,7 @@ namespace System.Drawing
                        if (imageData == null)
                                return new Bitmap (32, 32);
 
-                       IconImage ii = imageData [id];
+                       IconImage ii = (IconImage) imageData [id];
                        BitmapInfoHeader bih = ii.iconHeader;
                        int biHeight = bih.biHeight / 2;
 
@@ -574,7 +615,6 @@ namespace System.Drawing
                                        bitmap = BuildBitmapOnWin32 ();
                                }
                        }
-
                        return bitmap;
                }
 
@@ -661,7 +701,9 @@ namespace System.Drawing
                                throw new System.ArgumentException ("Invalid Argument", "stream");
 
                        ushort dirEntryCount = reader.ReadUInt16();
-                       ArrayList entries = new ArrayList (dirEntryCount);
+                       imageData = new ImageData [dirEntryCount]; 
+                       iconDir.idCount = dirEntryCount; 
+                       iconDir.idEntries = new IconDirEntry [dirEntryCount];
                        bool sizeObtained = false;
                        // now read in the IconDirEntry structures
                        for (int i = 0; i < dirEntryCount; i++) {
@@ -685,18 +727,21 @@ Console.WriteLine ("\tide.bitCount: {0}", ide.bitCount);
 Console.WriteLine ("\tide.bytesInRes: {0}", ide.bytesInRes);
 Console.WriteLine ("\tide.imageOffset: {0}", ide.imageOffset);
 #endif
-
+                               // Vista 256x256 icons points directly to a PNG bitmap
                                // 256x256 icons are decoded as 0x0 (width and height are encoded as BYTE)
-                               // and we ignore them just like MS does (at least up to fx 2.0)
+                               // and we ignore them just like MS does (at least up to fx 2.0) 
+                               // Added: storing data so it can be saved back
                                if ((ide.width == 0) && (ide.height == 0))
-                                       continue;
+                                       ide.ignore = true;
+                               else
+                                       ide.ignore = false;
 
-                               int index = entries.Add (ide);
+                               iconDir.idEntries [i] = ide;
 
                                //is this is the best fit??
                                if (!sizeObtained) {
-                                       if ((ide.height == height) || (ide.width == width)) {
-                                               this.id = (ushort) index;
+                                       if (((ide.height == height) || (ide.width == width)) && !ide.ignore) {
+                                               this.id = (ushort) i;
                                                sizeObtained = true;
                                                this.iconSize.Height = ide.height;
                                                this.iconSize.Width = ide.width;
@@ -704,22 +749,22 @@ Console.WriteLine ("\tide.imageOffset: {0}", ide.imageOffset);
                                }
                        }
 
-                       // Vista 256x256 icons points directly to a PNG bitmap
-                       dirEntryCount = (ushort) entries.Count;
-                       if (dirEntryCount == 0)
-                               throw new Win32Exception (0, "No valid icon entry were found.");
+                       // throw error if no valid entries found
+                       int valid = 0;
+                       for (int i = 0; i < dirEntryCount; i++) {
+                               if (!(iconDir.idEntries [i].ignore))
+                                       valid++;
+                       }
 
-                       iconDir.idCount = dirEntryCount;
-                       imageData = new IconImage [dirEntryCount];
-                       iconDir.idEntries = new IconDirEntry [dirEntryCount];
-                       entries.CopyTo (iconDir.idEntries);
+                       if (valid == 0) 
+                               throw new Win32Exception (0, "No valid icon entry were found.");
 
-                       //if we havent found the best match, return the one with the
-                       //largest size. Is this approach correct??
+                       // if we havent found the best match, return the one with the
+                       // largest size. Is this approach correct??
                        if (!sizeObtained){
                                uint largestSize = 0;
-                               for (int j=0; j<dirEntryCount; j++){
-                                       if (iconDir.idEntries [j].bytesInRes >= largestSize   {
+                               for (int j=0; j<dirEntryCount; j++){ 
+                                       if (iconDir.idEntries [j].bytesInRes >= largestSize && !iconDir.idEntries [j].ignore)   {
                                                largestSize = iconDir.idEntries [j].bytesInRes;
                                                this.id = (ushort) j;
                                                this.iconSize.Height = iconDir.idEntries [j].height;
@@ -729,8 +774,18 @@ Console.WriteLine ("\tide.imageOffset: {0}", ide.imageOffset);
                        }
                        
                        //now read in the icon data
-                       for (int j = 0; j<dirEntryCount; j++)
+                       for (int j = 0; j<dirEntryCount; j++) 
                        {
+                               // process ignored into IconDump
+                               if (iconDir.idEntries [j].ignore) {
+                                       IconDump id = new IconDump ();
+                                       stream.Seek (iconDir.idEntries [j].imageOffset, SeekOrigin.Begin);
+                                       id.data = new byte [iconDir.idEntries [j].bytesInRes];
+                                       stream.Read (id.data, 0, id.data.Length);
+                                       imageData [j] = id;
+                                       continue;
+                               }
+                               // standard image
                                IconImage iidata = new IconImage();
                                BitmapInfoHeader bih = new BitmapInfoHeader();
                                stream.Seek (iconDir.idEntries [j].imageOffset, SeekOrigin.Begin);
index 5c8f3097c01451b7cfe3f6ced5e06d36445cc17f..36568741c3ffaf922423b1279f01a5e57d230522 100644 (file)
@@ -2,6 +2,7 @@
 // Icon class testing unit
 //
 // Authors:
+//     Gary Barnett <gary.barnett.mono@gmail.com>
 //     Sanjay Gupta <gsanjay@novell.com>
 //     Sebastien Pouliot  <sebastien@ximian.com>
 //
@@ -144,6 +145,48 @@ namespace MonoTests.System.Drawing {
                        Assert.AreEqual (32, non_square.Width, "Width");
                }
 
+               [Test]
+               public void Constructor_Icon_GetNormalSizeFromIconWith256 ()
+               {
+                       string filepath = TestBitmap.getInFile ("bitmaps/323511.ico");
+
+                       Icon orig = new Icon (filepath);
+                       Assert.AreEqual (32,orig.Height);
+                       Assert.AreEqual (32,orig.Width);
+
+                       Icon ret = new Icon (orig, 48, 48);
+                       Assert.AreEqual (48, ret.Height);
+                       Assert.AreEqual (48, ret.Width);
+               }
+
+               [Test]
+               public void Constructor_Icon_DoesntReturn256Passing0 ()
+               {
+                       string filepath = TestBitmap.getInFile ("bitmaps/323511.ico");
+                       
+                       Icon orig = new Icon (filepath);
+                       Assert.AreEqual (32,orig.Height);
+                       Assert.AreEqual (32,orig.Width);
+                       
+                       Icon ret = new Icon (orig, 0, 0);
+                       Assert.AreNotEqual (0, ret.Height);
+                       Assert.AreNotEqual (0, ret.Width);
+               }
+
+               [Test]
+               public void Constructor_Icon_DoesntReturn256Passing1 ()
+               {
+                       string filepath = TestBitmap.getInFile ("bitmaps/323511.ico");
+                       
+                       Icon orig = new Icon (filepath);
+                       Assert.AreEqual (32,orig.Height);
+                       Assert.AreEqual (32,orig.Width);
+                       
+                       Icon ret = new Icon (orig, 1, 1);
+                       Assert.AreNotEqual (0, ret.Height);
+                       Assert.AreNotEqual (0, ret.Width);
+               }
+
                [Test]
                [ExpectedException (typeof (ArgumentException))]
                public void Constructor_StreamNull ()
@@ -361,22 +404,19 @@ namespace MonoTests.System.Drawing {
                [Test] // bug #410608
                public void Save_256 ()
                {
-                       if (RunningOnUnix)
-                               Assert.Ignore ("Depends on bug #323511");
-
-                       using (Icon icon = new Icon (TestBitmap.getInFile ("bitmaps/323511.ico"))) {
-                               // FIXME: use this instead after bug #415809 is fixed
-                               //SaveAndCompare ("256", icon, true);
+                       string filepath = TestBitmap.getInFile ("bitmaps/323511.ico");
 
-                               MemoryStream ms = new MemoryStream ();
-                               icon.Save (ms);
-                               ms.Position = 0;
-
-                               using (Icon loaded = new Icon (ms)) {
-                                       Assert.AreEqual (icon.Height, loaded.Height, "Loaded.Height");
-                                       Assert.AreEqual (icon.Width, loaded.Width, "Loaded.Width");
-                               }
+                       using (Icon icon = new Icon (filepath)) {
+                               // bug #415809 fixed
+                               SaveAndCompare ("256", icon, true);
                        }
+
+                       // binary comparison
+                       var orig = new MemoryStream (File.ReadAllBytes (filepath));
+                       var saved = new MemoryStream ();
+                       using (Icon icon = new Icon (filepath))
+                               icon.Save (saved);
+                       FileAssert.AreEqual (orig, saved, "binary comparison");
                }
 
                [Test]
@@ -480,9 +520,6 @@ namespace MonoTests.System.Drawing {
 #endif
                public void Icon256ToBitmap ()
                {
-                       if (RunningOnUnix)
-                               Assert.Ignore ("Depends on bug #323511");
-
                        using (FileStream fs = File.OpenRead (TestBitmap.getInFile ("bitmaps/415581.ico"))) {
                                Icon icon = new Icon (fs, 48, 48);
                                using (Bitmap b = icon.ToBitmap ()) {
@@ -507,6 +544,31 @@ namespace MonoTests.System.Drawing {
                        }
                }
 
+               [Test]
+               public void Icon256ToBitmap_Request0 ()
+               {
+                       // 415581.ico has 2 images, the 256 and 48
+                       using (FileStream fs = File.OpenRead (TestBitmap.getInFile ("bitmaps/415581.ico"))) {
+                               Icon icon = new Icon (fs, 0, 0);
+                               using (Bitmap b = icon.ToBitmap ()) {
+                                       Assert.AreEqual (0, b.Palette.Entries.Length, "#B1");
+                                       Assert.AreEqual (48, b.Height, "#B2");
+                                       Assert.AreEqual (48, b.Width, "#B3");
+                                       Assert.IsTrue (b.RawFormat.Equals (ImageFormat.MemoryBmp), "#B4");
+                                       Assert.AreEqual (2, b.Flags, "#B5");
+                               }
+                       }
+               }
+
+               [Test, ExpectedException ()] //ToDo: System.ComponentModel.Win32Exception
+               public void Only256InFile ()
+               {
+                       using (FileStream fs = File.OpenRead (TestBitmap.getInFile ("bitmaps/only256.ico"))) {
+                               Icon icon = new Icon (fs, 0, 0);
+                       }
+               }
+
+
 #if NET_2_0
                [Test]
                [ExpectedException (typeof (ArgumentException))]
diff --git a/mcs/class/System.Drawing/Test/System.Drawing/bitmaps/only256.ico b/mcs/class/System.Drawing/Test/System.Drawing/bitmaps/only256.ico
new file mode 100644 (file)
index 0000000..d6fdd64
Binary files /dev/null and b/mcs/class/System.Drawing/Test/System.Drawing/bitmaps/only256.ico differ