Code Quality: Improved the new Windows thumbnail API (#17071)

Author: 0x5bfa

src/Files.App.CsWin32/Extras.cs

@@ -40,6 +40,8 @@ public static unsafe nint SetWindowLongPtr(HWND hWnd, WINDOW_LONG_PTR_INDEX nInd
 
 		[DllImport("shell32.dll", EntryPoint = "SHUpdateRecycleBinIcon", CharSet = CharSet.Unicode, SetLastError = true)]
 		public static extern void SHUpdateRecycleBinIcon();
+
+		public const int PixelFormat32bppARGB = 2498570;
 	}
 
 	namespace Extras

src/Files.App.CsWin32/NativeMethods.txt

@@ -219,3 +219,6 @@ DestinationList
 IObjectArray
 GetCurrentProcessExplicitAppUserModelID
 SetCurrentProcessExplicitAppUserModelID
+GdipCreateBitmapFromScan0
+BITMAP
+GetObject

src/Files.App.Storage/Storables/HomeFolder/HomeFolder.cs

@@ -46,37 +46,40 @@ public IAsyncEnumerable<IStorableChild> GetQuickAccessFolderAsync(CancellationTo
 		}
 
 		/// <inheritdoc/>
-		public async IAsyncEnumerable<IStorableChild> GetLogicalDrivesAsync([EnumeratorCancellation] CancellationToken cancellationToken = default)
+		public IAsyncEnumerable<IStorableChild> GetLogicalDrivesAsync(CancellationToken cancellationToken = default)
 		{
-			var availableDrives = PInvoke.GetLogicalDrives();
-			if (availableDrives is 0)
-				yield break;
+			return GetLogicalDrives().ToAsyncEnumerable();
 
-			int count = BitOperations.PopCount(availableDrives);
-			var driveLetters = new char[count];
-
-			count = 0;
-			char driveLetter = 'A';
-			while (availableDrives is not 0)
+			IEnumerable<IStorableChild> GetLogicalDrives()
 			{
-				if ((availableDrives & 1) is not 0)
-					driveLetters[count++] = driveLetter;
+				var availableDrives = PInvoke.GetLogicalDrives();
+				if (availableDrives is 0)
+					yield break;
 
-				availableDrives >>= 1;
-				driveLetter++;
-			}
+				int count = BitOperations.PopCount(availableDrives);
+				var driveLetters = new char[count];
 
-			foreach (int letter in driveLetters)
-			{
-				cancellationToken.ThrowIfCancellationRequested();
+				count = 0;
+				char driveLetter = 'A';
+				while (availableDrives is not 0)
+				{
+					if ((availableDrives & 1) is not 0)
+						driveLetters[count++] = driveLetter;
 
-				if (WindowsStorable.TryParse($"{letter}:\\") is not IWindowsStorable driveRoot)
-					throw new InvalidOperationException();
+					availableDrives >>= 1;
+					driveLetter++;
+				}
 
-				yield return new WindowsFolder(driveRoot.ThisPtr);
-				await Task.Yield();
-			}
+				foreach (char letter in driveLetters)
+				{
+					cancellationToken.ThrowIfCancellationRequested();
+
+					if (WindowsStorable.TryParse($"{letter}:\\") is not IWindowsStorable driveRoot)
+						throw new InvalidOperationException();
 
+					yield return new WindowsFolder(driveRoot.ThisPtr);
+				}
+			}
 		}
 
 		/// <inheritdoc/>

src/Files.App.Storage/Storables/WindowsStorage/IWindowsStorable.cs

@@ -6,7 +6,7 @@
 
 namespace Files.App.Storage
 {
-	public interface IWindowsStorable
+	public interface IWindowsStorable : IDisposable
 	{
 		ComPtr<IShellItem> ThisPtr { get; }
 	}

src/Files.App.Storage/Storables/WindowsStorage/WindowsFile.cs

@@ -8,7 +8,7 @@
 namespace Files.App.Storage
 {
 	[DebuggerDisplay("{" + nameof(ToString) + "()}")]
-	public sealed class WindowsFile : WindowsStorable, IChildFile, IDisposable
+	public sealed class WindowsFile : WindowsStorable, IChildFile
 	{
 		public WindowsFile(ComPtr<IShellItem> nativeObject)
 		{
@@ -19,13 +19,5 @@ public Task<Stream> OpenStreamAsync(FileAccess accessMode, CancellationToken can
 		{
 			throw new NotImplementedException();
 		}
-
-		// Disposer
-
-		/// <inheritdoc/>
-		public void Dispose()
-		{
-			ThisPtr.Dispose();
-		}
 	}
 }

src/Files.App.Storage/Storables/WindowsStorage/WindowsFolder.cs

@@ -9,8 +9,8 @@
 
 namespace Files.App.Storage
 {
-	[DebuggerDisplay("{" + nameof(ToString) + "()")]
-	public sealed class WindowsFolder : WindowsStorable, IChildFolder, IDisposable
+	[DebuggerDisplay("{" + nameof(ToString) + "()}")]
+	public sealed class WindowsFolder : WindowsStorable, IChildFolder
 	{
 		public WindowsFolder(ComPtr<IShellItem> nativeObject)
 		{
@@ -26,81 +26,64 @@ public unsafe WindowsFolder(IShellItem* nativeObject)
 
 		public unsafe WindowsFolder(Guid folderId)
 		{
-			Guid folderIdLocal = folderId;
-			Guid IID_IShellItem = IShellItem.IID_Guid;
 			ComPtr<IShellItem> pItem = default;
 
-			HRESULT hr = PInvoke.SHGetKnownFolderItem(&folderIdLocal, KNOWN_FOLDER_FLAG.KF_FLAG_DEFAULT, HANDLE.Null, &IID_IShellItem, (void**)pItem.GetAddressOf());
-			if (hr.Succeeded)
+			HRESULT hr = PInvoke.SHGetKnownFolderItem(&folderId, KNOWN_FOLDER_FLAG.KF_FLAG_DEFAULT, HANDLE.Null, IID.IID_IShellItem, (void**)pItem.GetAddressOf());
+			if (hr.Failed)
 			{
-				ThisPtr = pItem;
-				return;
-			}
+				fixed (char* pszShellPath = $"Shell:::{folderId:B}")
+					hr = PInvoke.SHCreateItemFromParsingName(pszShellPath, null, IID.IID_IShellItem, (void**)pItem.GetAddressOf());
 
-			fixed (char* pszShellPath = $"Shell:::{folderId:B}")
-				hr = PInvoke.SHCreateItemFromParsingName(pszShellPath, null, &IID_IShellItem, (void**)pItem.GetAddressOf());
-			if (hr.Succeeded)
-			{
-				ThisPtr = pItem;
-				return;
+				// Invalid FOLDERID; this should never happen.
+				hr.ThrowOnFailure();
 			}
+
+			ThisPtr = pItem;
 		}
 
-		public async IAsyncEnumerable<IStorableChild> GetItemsAsync(StorableType type = StorableType.All, [EnumeratorCancellation] CancellationToken cancellationToken = default)
+		public IAsyncEnumerable<IStorableChild> GetItemsAsync(StorableType type = StorableType.All, CancellationToken cancellationToken = default)
 		{
-			using ComPtr<IEnumShellItems> pEnumShellItems = GetEnumShellItems();
-			while (GetNext(pEnumShellItems) is { } pShellItem && !pShellItem.IsNull)
-			{
-				cancellationToken.ThrowIfCancellationRequested();
+			return GetItems().ToAsyncEnumerable();
 
-				var pShellFolder = pShellItem.As<IShellFolder>();
-				var isFolder = IsFolder(pShellItem);
+			unsafe IEnumerable<IStorableChild> GetItems()
+			{
+				ComPtr<IEnumShellItems> pEnumShellItems = default;
+				GetEnumerator();
 
-				if (type is StorableType.File && !isFolder)
+				ComPtr<IShellItem> pShellItem = default;
+				while (GetNext() && !pShellItem.IsNull)
 				{
-					yield return new WindowsFile(pShellItem);
+					cancellationToken.ThrowIfCancellationRequested();
+					var isFolder = pShellItem.HasShellAttributes(SFGAO_FLAGS.SFGAO_FOLDER);
+
+					if (type is StorableType.File && !isFolder)
+					{
+						yield return new WindowsFile(pShellItem);
+					}
+					else if (type is StorableType.Folder && isFolder)
+					{
+						yield return new WindowsFolder(pShellItem);
+					}
+					else
+					{
+						continue;
+					}
 				}
-				else if (type is StorableType.Folder && isFolder)
+
+				yield break;
+
+				unsafe void GetEnumerator()
 				{
-					yield return new WindowsFile(pShellItem);
+					HRESULT hr = ThisPtr.Get()->BindToHandler(null, BHID.BHID_EnumItems, IID.IID_IEnumShellItems, (void**)pEnumShellItems.GetAddressOf());
+					hr.ThrowIfFailedOnDebug();
 				}
-				else
+
+				unsafe bool GetNext()
 				{
-					continue;
+					HRESULT hr = pEnumShellItems.Get()->Next(1, pShellItem.GetAddressOf());
+					return hr.ThrowIfFailedOnDebug() == HRESULT.S_OK;
 				}
-
-				await Task.Yield();
 			}
-
-			unsafe ComPtr<IEnumShellItems> GetEnumShellItems()
-			{
-				ComPtr<IEnumShellItems> pEnumShellItems = default;
-				Guid IID_IEnumShellItems = typeof(IEnumShellItems).GUID;
-				Guid BHID_EnumItems = PInvoke.BHID_EnumItems;
-				HRESULT hr = ThisPtr.Get()->BindToHandler(null, &BHID_EnumItems, &IID_IEnumShellItems, (void**)pEnumShellItems.GetAddressOf());
-				return pEnumShellItems;
-			}
-
-			unsafe ComPtr<IShellItem> GetNext(ComPtr<IEnumShellItems> pEnumShellItems)
-			{
-				ComPtr<IShellItem> pShellItem = default;
-				HRESULT hr = pEnumShellItems.Get()->Next(1, pShellItem.GetAddressOf());
-				return pShellItem;
-			}
-
-			unsafe bool IsFolder(ComPtr<IShellItem> pShellItem)
-			{
-				return pShellItem.Get()->GetAttributes(SFGAO_FLAGS.SFGAO_FOLDER, out var specifiedAttribute).Succeeded &&
-					specifiedAttribute is SFGAO_FLAGS.SFGAO_FOLDER;
-			}
-		}
-
-		// Disposer
-
-		/// <inheritdoc/>
-		public void Dispose()
-		{
-			ThisPtr.Dispose();
 		}
 	}
 }

src/Files.App.Storage/Storables/WindowsStorage/WindowsStorable.cs

@@ -10,7 +10,7 @@ namespace Files.App.Storage
 {
 	public abstract class WindowsStorable : IWindowsStorable, IStorableChild
 	{
-		public ComPtr<IShellItem> ThisPtr { get; protected set; } = default;
+		public ComPtr<IShellItem> ThisPtr { get; protected set; }
 
 		public string Id => this.GetDisplayName(SIGDN.SIGDN_FILESYSPATH);
 
@@ -65,6 +65,12 @@ public abstract class WindowsStorable : IWindowsStorable, IStorableChild
 			return Task.FromResult<IFolder?>(new WindowsFolder(pParentFolder));
 		}
 
+		/// <inheritdoc/>
+		public void Dispose()
+		{
+			ThisPtr.Dispose();
+		}
+
 		/// <inheritdoc/>
 		public override string ToString()
 		{

src/Files.App.Storage/Storables/WindowsStorage/WindowsStorableHelpers.Icon.cs

@@ -57,21 +57,46 @@ public unsafe static HRESULT TryGetThumbnail(this IWindowsStorable storable, int
 				return hr;
 			}
 
-			// Convert to GpBitmap of GDI+
+			// Retrieve BITMAP data
+			BITMAP bmp = default;
+			if (PInvoke.GetObject(hBitmap, sizeof(BITMAP), &bmp) is 0)
+			{
+				if (!hBitmap.IsNull) PInvoke.DeleteObject(hBitmap);
+				return HRESULT.E_FAIL;
+			}
+
+			// Allocate buffer for flipped pixel data
+			byte* flippedBits = (byte*)NativeMemory.AllocZeroed((nuint)(bmp.bmWidthBytes * bmp.bmHeight));
+
+			// Flip the image manually row by row
+			for (int y = 0; y < bmp.bmHeight; y++)
+			{
+				Buffer.MemoryCopy(
+					(byte*)bmp.bmBits + y * bmp.bmWidthBytes,
+					flippedBits + (bmp.bmHeight - y - 1) * bmp.bmWidthBytes,
+					bmp.bmWidthBytes,
+					bmp.bmWidthBytes
+				);
+			}
+
+			// Create GpBitmap from the flipped pixel data
 			GpBitmap* gpBitmap = default;
-			if (PInvoke.GdipCreateBitmapFromHBITMAP(hBitmap, HPALETTE.Null, &gpBitmap) is not Status.Ok)
+			if (PInvoke.GdipCreateBitmapFromScan0(bmp.bmWidth, bmp.bmHeight, bmp.bmWidthBytes, PInvoke.PixelFormat32bppARGB, flippedBits, &gpBitmap) != Status.Ok)
 			{
-				if (gpBitmap is not null) PInvoke.GdipDisposeImage((GpImage*)gpBitmap);
+				if (flippedBits is not null) NativeMemory.Free(flippedBits);
 				if (!hBitmap.IsNull) PInvoke.DeleteObject(hBitmap);
 				return HRESULT.E_FAIL;
 			}
 
-			if (TryConvertGpBitmapToByteArray(gpBitmap, out thumbnailData))
+			if (!TryConvertGpBitmapToByteArray(gpBitmap, out thumbnailData))
 			{
 				if (!hBitmap.IsNull) PInvoke.DeleteObject(hBitmap);
 				return HRESULT.E_FAIL;
 			}
 
+			if (flippedBits is not null) NativeMemory.Free(flippedBits);
+			if (!hBitmap.IsNull) PInvoke.DeleteObject(hBitmap);
+
 			return HRESULT.S_OK;
 		}
 
@@ -110,14 +135,14 @@ public unsafe static HRESULT TryExtractImageFromDll(this IWindowsStorable storab
 					return HRESULT.E_FAIL;
 				}
 
-				if (!TryConvertGpBitmapToByteArray(gpBitmap, out imageData))
+				if (!TryConvertGpBitmapToByteArray(gpBitmap, out imageData) || imageData is null)
 				{
 					if (!hIcon.IsNull) PInvoke.DestroyIcon(hIcon);
 					return HRESULT.E_FAIL;
 				}
 
 				DllIconCache[(path, index, size)] = imageData;
-				PInvoke.DestroyIcon(hIcon);
+				if (!hIcon.IsNull) PInvoke.DestroyIcon(hIcon);
 
 				return HRESULT.S_OK;
 			}

src/Files.App.Storage/Storables/WindowsStorage/WindowsStorableHelpers.Shell.cs

@@ -48,7 +48,7 @@ public unsafe static string GetDisplayName(this IWindowsStorable storable, SIGDN
 			HRESULT hr = storable.ThisPtr.Get()->GetDisplayName(options, (PWSTR*)pszName.GetAddressOf());
 
 			return hr.ThrowIfFailedOnDebug().Succeeded
-				? (*pszName.Get()).ToString()
+				? new string((char*)pszName.Get()) // this is safe as it gets memcpy'd internally
 				: string.Empty;
 		}
 	}