[PATCH v5 0/1] MR8367: shell32: Try to lookup icon, with fallback on failure

[James McDonnell (＠ElementalWarrior)]

Try to lookup icon information from shell32.dll. If an icon is not
found, default to the file icon.

Instead of defaulting to -1, which was failing for Affinity Photo's
installer.

--
  v5: shell32: Try to lookup icon, with fallback on failure
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367

[PATCH v5 1/1] shell32: Try to lookup icon, with fallback on failure

[James McDonnell]

From: James McDonnell <topgamer7(a)gmail.com>

Try to lookup icon information from shell32.dll. If an icon is not
found, default to the help icon.

This resolves the issue where in affinity photos installer it
would crash because hIcon was returned as 0
---
 dlls/shell32/iconcache.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/dlls/shell32/iconcache.c b/dlls/shell32/iconcache.c
index 8f9519d2ca8..c72671eb528 100644
--- a/dlls/shell32/iconcache.c
+++ b/dlls/shell32/iconcache.c
@@ -976,6 +976,16 @@ INT WINAPI SHGetIconOverlayIndexW(LPCWSTR pszIconPath, INT iIconIndex)
   return -1;
 }
 
+/****************************************************************************
+ * StockIconIdToResourceId    [SHELL32.@]
+ *
+ * Map the SHSTOCKICONID value to shresdef.h IDI_ values
+ */
+INT StockIconIdToResourceId(SHSTOCKICONID id)
+{
+  return id + 1;
+}
+
 /****************************************************************************
  * SHGetStockIconInfo [SHELL32.@]
  *
@@ -1000,16 +1010,24 @@ HRESULT WINAPI SHGetStockIconInfo(SHSTOCKICONID id, UINT flags, SHSTOCKICONINFO
 
     GetSystemDirectoryW(sii->szPath, MAX_PATH);
 
-    /* no icons defined: use default */
-    sii->iIcon = -IDI_SHELL_FILE;
+    sii->iIcon = id;
     lstrcatW(sii->szPath, L"\\shell32.dll");
 
-    if (flags)
-        FIXME("flags 0x%x not implemented\n", flags);
+    if (flags & ~SHGSI_ICON)
+        FIXME("unhandled flags 0x%x\n", flags);
 
     sii->hIcon = NULL;
-    if (flags & SHGSI_ICON)
-        sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(sii->iIcon));
+    if (flags & SHGSI_ICON) {
+        sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(StockIconIdToResourceId(id)));
+
+        // many icons do not exist yet in wine, map to the question mark
+        if (!sii->hIcon) {
+          FIXME("Icon %p missing, defaulting to help icon.\n", sii->hIcon);
+
+          sii->iIcon = IDI_SHELL_HELP - 1;
+          sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(IDI_SHELL_HELP));
+        }
+    }
     sii->iSysImageIndex = -1;
 
     TRACE("%3d: returning %s (%d)\n", id, debugstr_w(sii->szPath), sii->iIcon);
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8367

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/shell32/iconcache.c:
> -    sii->iIcon = -IDI_SHELL_FILE;
> +    sii->iIcon = id;
>      lstrcatW(sii->szPath, L"\\shell32.dll");
>
> -    if (flags)
> -        FIXME("flags 0x%x not implemented\n", flags);
> +    if (flags & ~SHGSI_ICON)
> +        FIXME("unhandled flags 0x%x\n", flags);
>
> sii->hIcon = NULL;
> -    if (flags & SHGSI_ICON)
> -        sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(sii->iIcon));
> +    if (flags & SHGSI_ICON) {
> +        sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(StockIconIdToResourceId(id)));
> +
> +        // many icons do not exist yet in wine, map to the question mark
> Use /**/ for comments.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110407

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/shell32/iconcache.c:
> -    if (flags)
> -        FIXME("flags 0x%x not implemented\n", flags);
> +    if (flags & ~SHGSI_ICON)
> +        FIXME("unhandled flags 0x%x\n", flags);
>
> sii->hIcon = NULL;
> -    if (flags & SHGSI_ICON)
> -        sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(sii->iIcon));
> +    if (flags & SHGSI_ICON) {
> +        sii->hIcon = LoadIconW(GetModuleHandleW(sii->szPath), MAKEINTRESOURCEW(StockIconIdToResourceId(id)));
> +
> +        // many icons do not exist yet in wine, map to the question mark
> +        if (!sii->hIcon) {
> +          FIXME("Icon %p missing, defaulting to help icon.\n", sii->hIcon);
> +
> +          sii->iIcon = IDI_SHELL_HELP - 1;
> Please add some tests.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110408

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/shell32/iconcache.c:
> return -1;
>  }
>
> +/****************************************************************************
> + * StockIconIdToResourceId    [SHELL32.@]
> + *
> + * Map the SHSTOCKICONID value to shresdef.h IDI_ values
> + */
> +INT StockIconIdToResourceId(SHSTOCKICONID id)
> +{
> +  return id + 1;
> I don't think this worth adding a function.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110410

[James McDonnell (＠ElementalWarrior)]

On Sun Jul 20 03:30:00 2025 +0000, Zhiyi Zhang wrote:
> Please add some tests. And I don't think falling back to IDI_SHELL_HELP
> is correct.
> Well no, its not. But it at least returns an icon. There are only 43 icons defined in dlls/shell32/resources. But this function is supposed to return something like 180.

This at least returns an icon, as opposed to failing because the icon does not exist.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110411

[James McDonnell (＠ElementalWarrior)]

On Sun Jul 20 03:44:12 2025 +0000, James McDonnell wrote:
> Well no, its not. But it at least returns an icon. There are only 43
> icons defined in dlls/shell32/resources. But this function is supposed
> to return something like 180.
> This at least returns an icon, as opposed to failing because the icon
> does not exist.
> It seems like there is a need to fallback to something. That or create the missing icons. Given the wine projects conservative nature regarding IP, I doubt generating them with AI would be accepted.

If using a default of an existing icon is not a good solution, what would you suggest?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110412

[Zhiyi Zhang (＠zhiyi)]

On Sun Jul 20 03:47:32 2025 +0000, James McDonnell wrote:
> It seems like there is a need to fallback to something. That or create
> the missing icons. Given the wine projects conservative nature regarding
> IP, I doubt generating them with AI would be accepted.
> If using a default of an existing icon is not a good solution, what
> would you suggest?
> I would add the missing icons when necessary. There are some icon sets out there with licenses compatible with Wine's license.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110414

[Zhiyi Zhang (＠zhiyi)]

On Sun Jul 20 04:22:06 2025 +0000, Zhiyi Zhang wrote:
> I would add the missing icons when necessary. There are some icon sets
> out there with licenses compatible with Wine's license.
> For example, 4213781ae0f8b45c7ef01e8f4c28a47d8c348e3b

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8367#note_110415