[PATCH v7 0/3] Partial IShellItemImageFactory implementation (#52673)
Jinoh Kang (3): shell32/tests: Add tests for IShellItemImageFactory. shell32: Add stub for IShellItemImageFactory. shell32: Partially implement IShellItemImageFactory (icon only, no thumbnail). dlls/shell32/Makefile.in | 2 +- dlls/shell32/shellitem.c | 241 +++++++++++++++++++++++++++++++++ dlls/shell32/tests/shlfolder.c | 50 +++++++ 3 files changed, 292 insertions(+), 1 deletion(-) -- 2.34.1
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: v1 -> v2: mark test broken on Windows 7 v2 -> v3: remove erroneous todo_wine v3 -> v4: no changes v4 -> v5: test if IShellItemImageFactory::GetImage loads windowscodecs.dll v5 -> v6: no changes v6 -> v7: - remove missing SHCreateShellItem function check (thanks nsivov) - remove some more error checks (thanks nsivov) - remove windowscodecs.dll load check (thanks nsivov) - replace GetObjectType check with DIBSECTION check (thanks nsivov) - release resources as soon as they become unneeded - clarify test fail messages - remove broken() condition dlls/shell32/tests/shlfolder.c | 52 ++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c index f99045c33d5..d37d368c5cd 100644 --- a/dlls/shell32/tests/shlfolder.c +++ b/dlls/shell32/tests/shlfolder.c @@ -4433,6 +4433,57 @@ static void test_contextmenu(IContextMenu *menu, BOOL background) DestroyMenu(hmenu); } +static void test_IShellItemImageFactory(void) +{ + HRESULT ret; + IShellItem *shellitem; + IShellItemImageFactory *siif; + LPITEMIDLIST pidl_desktop = NULL; + + ret = SHGetSpecialFolderLocation(NULL, CSIDL_DESKTOP, &pidl_desktop); + ok(ret == S_OK, "SHGetSpecialFolderLocation returned 0x%08lx\n", ret); + + ret = pSHCreateShellItem(NULL, NULL, pidl_desktop, &shellitem); + ILFree(pidl_desktop); + ok(SUCCEEDED(ret), "SHCreateShellItem returned 0x%08lx\n", ret); + + ret = IShellItem_QueryInterface(shellitem, &IID_IShellItemImageFactory, (void **)&siif); + IShellItem_Release(shellitem); + todo_wine + ok(ret == S_OK, "QueryInterface returned 0x%08lx\n", ret); + if (SUCCEEDED(ret)) + { + HBITMAP hbm = NULL; + SIZE size = {32, 32}; + + ret = IShellItemImageFactory_GetImage(siif, size, SIIGBF_BIGGERSIZEOK, &hbm); + IShellItemImageFactory_Release(siif); + todo_wine + ok(ret == S_OK, "GetImage returned %lx\n", ret); + ok(FAILED(ret) == !hbm, "result = %lx but bitmap = %p\n", ret, hbm); + + if (SUCCEEDED(ret) && hbm) + { + DIBSECTION dib; + int infosz; + + infosz = GetObjectW(hbm, sizeof(dib), &dib); + ok(infosz == sizeof(dib), "Expected GetObjectW to return sizeof(DIBSECTION), got %d\n", infosz); + + /* Bitmap must have 32-bit pixels regardless of original image format */ + ok(dib.dsBm.bmPlanes == 1, "Expected bmPlanes to be %d, got %d\n", 1, dib.dsBm.bmPlanes); + ok(dib.dsBm.bmBitsPixel == 32, "Expected bmBitsPixel to be %d, got %d\n", 32, dib.dsBm.bmBitsPixel); + + /* DIB must be truecolor (32bppRGB/32bppARGB) regardless of original image format */ + ok(dib.dsBmih.biPlanes == 1, "Expected biPlanes to be %d, got %d\n", 1, dib.dsBmih.biPlanes); + ok(dib.dsBmih.biBitCount == 32, "Expected biBitCount to be %d, got %d\n", 32, dib.dsBmih.biBitCount); + ok(dib.dsBmih.biCompression == BI_RGB, "Expected biCompression to be BI_RGB, got %lu\n", dib.dsBmih.biCompression); + + DeleteObject(hbm); + } + } +} + static void test_GetUIObject(void) { IShellFolder *psf_desktop; @@ -5385,6 +5436,7 @@ START_TEST(shlfolder) test_SHCreateShellItemArray(); test_ShellItemArrayEnumItems(); test_desktop_IPersist(); + test_IShellItemImageFactory(); test_GetUIObject(); test_CreateViewObject_contextmenu(); test_SHSimpleIDListFromPath(); -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113477 Your paranoid android. === w1064v1809 (32 bit report) === shell32: shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064 (32 bit report) === shell32: shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064v1809 (64 bit report) === shell32: shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064 (64 bit report) === shell32: shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064_2qxl (64 bit report) === shell32: shlfolder.c:4962: Test failed: RMDIR: expected notification type 10, got: 40000 shlfolder.c:4969: Test failed: GetDisplayNameOf failed: 0x80070057 shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 10) === w10pro64_ar (64 bit report) === shell32: shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 2) shlfolder.c:4962: Test failed: RMDIR: expected notification type 10, got: 40000 shlfolder.c:4969: Test failed: GetDisplayNameOf failed: 0x80070057 shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 10) === w10pro64_he (64 bit report) === shell32: shlfolder.c:4962: Test failed: CREATE: expected notification type 2, got: 40000 shlfolder.c:4969: Test failed: GetDisplayNameOf failed: 0x80070057 shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_ja (64 bit report) === shell32: shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_zh_CN (64 bit report) === shell32: shlfolder.c:4979: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000)
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: v1 -> v2: no changes v2 -> v3: change iface lpVtbl initialization order to match struct order v3 -> v4: no changes v4 -> v5: %lu -> %d (SIIGBF) v5 -> v6: no changes v6 -> v7: account for changes in previous patch dlls/shell32/shellitem.c | 50 ++++++++++++++++++++++++++++++++++ dlls/shell32/tests/shlfolder.c | 1 - 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/dlls/shell32/shellitem.c b/dlls/shell32/shellitem.c index 0a3a76cbd6a..0381fa67f0d 100644 --- a/dlls/shell32/shellitem.c +++ b/dlls/shell32/shellitem.c @@ -39,6 +39,7 @@ typedef struct _ShellItem { LONG ref; LPITEMIDLIST pidl; IPersistIDList IPersistIDList_iface; + IShellItemImageFactory IShellItemImageFactory_iface; } ShellItem; typedef struct _CustomDestinationList { @@ -56,6 +57,11 @@ static inline ShellItem *impl_from_IPersistIDList( IPersistIDList *iface ) return CONTAINING_RECORD(iface, ShellItem, IPersistIDList_iface); } +static inline ShellItem *impl_from_IShellItemImageFactory( IShellItemImageFactory *iface ) +{ + return CONTAINING_RECORD(iface, ShellItem, IShellItemImageFactory_iface); +} + static inline CustomDestinationList *impl_from_ICustomDestinationList( ICustomDestinationList *iface ) { return CONTAINING_RECORD(iface, CustomDestinationList, ICustomDestinationList_iface); @@ -79,6 +85,10 @@ static HRESULT WINAPI ShellItem_QueryInterface(IShellItem2 *iface, REFIID riid, { *ppv = &This->IPersistIDList_iface; } + else if (IsEqualIID(&IID_IShellItemImageFactory, riid)) + { + *ppv = &This->IShellItemImageFactory_iface; + } else { FIXME("not implemented for %s\n", shdebugstr_guid(riid)); *ppv = NULL; @@ -536,6 +546,45 @@ static const IPersistIDListVtbl ShellItem_IPersistIDList_Vtbl = { ShellItem_IPersistIDList_GetIDList }; +static HRESULT WINAPI ShellItem_IShellItemImageFactory_QueryInterface(IShellItemImageFactory *iface, + REFIID riid, void **ppv) +{ + ShellItem *This = impl_from_IShellItemImageFactory(iface); + return IShellItem2_QueryInterface(&This->IShellItem2_iface, riid, ppv); +} + +static ULONG WINAPI ShellItem_IShellItemImageFactory_AddRef(IShellItemImageFactory *iface) +{ + ShellItem *This = impl_from_IShellItemImageFactory(iface); + return IShellItem2_AddRef(&This->IShellItem2_iface); +} + +static ULONG WINAPI ShellItem_IShellItemImageFactory_Release(IShellItemImageFactory *iface) +{ + ShellItem *This = impl_from_IShellItemImageFactory(iface); + return IShellItem2_Release(&This->IShellItem2_iface); +} + +static HRESULT WINAPI ShellItem_IShellItemImageFactory_GetImage(IShellItemImageFactory *iface, + SIZE size, SIIGBF flags, HBITMAP *phbm) +{ + ShellItem *This = impl_from_IShellItemImageFactory(iface); + static int once; + + if (!once++) + FIXME("%p ({%lu, %lu} %d %p): stub\n", This, size.cx, size.cy, flags, phbm); + + *phbm = NULL; + return E_NOTIMPL; +} + +static const IShellItemImageFactoryVtbl ShellItem_IShellItemImageFactory_Vtbl = { + ShellItem_IShellItemImageFactory_QueryInterface, + ShellItem_IShellItemImageFactory_AddRef, + ShellItem_IShellItemImageFactory_Release, + ShellItem_IShellItemImageFactory_GetImage, +}; + HRESULT WINAPI IShellItem_Constructor(IUnknown *pUnkOuter, REFIID riid, void **ppv) { @@ -553,6 +602,7 @@ HRESULT WINAPI IShellItem_Constructor(IUnknown *pUnkOuter, REFIID riid, void **p This->ref = 1; This->pidl = NULL; This->IPersistIDList_iface.lpVtbl = &ShellItem_IPersistIDList_Vtbl; + This->IShellItemImageFactory_iface.lpVtbl = &ShellItem_IShellItemImageFactory_Vtbl; ret = IShellItem2_QueryInterface(&This->IShellItem2_iface, riid, ppv); IShellItem2_Release(&This->IShellItem2_iface); diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c index d37d368c5cd..b690e979ab5 100644 --- a/dlls/shell32/tests/shlfolder.c +++ b/dlls/shell32/tests/shlfolder.c @@ -4449,7 +4449,6 @@ static void test_IShellItemImageFactory(void) ret = IShellItem_QueryInterface(shellitem, &IID_IShellItemImageFactory, (void **)&siif); IShellItem_Release(shellitem); - todo_wine ok(ret == S_OK, "QueryInterface returned 0x%08lx\n", ret); if (SUCCEEDED(ret)) { -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113478 Your paranoid android. === w1064v1809 (32 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064 (32 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064v1809 (64 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) shlfolder.c:4961: Test failed: CREATE: expected notification type 2, got: 40000 shlfolder.c:4968: Test failed: GetDisplayNameOf failed: 0x80070057 shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 2) === w1064 (64 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064_2qxl (64 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w10pro64_ar (64 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_he (64 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_ja (64 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w10pro64_zh_CN (64 bit report) === shell32: shlfolder.c:4978: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3)
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52673 Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: v1 -> v2: no changes v3 -> v4: - Remove patch "shell32: Factor out ShellItem_get_uiobject." - Use IShellItem2_BindToHandler instead of factoring out ShellItem_get_uiobject - Use CreateCompatibleDC(NULL) instead of GetDC(NULL) - Remove new dependency on win32u, use GetIconInfo instead - Remove new dependency on windowcodecs, use gdiplus instead v4 -> v5: revert to using windowscodecs v5 -> v6: always set [out] parameter of IShellItemImageFactory::GetImage v6 -> v7: account for changes in previous patch dlls/shell32/Makefile.in | 2 +- dlls/shell32/shellitem.c | 195 ++++++++++++++++++++++++++++++++- dlls/shell32/tests/shlfolder.c | 1 - 3 files changed, 194 insertions(+), 4 deletions(-) diff --git a/dlls/shell32/Makefile.in b/dlls/shell32/Makefile.in index eeb6cd63d60..0cfaac8e9cc 100644 --- a/dlls/shell32/Makefile.in +++ b/dlls/shell32/Makefile.in @@ -2,7 +2,7 @@ EXTRADEFS = -D_SHELL32_ MODULE = shell32.dll IMPORTLIB = shell32 IMPORTS = uuid shlwapi user32 gdi32 advapi32 -DELAYIMPORTS = ole32 oleaut32 shdocvw version comctl32 gdiplus +DELAYIMPORTS = ole32 oleaut32 shdocvw version comctl32 gdiplus windowscodecs C_SRCS = \ appbar.c \ diff --git a/dlls/shell32/shellitem.c b/dlls/shell32/shellitem.c index 0381fa67f0d..7ce8669fdfb 100644 --- a/dlls/shell32/shellitem.c +++ b/dlls/shell32/shellitem.c @@ -31,9 +31,12 @@ #include "pidl.h" #include "shell32_main.h" #include "debughlp.h" +#include "wincodec.h" WINE_DEFAULT_DEBUG_CHANNEL(shell); +HRESULT WINAPI WICCreateImagingFactory_Proxy(UINT, IWICImagingFactory**); + typedef struct _ShellItem { IShellItem2 IShellItem2_iface; LONG ref; @@ -565,17 +568,205 @@ static ULONG WINAPI ShellItem_IShellItemImageFactory_Release(IShellItemImageFact return IShellItem2_Release(&This->IShellItem2_iface); } +static HRESULT ShellItem_get_icons(ShellItem *This, SIZE size, HICON *big_icon, HICON *small_icon) +{ + HRESULT hr; + IBindCtx *pbc; + IExtractIconW *ei; + WCHAR icon_file[MAX_PATH]; + INT source_index; + UINT gil_in_flags = 0, gil_out_flags; + INT iconsize; + + iconsize = min(size.cx, size.cy); + if (iconsize <= 0 || iconsize > 0x7fff) + iconsize = 0x7fff; + + hr = CreateBindCtx(0, &pbc); + if (FAILED(hr)) goto done; + + hr = IShellItem2_BindToHandler(&This->IShellItem2_iface, pbc, &BHID_SFUIObject, + &IID_IExtractIconW, (void **)&ei); + IBindCtx_Release(pbc); + if (FAILED(hr)) goto done; + + hr = IExtractIconW_GetIconLocation(ei, gil_in_flags, icon_file, MAX_PATH, &source_index, &gil_out_flags); + if (FAILED(hr)) goto free_ei; + + if (!(gil_out_flags & GIL_NOTFILENAME)) + { + UINT ei_res; + + if (source_index == -1) + source_index = 0; /* special case for some control panel applications */ + + FIXME("%s %d\n", debugstr_w(icon_file), source_index); + ei_res = ExtractIconExW(icon_file, source_index, big_icon, small_icon, 1); + if (!ei_res || ei_res == (UINT)-1) + { + WARN("Failed to extract icon.\n"); + hr = E_FAIL; + } + } + else + { + hr = IExtractIconW_Extract(ei, icon_file, source_index, big_icon, small_icon, MAKELONG(iconsize, iconsize)); + } + +free_ei: + IExtractIconW_Release(ei); +done: + return hr; +} + +static HICON choose_best_icon(HICON *icons, UINT count, SIZE size_limit, SIZE *out_size) +{ + HICON best_icon = NULL; + SIZE best_size = {0, 0}; + UINT i; + + for (i = 0; i < count; i++) + { + ICONINFO iinfo; + BITMAP bm; + SIZE size; + BOOL is_color, ret; + + if (!icons[i] || !GetIconInfo(icons[i], &iinfo)) continue; + + is_color = iinfo.hbmColor != NULL; + ret = GetObjectW(is_color ? iinfo.hbmColor : iinfo.hbmMask, sizeof(bm), &bm); + DeleteObject(iinfo.hbmColor); + DeleteObject(iinfo.hbmMask); + if (!ret) continue; + + size.cx = bm.bmWidth; + size.cy = is_color ? abs(bm.bmHeight) : abs(bm.bmHeight) / 2; + + if (!best_icon || (best_size.cx < size.cx && size.cx <= size_limit.cx && + best_size.cy < size.cy && size.cy <= size_limit.cy)) + { + best_icon = icons[i]; + best_size = size; + } + } + + *out_size = best_size; + return best_icon; +} + +static HRESULT ShellItem_get_icon_bitmap(ShellItem *This, IWICImagingFactory *imgfactory, + SIZE size, SIIGBF flags, IWICBitmap **bitmap) +{ + HRESULT hr; + HICON icons[2] = { NULL, NULL }, best_icon; + SIZE best_icon_size; + UINT i; + + *bitmap = NULL; + + hr = ShellItem_get_icons(This, size, &icons[0], &icons[1]); + if (FAILED(hr)) return hr; + + best_icon = choose_best_icon(icons, ARRAY_SIZE(icons), size, &best_icon_size); + for (i = 0; i < ARRAY_SIZE(icons); i++) + if (icons[i] && icons[i] != best_icon) DeleteObject(icons[i]); + + if (!best_icon) return E_FAIL; + + hr = IWICImagingFactory_CreateBitmapFromHICON(imgfactory, best_icon, bitmap); + DeleteObject(best_icon); + return hr; +} + +static HRESULT convert_wicbitmapsource_to_gdi(IWICImagingFactory *imgfactory, + IWICBitmapSource *bitmapsource, HBITMAP *gdibitmap) +{ + BITMAPINFOHEADER bmi; + HRESULT hr; + UINT width, height; + IWICBitmapSource *newsrc; + HDC dc; + HBITMAP bm; + void *bits; + + *gdibitmap = NULL; + + hr = WICConvertBitmapSource(&GUID_WICPixelFormat32bppBGRA, bitmapsource, &newsrc); + if (FAILED(hr)) goto done; + + hr = IWICBitmapSource_GetSize(newsrc, &width, &height); + if (FAILED(hr)) goto free_newsrc; + + dc = CreateCompatibleDC(NULL); + if (!dc) + { + hr = E_FAIL; + goto free_newsrc; + } + + memset(&bmi, 0, sizeof(bmi)); + bmi.biSize = sizeof(bmi); + bmi.biWidth = width; + bmi.biHeight = -height; + bmi.biPlanes = 1; + bmi.biBitCount = 32; + bmi.biCompression = BI_RGB; + + bm = CreateDIBSection(dc, (const BITMAPINFO *)&bmi, DIB_RGB_COLORS, &bits, NULL, 0); + DeleteDC(dc); + if (!bm) + { + WARN("Cannot create bitmap.\n"); + hr = E_FAIL; + goto free_newsrc; + } + + hr = IWICBitmapSource_CopyPixels(newsrc, NULL, width * 4, width * height * 4, bits); + if (FAILED(hr)) + { + DeleteObject(bm); + goto free_newsrc; + } + + hr = S_OK; + *gdibitmap = bm; + +free_newsrc: + IWICBitmapSource_Release(newsrc); +done: + return hr; +} + static HRESULT WINAPI ShellItem_IShellItemImageFactory_GetImage(IShellItemImageFactory *iface, SIZE size, SIIGBF flags, HBITMAP *phbm) { ShellItem *This = impl_from_IShellItemImageFactory(iface); + HRESULT hr; + IWICImagingFactory *imgfactory; + IWICBitmap *bitmap = NULL; static int once; if (!once++) - FIXME("%p ({%lu, %lu} %d %p): stub\n", This, size.cx, size.cy, flags, phbm); + FIXME("%p ({%lu, %lu} %d %p): partial stub\n", This, size.cx, size.cy, flags, phbm); *phbm = NULL; - return E_NOTIMPL; + + if (flags != SIIGBF_BIGGERSIZEOK) return E_NOTIMPL; + + hr = WICCreateImagingFactory_Proxy(WINCODEC_SDK_VERSION, &imgfactory); + if (SUCCEEDED(hr)) + { + hr = ShellItem_get_icon_bitmap(This, imgfactory, size, flags, &bitmap); + if (SUCCEEDED(hr)) + { + hr = convert_wicbitmapsource_to_gdi(imgfactory, (IWICBitmapSource *)bitmap, phbm); + IWICBitmap_Release(bitmap); + } + IWICImagingFactory_Release(imgfactory); + } + + return hr; } static const IShellItemImageFactoryVtbl ShellItem_IShellItemImageFactory_Vtbl = { diff --git a/dlls/shell32/tests/shlfolder.c b/dlls/shell32/tests/shlfolder.c index b690e979ab5..50964a5768d 100644 --- a/dlls/shell32/tests/shlfolder.c +++ b/dlls/shell32/tests/shlfolder.c @@ -4457,7 +4457,6 @@ static void test_IShellItemImageFactory(void) ret = IShellItemImageFactory_GetImage(siif, size, SIIGBF_BIGGERSIZEOK, &hbm); IShellItemImageFactory_Release(siif); - todo_wine ok(ret == S_OK, "GetImage returned %lx\n", ret); ok(FAILED(ret) == !hbm, "result = %lx but bitmap = %p\n", ret, hbm); -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113479 Your paranoid android. === w1064v1809 (32 bit report) === shell32: shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064 (32 bit report) === shell32: shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064v1809 (64 bit report) === shell32: shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064 (64 bit report) === shell32: shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 40000) === w1064_2qxl (64 bit report) === shell32: shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_ar (64 bit report) === shell32: shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 10) shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_he (64 bit report) === shell32: shlfolder.c:4960: Test failed: MKDIR: expected notification type 8, got: 40000 shlfolder.c:4967: Test failed: GetDisplayNameOf failed: 0x80070057 shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_ja (64 bit report) === shell32: shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3) === w10pro64_zh_CN (64 bit report) === shell32: shlfolder.c:4960: Test failed: MKDIR: expected notification type 8, got: 40000 shlfolder.c:4967: Test failed: GetDisplayNameOf failed: 0x80070057 shlfolder.c:4977: Test failed: Didn't expect a WM_USER_NOTIFY message (event: 3)
I think this needs a lot of cleanup. On 4/26/22 21:17, Jinoh Kang wrote:
+ typedef struct _ShellItem { IShellItem2 IShellItem2_iface; LONG ref; @@ -565,17 +568,205 @@ static ULONG WINAPI ShellItem_IShellItemImageFactory_Release(IShellItemImageFact return IShellItem2_Release(&This->IShellItem2_iface); }
+static HRESULT ShellItem_get_icons(ShellItem *This, SIZE size, HICON *big_icon, HICON *small_icon)
You don't really need two icons, for SIIGBF_BIGGERSIZEOK.
+{ + HRESULT hr; + IBindCtx *pbc; + IExtractIconW *ei; + WCHAR icon_file[MAX_PATH]; + INT source_index; + UINT gil_in_flags = 0, gil_out_flags; + INT iconsize; + + iconsize = min(size.cx, size.cy); + if (iconsize <= 0 || iconsize > 0x7fff) + iconsize = 0x7fff; + + hr = CreateBindCtx(0, &pbc); + if (FAILED(hr)) goto done; + + hr = IShellItem2_BindToHandler(&This->IShellItem2_iface, pbc, &BHID_SFUIObject, + &IID_IExtractIconW, (void **)&ei); + IBindCtx_Release(pbc); + if (FAILED(hr)) goto done; + + hr = IExtractIconW_GetIconLocation(ei, gil_in_flags, icon_file, MAX_PATH, &source_index, &gil_out_flags); + if (FAILED(hr)) goto free_ei;
You probably can get rid of gil_in_flags and pbc.
+ + if (!(gil_out_flags & GIL_NOTFILENAME)) + { + UINT ei_res; + + if (source_index == -1) + source_index = 0; /* special case for some control panel applications */ + + FIXME("%s %d\n", debugstr_w(icon_file), source_index); + ei_res = ExtractIconExW(icon_file, source_index, big_icon, small_icon, 1); + if (!ei_res || ei_res == (UINT)-1) + { + WARN("Failed to extract icon.\n"); + hr = E_FAIL; + }
Instead of this you can probably do SHGetFileInfo(SHGFI_SYSICONINDEX), and then pick appropriate imagelist.
+ } + else + { + hr = IExtractIconW_Extract(ei, icon_file, source_index, big_icon, small_icon, MAKELONG(iconsize, iconsize)); + } + +free_ei: + IExtractIconW_Release(ei); +done: + return hr; +} + +static HICON choose_best_icon(HICON *icons, UINT count, SIZE size_limit, SIZE *out_size) +{ + HICON best_icon = NULL; + SIZE best_size = {0, 0}; + UINT i; + + for (i = 0; i < count; i++) + { + ICONINFO iinfo; + BITMAP bm; + SIZE size; + BOOL is_color, ret; + + if (!icons[i] || !GetIconInfo(icons[i], &iinfo)) continue; + + is_color = iinfo.hbmColor != NULL; + ret = GetObjectW(is_color ? iinfo.hbmColor : iinfo.hbmMask, sizeof(bm), &bm); + DeleteObject(iinfo.hbmColor); + DeleteObject(iinfo.hbmMask); + if (!ret) continue; + + size.cx = bm.bmWidth; + size.cy = is_color ? abs(bm.bmHeight) : abs(bm.bmHeight) / 2; + + if (!best_icon || (best_size.cx < size.cx && size.cx <= size_limit.cx && + best_size.cy < size.cy && size.cy <= size_limit.cy)) + { + best_icon = icons[i]; + best_size = size; + } + } + + *out_size = best_size; + return best_icon; +}
This looks too complicated. System imaglists come in few sizes. Desired size is know on GetImage(), I think it make sense to look for exact match first, and load that. For non-file based case Extract() has size argument on its own, should that do something to pick "the best" size?
+ +static HRESULT ShellItem_get_icon_bitmap(ShellItem *This, IWICImagingFactory *imgfactory, + SIZE size, SIIGBF flags, IWICBitmap **bitmap) +{ + HRESULT hr; + HICON icons[2] = { NULL, NULL }, best_icon; + SIZE best_icon_size; + UINT i; + + *bitmap = NULL; + + hr = ShellItem_get_icons(This, size, &icons[0], &icons[1]); + if (FAILED(hr)) return hr; + + best_icon = choose_best_icon(icons, ARRAY_SIZE(icons), size, &best_icon_size); + for (i = 0; i < ARRAY_SIZE(icons); i++) + if (icons[i] && icons[i] != best_icon) DeleteObject(icons[i]); + + if (!best_icon) return E_FAIL; + + hr = IWICImagingFactory_CreateBitmapFromHICON(imgfactory, best_icon, bitmap); + DeleteObject(best_icon); + return hr; +} + +static HRESULT convert_wicbitmapsource_to_gdi(IWICImagingFactory *imgfactory, + IWICBitmapSource *bitmapsource, HBITMAP *gdibitmap) +{ + BITMAPINFOHEADER bmi; + HRESULT hr; + UINT width, height; + IWICBitmapSource *newsrc; + HDC dc; + HBITMAP bm; + void *bits; + + *gdibitmap = NULL; + + hr = WICConvertBitmapSource(&GUID_WICPixelFormat32bppBGRA, bitmapsource, &newsrc); + if (FAILED(hr)) goto done; + + hr = IWICBitmapSource_GetSize(newsrc, &width, &height); + if (FAILED(hr)) goto free_newsrc; + + dc = CreateCompatibleDC(NULL); + if (!dc) + { + hr = E_FAIL; + goto free_newsrc; + } + + memset(&bmi, 0, sizeof(bmi)); + bmi.biSize = sizeof(bmi); + bmi.biWidth = width; + bmi.biHeight = -height; + bmi.biPlanes = 1; + bmi.biBitCount = 32; + bmi.biCompression = BI_RGB; + + bm = CreateDIBSection(dc, (const BITMAPINFO *)&bmi, DIB_RGB_COLORS, &bits, NULL, 0); + DeleteDC(dc); I don't think you need a device context for this.
+ if (!bm) + { + WARN("Cannot create bitmap.\n"); + hr = E_FAIL; + goto free_newsrc; + } + + hr = IWICBitmapSource_CopyPixels(newsrc, NULL, width * 4, width * height * 4, bits); + if (FAILED(hr)) + { + DeleteObject(bm); + goto free_newsrc; + } + + hr = S_OK; + *gdibitmap = bm; + +free_newsrc: + IWICBitmapSource_Release(newsrc); +done: + return hr; +} + static HRESULT WINAPI ShellItem_IShellItemImageFactory_GetImage(IShellItemImageFactory *iface, SIZE size, SIIGBF flags, HBITMAP *phbm) { ShellItem *This = impl_from_IShellItemImageFactory(iface); + HRESULT hr; + IWICImagingFactory *imgfactory; + IWICBitmap *bitmap = NULL; static int once;
if (!once++) - FIXME("%p ({%lu, %lu} %d %p): stub\n", This, size.cx, size.cy, flags, phbm); + FIXME("%p ({%lu, %lu} %d %p): partial stub\n", This, size.cx, size.cy, flags, phbm);
*phbm = NULL; - return E_NOTIMPL; + + if (flags != SIIGBF_BIGGERSIZEOK) return E_NOTIMPL; + + hr = WICCreateImagingFactory_Proxy(WINCODEC_SDK_VERSION, &imgfactory); + if (SUCCEEDED(hr)) + { + hr = ShellItem_get_icon_bitmap(This, imgfactory, size, flags, &bitmap); + if (SUCCEEDED(hr)) + { + hr = convert_wicbitmapsource_to_gdi(imgfactory, (IWICBitmapSource *)bitmap, phbm); + IWICBitmap_Release(bitmap); + } + IWICImagingFactory_Release(imgfactory); + } + + return hr; }
On 4/29/22 23:44, Nikolay Sivov wrote:
I think this needs a lot of cleanup.
On 4/26/22 21:17, Jinoh Kang wrote:
+ typedef struct _ShellItem { IShellItem2 IShellItem2_iface; LONG ref; @@ -565,17 +568,205 @@ static ULONG WINAPI ShellItem_IShellItemImageFactory_Release(IShellItemImageFact return IShellItem2_Release(&This->IShellItem2_iface); } +static HRESULT ShellItem_get_icons(ShellItem *This, SIZE size, HICON *big_icon, HICON *small_icon)
You don't really need two icons, for SIIGBF_BIGGERSIZEOK.
SIIGBF_BIGGERSIZEOK does not appear to signify that the size parameter can be entirely disregarded. Windows still takes the size argument as a hint. Maybe it needs more testing, but the impression I got is that it would choose the image that will experience the *least* distortion should it be eventually resized to the given size. It would then actually perform the resize if SIIGBF_RESIZETOFIT is specified; otherwise, the application is supposed to do the resize as needed. Although returning an image of arbitrary size isn't *contractually wrong* per se, it may choose a worse candidate for shrinking: for example, icons tend to have the same text size in both versions; thus, resizing a bigger icon would render the text unreadable. Also, it doesn't match Windows behavior anyway.
+{ + HRESULT hr; + IBindCtx *pbc; + IExtractIconW *ei; + WCHAR icon_file[MAX_PATH]; + INT source_index; + UINT gil_in_flags = 0, gil_out_flags; + INT iconsize; + + iconsize = min(size.cx, size.cy); + if (iconsize <= 0 || iconsize > 0x7fff) + iconsize = 0x7fff; + + hr = CreateBindCtx(0, &pbc); + if (FAILED(hr)) goto done; + + hr = IShellItem2_BindToHandler(&This->IShellItem2_iface, pbc, &BHID_SFUIObject, + &IID_IExtractIconW, (void **)&ei); + IBindCtx_Release(pbc); + if (FAILED(hr)) goto done; + + hr = IExtractIconW_GetIconLocation(ei, gil_in_flags, icon_file, MAX_PATH, &source_index, &gil_out_flags); + if (FAILED(hr)) goto free_ei;
You probably can get rid of gil_in_flags and pbc.
For gil_in_flags: ACK. I just wanted to clarify the purpose of the parameter; maybe it wasn't a good idea to unnecessarily make a constant-as-of-now a variable after all. For pbc: While the IBindCtx is currently ignored, wouldn't it make sense to pass something valid as required by the BindToHandler's interface definition anyway?
+ + if (!(gil_out_flags & GIL_NOTFILENAME)) + { + UINT ei_res; + + if (source_index == -1) + source_index = 0; /* special case for some control panel applications */ + + FIXME("%s %d\n", debugstr_w(icon_file), source_index); + ei_res = ExtractIconExW(icon_file, source_index, big_icon, small_icon, 1); + if (!ei_res || ei_res == (UINT)-1) + { + WARN("Failed to extract icon.\n"); + hr = E_FAIL; + }
Instead of this you can probably do SHGetFileInfo(SHGFI_SYSICONINDEX), and then pick appropriate imagelist.
That would preclude GIL_NOTFILENAME in Wine's current implementation; maybe it doesn't matter that much anyway...
+ } + else + { + hr = IExtractIconW_Extract(ei, icon_file, source_index, big_icon, small_icon, MAKELONG(iconsize, iconsize)); + } + +free_ei: + IExtractIconW_Release(ei); +done: + return hr; +} + +static HICON choose_best_icon(HICON *icons, UINT count, SIZE size_limit, SIZE *out_size) +{ + HICON best_icon = NULL; + SIZE best_size = {0, 0}; + UINT i; + + for (i = 0; i < count; i++) + { + ICONINFO iinfo; + BITMAP bm; + SIZE size; + BOOL is_color, ret; + + if (!icons[i] || !GetIconInfo(icons[i], &iinfo)) continue; + + is_color = iinfo.hbmColor != NULL; + ret = GetObjectW(is_color ? iinfo.hbmColor : iinfo.hbmMask, sizeof(bm), &bm); + DeleteObject(iinfo.hbmColor); + DeleteObject(iinfo.hbmMask); + if (!ret) continue; + + size.cx = bm.bmWidth; + size.cy = is_color ? abs(bm.bmHeight) : abs(bm.bmHeight) / 2; + + if (!best_icon || (best_size.cx < size.cx && size.cx <= size_limit.cx && + best_size.cy < size.cy && size.cy <= size_limit.cy)) + { + best_icon = icons[i]; + best_size = size; + } + } + + *out_size = best_size; + return best_icon; +}
This looks too complicated. System imaglists come in few sizes.
Not all icons belong to the system imagelist, are they?
Desired size is know on GetImage(), I think it make sense to look for exact match first, and load that.
I don't see how that would simplify the algorithm. An exact match may not always exist, but only subpar candidates.
For non-file based case Extract() has size argument on its own, should that do something to pick "the best" size?
My intention was not to rely on Extract() to actually do what we want, even as we provide the hint.
+ +static HRESULT ShellItem_get_icon_bitmap(ShellItem *This, IWICImagingFactory *imgfactory, + SIZE size, SIIGBF flags, IWICBitmap **bitmap) +{ + HRESULT hr; + HICON icons[2] = { NULL, NULL }, best_icon; + SIZE best_icon_size; + UINT i; + + *bitmap = NULL; + + hr = ShellItem_get_icons(This, size, &icons[0], &icons[1]); + if (FAILED(hr)) return hr; + + best_icon = choose_best_icon(icons, ARRAY_SIZE(icons), size, &best_icon_size); + for (i = 0; i < ARRAY_SIZE(icons); i++) + if (icons[i] && icons[i] != best_icon) DeleteObject(icons[i]); + + if (!best_icon) return E_FAIL; + + hr = IWICImagingFactory_CreateBitmapFromHICON(imgfactory, best_icon, bitmap); + DeleteObject(best_icon); + return hr; +} + +static HRESULT convert_wicbitmapsource_to_gdi(IWICImagingFactory *imgfactory, + IWICBitmapSource *bitmapsource, HBITMAP *gdibitmap) +{ + BITMAPINFOHEADER bmi; + HRESULT hr; + UINT width, height; + IWICBitmapSource *newsrc; + HDC dc; + HBITMAP bm; + void *bits; + + *gdibitmap = NULL; + + hr = WICConvertBitmapSource(&GUID_WICPixelFormat32bppBGRA, bitmapsource, &newsrc); + if (FAILED(hr)) goto done; + + hr = IWICBitmapSource_GetSize(newsrc, &width, &height); + if (FAILED(hr)) goto free_newsrc; + + dc = CreateCompatibleDC(NULL); + if (!dc) + { + hr = E_FAIL; + goto free_newsrc; + } + + memset(&bmi, 0, sizeof(bmi)); + bmi.biSize = sizeof(bmi); + bmi.biWidth = width; + bmi.biHeight = -height; + bmi.biPlanes = 1; + bmi.biBitCount = 32; + bmi.biCompression = BI_RGB; + + bm = CreateDIBSection(dc, (const BITMAPINFO *)&bmi, DIB_RGB_COLORS, &bits, NULL, 0); + DeleteDC(dc); I don't think you need a device context for this.
ACK. TIL that CreateDIBSection(NULL, ..., DIB_RGB_COLORS, ...) is legal.
+ if (!bm) + { + WARN("Cannot create bitmap.\n"); + hr = E_FAIL; + goto free_newsrc; + } + + hr = IWICBitmapSource_CopyPixels(newsrc, NULL, width * 4, width * height * 4, bits); + if (FAILED(hr)) + { + DeleteObject(bm); + goto free_newsrc; + } + + hr = S_OK; + *gdibitmap = bm; + +free_newsrc: + IWICBitmapSource_Release(newsrc); +done: + return hr; +} + static HRESULT WINAPI ShellItem_IShellItemImageFactory_GetImage(IShellItemImageFactory *iface, SIZE size, SIIGBF flags, HBITMAP *phbm) { ShellItem *This = impl_from_IShellItemImageFactory(iface); + HRESULT hr; + IWICImagingFactory *imgfactory; + IWICBitmap *bitmap = NULL; static int once; if (!once++) - FIXME("%p ({%lu, %lu} %d %p): stub\n", This, size.cx, size.cy, flags, phbm); + FIXME("%p ({%lu, %lu} %d %p): partial stub\n", This, size.cx, size.cy, flags, phbm); *phbm = NULL; - return E_NOTIMPL; + + if (flags != SIIGBF_BIGGERSIZEOK) return E_NOTIMPL; + + hr = WICCreateImagingFactory_Proxy(WINCODEC_SDK_VERSION, &imgfactory); + if (SUCCEEDED(hr)) + { + hr = ShellItem_get_icon_bitmap(This, imgfactory, size, flags, &bitmap); + if (SUCCEEDED(hr)) + { + hr = convert_wicbitmapsource_to_gdi(imgfactory, (IWICBitmapSource *)bitmap, phbm); + IWICBitmap_Release(bitmap); + } + IWICImagingFactory_Release(imgfactory); + } + + return hr; }
-- Sincerely, Jinoh Kang
participants (3)
-
Jinoh Kang -
Marvin -
Nikolay Sivov