[PATCH v7 3/3] shell32: Partially implement IShellItemImageFactory (icon only, no thumbnail).
Jinoh Kang
jinoh.kang.kr at gmail.com
Fri Apr 29 12:38:55 CDT 2022
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
More information about the wine-devel
mailing list