Refactor FolderIconService and IconExtractor to streamline icon extraction logic. Update documentation to clarify index handling for resource IDs and ordinal indices. Enhance tests to verify extraction functionality for negative indices and ensure consistency between GetIcon and SaveIcon methods.

Author: guwidoe

src/FolderIconManager.Core/Services/FolderIconService.cs

@@ -326,26 +326,23 @@ public void ExtractIcon(IconResource resource, string outputPath)
     }
 
     /// <summary>
-    /// Extracts an icon from a fully resolved file path
+    /// Extracts an icon from a fully resolved file path.
+    /// 
+    /// Index interpretation (Windows desktop.ini convention):
+    /// - Negative values (e.g., -183) = resource ID (extracts icon with resource ID 183)
+    /// - Positive values (e.g., 5) = ordinal index (extracts the 6th icon)
+    /// 
+    /// PrivateExtractIcons handles both conventions natively.
     /// </summary>
     public void ExtractIconFromPath(string sourcePath, int index, string outputPath)
     {
-        // Handle negative indices (resource IDs) - convert to zero-based ordinal
-        // For now, we treat negative as absolute value for ordinal lookup
-        if (index < 0)
-            index = Math.Abs(index);
-
         using var extractor = new IconExtractor(sourcePath);
 
         if (extractor.IconCount == 0)
             throw new InvalidOperationException($"No icons found in {sourcePath}");
 
-        if (index >= extractor.IconCount)
-        {
-            _log.Warning($"Icon index {index} out of range (max {extractor.IconCount - 1}), using index 0");
-            index = 0;
-        }
-
+        // Pass index directly - PrivateExtractIcons handles both positive (ordinal)
+        // and negative (resource ID) indices natively
         extractor.SaveIcon(index, outputPath);
     }
 

src/FolderIconManager.Core/Services/IconExtractor.cs

@@ -74,52 +74,17 @@ public IconExtractor(string filePath)
     public bool HasResourceId(int resourceId) => _iconIds.Contains(resourceId);
 
     /// <summary>
-    /// Resolves an index to the actual resource ID to use.
-    /// 
-    /// Windows desktop.ini convention:
-    /// - Positive numbers (e.g., 266) = ordinal index (0-based position in icon list)
-    /// - Negative numbers (e.g., -266) = direct resource ID (abs value)
+    /// Checks if an index refers to a resource ID (negative) or ordinal (positive).
+    /// This is for informational purposes - PrivateExtractIcons handles both natively.
     /// </summary>
-    private int ResolveToResourceId(int index)
-    {
-        // Negative numbers are direct resource IDs (Windows convention)
-        if (index < 0)
-        {
-            var resourceId = Math.Abs(index);
-            // If this resource ID exists, use it
-            if (_iconIds.Contains(resourceId))
-            {
-                return resourceId;
-            }
-            // If not found, log warning but still try to use it
-            // (in case enumeration missed it)
-            return resourceId;
-        }
-
-        // Positive numbers are ordinal indices (0-based)
-        if (index >= 0 && index < _iconIds.Count)
-        {
-            return _iconIds[index];
-        }
-
-        // Out of range - fall back to first icon
-        if (_iconIds.Count > 0)
-        {
-            return _iconIds[0];
-        }
-
-        throw new ArgumentOutOfRangeException(nameof(index), 
-            $"No icon found for index {index}. File has {_iconIds.Count} icons.");
-    }
+    public bool IsResourceIdIndex(int index) => index < 0;
 
     /// <summary>
     /// Extracts an icon and saves it to a file with all resolutions.
     /// 
-    /// Index interpretation (Windows convention):
-    /// - Positive: ordinal index (e.g., 266 = the 267th icon)
-    /// - Negative: resource ID (e.g., -266 = resource with ID 266)
-    /// 
-    /// Uses PrivateExtractIcons which matches Windows Shell behavior exactly.
+    /// Index interpretation (Windows desktop.ini convention, handled natively by PrivateExtractIcons):
+    /// - Positive: ordinal index (e.g., 5 = the 6th icon)
+    /// - Negative: resource ID (e.g., -183 = icon with resource ID 183)
     /// </summary>
     public void SaveIcon(int index, string outputPath)
     {
@@ -130,8 +95,7 @@ public void SaveIcon(int index, string outputPath)
             return;
         }
 
-        // Use PrivateExtractIcons to match Windows Shell behavior exactly
-        // This extracts at the same index that ExtractIconEx and Windows Shell use
+        // PrivateExtractIcons handles both positive (ordinal) and negative (resource ID) indices natively
         SaveIconUsingPrivateExtractIcons(index, outputPath);
     }
 
@@ -489,7 +453,10 @@ private struct BITMAPINFOHEADER
 
     /// <summary>
     /// Gets an Icon object for preview purposes.
-    /// Uses PrivateExtractIcons to match Windows Shell behavior exactly.
+    /// 
+    /// Index interpretation (Windows desktop.ini convention, handled natively by PrivateExtractIcons):
+    /// - Positive: ordinal index (e.g., 5 = the 6th icon)
+    /// - Negative: resource ID (e.g., -183 = icon with resource ID 183)
     /// </summary>
     public Icon? GetIcon(int index, int size = 32)
     {
@@ -506,7 +473,7 @@ private struct BITMAPINFOHEADER
             }
         }
 
-        // Use PrivateExtractIcons to get the same icon that Windows Shell uses
+        // PrivateExtractIcons handles both positive (ordinal) and negative (resource ID) indices natively
         var icons = new IntPtr[1];
         var ids = new uint[1];
 

tests/FolderIconManager.Tests/ExtractionTests.cs

@@ -173,6 +173,138 @@ public void IconExtractor_NegativeIndex_TreatedAsResourceId()
         Assert.True(fileInfo.Length > 1000, "Icon file should have content");
     }
 
+    /// <summary>
+    /// Tests that negative indices (resource IDs) extract successfully.
+    /// PrivateExtractIcons handles negative indices as resource IDs natively.
+    /// </summary>
+    [Fact]
+    public void IconExtractor_NegativeIndex_ExtractsSuccessfully()
+    {
+        // Arrange
+        var imageresPath = @"C:\Windows\System32\imageres.dll";
+        if (!File.Exists(imageresPath))
+            return;
+
+        using var extractor = new IconExtractor(imageresPath);
+
+        // Resource ID 183 exists in imageres.dll
+        Assert.True(extractor.HasResourceId(183), "imageres.dll should have resource ID 183");
+
+        // Act - Extract with -183 (resource ID)
+        var outputPath = Path.Combine(_fixture.TestDataPath, "test_imageres_neg183.ico");
+        extractor.SaveIcon(-183, outputPath);
+
+        // Assert - File was created with content
+        Assert.True(File.Exists(outputPath), "Icon file should be created");
+        var fileInfo = new FileInfo(outputPath);
+        Assert.True(fileInfo.Length > 1000, "Icon file should have content");
+    }
+
+    /// <summary>
+    /// Tests that GetIcon and SaveIcon produce consistent results for the same index.
+    /// This is a regression test for the bug where preview showed a different icon than extraction.
+    /// </summary>
+    [Fact]
+    public void IconExtractor_GetIconAndSaveIcon_ProduceConsistentResults()
+    {
+        // Arrange
+        var imageresPath = @"C:\Windows\System32\imageres.dll";
+        if (!File.Exists(imageresPath))
+            return;
+
+        using var extractor = new IconExtractor(imageresPath);
+        
+        // Act - Get icon via GetIcon (preview) and SaveIcon (extraction) with the same index
+        var previewIcon = extractor.GetIcon(-183, 32);
+        Assert.NotNull(previewIcon);
+        
+        var outputPath = Path.Combine(_fixture.TestDataPath, "test_consistency.ico");
+        extractor.SaveIcon(-183, outputPath);
+        
+        // Assert - Both should succeed (we can't easily compare pixels, but both should work)
+        Assert.True(File.Exists(outputPath), "Extracted icon file should exist");
+        Assert.True(previewIcon.Width > 0, "Preview icon should have valid dimensions");
+        
+        previewIcon.Dispose();
+    }
+
+    /// <summary>
+    /// End-to-end test for imageres.dll,-183 through the full extraction pipeline.
+    /// This verifies that negative resource IDs are handled correctly by the entire system.
+    /// </summary>
+    [Fact]
+    public void ExtractAndInstall_ImageresDll_Neg183_ExtractsCorrectIcon()
+    {
+        // Arrange - Create a test folder with imageres.dll,-183 (negative resource ID)
+        var testFolderPath = Path.Combine(_fixture.TestDataPath, "NegativeResourceIdTest");
+        Directory.CreateDirectory(testFolderPath);
+        
+        // Write desktop.ini with negative resource ID (-183)
+        var iniPath = Path.Combine(testFolderPath, "desktop.ini");
+        File.WriteAllText(iniPath, 
+            "[.ShellClassInfo]\r\n" +
+            "IconResource=%SystemRoot%\\System32\\imageres.dll,-183\r\n");
+
+        try
+        {
+            var service = new FolderIconService();
+            
+            // Scan
+            var scanResult = service.Scan(testFolderPath, recursive: false);
+            Assert.Single(scanResult.Folders);
+            
+            var folder = scanResult.Folders.First();
+            Assert.Equal(-183, folder.CurrentIconResource?.Index);
+            Assert.Equal(FolderIconStatus.ExternalAndValid, folder.Status);
+
+            // Act - Fix (extract icon)
+            var extractResult = service.ExtractAndInstall(new[] { folder }, skipExisting: false);
+
+            // Assert
+            Assert.Single(extractResult.Succeeded);
+            
+            // Verify the icon was extracted
+            var iconPath = Path.Combine(testFolderPath, "folder.ico");
+            Assert.True(File.Exists(iconPath), "Icon should be extracted");
+            
+            // Verify it's a valid ICO file with content
+            var fileInfo = new FileInfo(iconPath);
+            Assert.True(fileInfo.Length > 1000, "Icon file should have substantial content");
+            
+            // Read the icon file header to verify it's valid
+            using var fs = File.OpenRead(iconPath);
+            var header = new byte[6];
+            fs.Read(header, 0, 6);
+            
+            Assert.Equal(0, header[0]); // Reserved
+            Assert.Equal(0, header[1]); // Reserved
+            Assert.Equal(1, header[2]); // Type = 1 (ICO)
+            Assert.Equal(0, header[3]); // Type high byte
+            
+            // Icon count should be > 0
+            var iconCount = header[4] | (header[5] << 8);
+            Assert.True(iconCount > 0, "Icon should have at least one image");
+        }
+        finally
+        {
+            // Cleanup
+            if (Directory.Exists(testFolderPath))
+            {
+                foreach (var file in Directory.GetFiles(testFolderPath))
+                {
+                    try
+                    {
+                        File.SetAttributes(file, FileAttributes.Normal);
+                        File.Delete(file);
+                    }
+                    catch { /* Ignore cleanup errors */ }
+                }
+                try { Directory.Delete(testFolderPath); }
+                catch { /* Ignore cleanup errors */ }
+            }
+        }
+    }
+
     /// <summary>
     /// End-to-end test: Create a folder with a high ordinal index reference,
     /// fix it, and verify the correct icon is extracted.