Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SHELL32][USER32] Fix icon regression and one test #5207

Merged
merged 5 commits into from
May 21, 2023
Merged

[SHELL32][USER32] Fix icon regression and one test #5207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ed95d0e
[WIN32SS] Fix icon regression and tests
Doug-Lyons Mar 30, 2023
1c32342
Changes based on reviewer comments.
Doug-Lyons Apr 25, 2023
27dbbba
Update win32ss/user/user32/misc/exticon.c
Doug-Lyons Apr 25, 2023
a7101f1
Changes based on reviewer comments.
Doug-Lyons May 11, 2023
9e651a1
Merge branch 'icon_fixes' of https://github.com/Doug-Lyons/reactos in…
Doug-Lyons May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion dll/win32/shell32/folders/CFSFolder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ HRESULT CFSExtractIcon_CreateInstance(IShellFolder * psf, LPCITEMIDLIST pidl, RE
ILGetDisplayNameExW(psf, pidl, wTemp, ILGDN_FORPARSING);
icon_idx = 0;

INT ret = ExtractIconExW(wTemp, -1, NULL, NULL, 0);
INT ret = PrivateExtractIconsW(wTemp, 0, 0, 0, NULL, NULL, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other ExtractIconExW() calls in shell32 (and elsewhere).
Does only this one need to be changed? (Why?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the problem in https://jira.reactos.org/browse/CORE-18897 you can see that some icons are shown correctly and others are shown with the IDI_SHELL_DOCUMENT default icon. I found that the problem icons were those that had a PNG icon embedded in them. So I created a new ROS.ico file with the properties of having one normal BMP icon and one PNG encoded icon in it. Then I looked at the tests for shell32:ExtractIconEx and user32:PrivateExtractIcons.
I wrote more tests to include the new ROS.ico in PR #5169 and PR #5209. My findings were that the expected result from this function needed to be more in line with PrivateExtractIcons because it returned "1" on the problematic icons (ROS.ico) which kept the default document icon from being applied in the code just below. (Backed by Tests)

if (ret <= 0)
{
StringCbCopyW(wTemp, sizeof(wTemp), swShell32Name);
Expand Down
67 changes: 67 additions & 0 deletions win32ss/user/user32/misc/exticon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ typedef struct
DWORD resloader;
} NE_TYPEINFO;

#ifdef __REACTOS__
// From: James Houghtaling
// https://www.moon-soft.com/program/FORMAT/windows/ani.htm
typedef struct taganiheader
Expand All @@ -85,6 +86,7 @@ typedef struct taganiheader
DWORD jifrate; // default jiffies (1/60th sec) if rate chunk not present.
DWORD flags; // animation flag
} aniheader;
#endif

#define NE_RSCTYPE_ICON 0x8003
#define NE_RSCTYPE_GROUP_ICON 0x800e
Expand Down Expand Up @@ -283,7 +285,19 @@ static UINT ICO_ExtractIconExW(
UINT cxDesired,
UINT cyDesired,
UINT *pIconId,
#ifdef __REACTOS__
UINT flags,
/* This function is called from two different code paths.
* One is from Shell32 using the ExtractIconEx function.
* The other is from User32 using PrivateExtractIcons.
* Based on W2K3SP2 testing, the count of icons returned
* is zero (0) for PNG ones using ExtractIconEx and
* one (1) for PNG icons using PrivateExtractIcons.
* We can handle the difference using the fIconEx flag.*/
BOOL fIconEx)
#else
UINT flags)
#endif
{
UINT ret = 0;
UINT cx1, cx2, cy1, cy2;
Expand All @@ -300,7 +314,11 @@ static UINT ICO_ExtractIconExW(
WCHAR szExePath[MAX_PATH];
DWORD dwSearchReturn;

#ifdef __REACTOS__
TRACE("%s, %d, %d, %p, 0x%08x, %d\n", debugstr_w(lpszExeFileName), nIconIndex, nIcons, pIconId, flags, fIconEx);
#else
TRACE("%s, %d, %d %p 0x%08x\n", debugstr_w(lpszExeFileName), nIconIndex, nIcons, pIconId, flags);
#endif

#ifdef __REACTOS__
if (RetPtr)
Expand All @@ -313,8 +331,17 @@ static UINT ICO_ExtractIconExW(
dwSearchReturn = SearchPathW(NULL, lpszExeFileName, NULL, ARRAY_SIZE(szExePath), szExePath, NULL);
if ((dwSearchReturn == 0) || (dwSearchReturn > ARRAY_SIZE(szExePath)))
{
#ifdef __REACTOS__
WARN("File %s not found or path too long and fIconEx is '%d'\n",
debugstr_w(lpszExeFileName), fIconEx);
if (fIconEx && !RetPtr && !pIconId)
return 0;
else
return -1;
#else
WARN("File %s not found or path too long\n", debugstr_w(lpszExeFileName));
return -1;
#endif
}

hFile = CreateFileW(szExePath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0);
Expand Down Expand Up @@ -576,6 +603,8 @@ static UINT ICO_ExtractIconExW(

#ifdef __REACTOS__
icon = CreateIconFromResourceEx(imageData, cbTotal, sig == 1, 0x00030000, cx[index], cy[index], flags);
if (fIconEx && sig == 1)
iconCount = 1;
#else
icon = CreateIconFromResourceEx(imageData, entry->icHeader.biSizeImage, sig == 1, 0x00030000, cx[index], cy[index], flags);
#endif
Expand Down Expand Up @@ -616,6 +645,15 @@ static UINT ICO_ExtractIconExW(
goto end;
}

#ifdef __REACTOS__
/* Check for boundary limit (and overflow) */
if (((ULONG_PTR)(rootresdir + 1) < (ULONG_PTR)rootresdir) ||
((ULONG_PTR)(rootresdir + 1) > (ULONG_PTR)peimage + fsizel))
{
goto end;
}
#endif

/* search for the group icon directory */
if (!(icongroupresdir = find_entry_by_id(rootresdir, LOWORD(RT_GROUP_ICON), rootresdir)))
{
Expand Down Expand Up @@ -764,7 +802,12 @@ UINT WINAPI PrivateExtractIconsW (
{
WARN("Uneven number %d of icons requested for small and large icons!\n", nIcons);
}
#ifdef __REACTOS__
return ICO_ExtractIconExW(lpwstrFile, phicon, nIndex, nIcons, sizeX, sizeY,
pIconId, flags, TRUE);
#else
return ICO_ExtractIconExW(lpwstrFile, phicon, nIndex, nIcons, sizeX, sizeY, pIconId, flags);
#endif
}

/***********************************************************************
Expand Down Expand Up @@ -814,9 +857,16 @@ UINT WINAPI PrivateExtractIconExW (
TRACE("%s %d %p %p %d\n",
debugstr_w(lpwstrFile),nIndex,phIconLarge, phIconSmall, nIcons);

#ifdef __REACTOS__
if (nIndex == -1 || (!phIconSmall && !phIconLarge))
/* get the number of icons */
JoachimHenze marked this conversation as resolved.
Show resolved Hide resolved
return ICO_ExtractIconExW(lpwstrFile, NULL, 0, 0, 0, 0, NULL,
LR_DEFAULTCOLOR, FALSE);
#else
if (nIndex == -1)
/* get the number of icons */
return ICO_ExtractIconExW(lpwstrFile, NULL, 0, 0, 0, 0, NULL, LR_DEFAULTCOLOR);
#endif

if (nIcons == 1 && phIconSmall && phIconLarge)
{
Expand All @@ -826,8 +876,15 @@ UINT WINAPI PrivateExtractIconExW (
cxsmicon = GetSystemMetrics(SM_CXSMICON);
cysmicon = GetSystemMetrics(SM_CYSMICON);

#ifdef __REACTOS__
Doug-Lyons marked this conversation as resolved.
Show resolved Hide resolved
ret = ICO_ExtractIconExW(lpwstrFile, hIcon, nIndex, 2,
cxicon | (cxsmicon<<16),
cyicon | (cysmicon<<16), NULL,
LR_DEFAULTCOLOR, FALSE);
#else
ret = ICO_ExtractIconExW(lpwstrFile, hIcon, nIndex, 2, cxicon | (cxsmicon<<16),
cyicon | (cysmicon<<16), NULL, LR_DEFAULTCOLOR);
#endif
*phIconLarge = hIcon[0];
*phIconSmall = hIcon[1];
return ret;
Expand All @@ -838,16 +895,26 @@ UINT WINAPI PrivateExtractIconExW (
/* extract n small icons */
cxsmicon = GetSystemMetrics(SM_CXSMICON);
cysmicon = GetSystemMetrics(SM_CYSMICON);
#ifdef __REACTOS__
ret = ICO_ExtractIconExW(lpwstrFile, phIconSmall, nIndex, nIcons, cxsmicon,
cysmicon, NULL, LR_DEFAULTCOLOR, FALSE);
#else
ret = ICO_ExtractIconExW(lpwstrFile, phIconSmall, nIndex, nIcons, cxsmicon,
cysmicon, NULL, LR_DEFAULTCOLOR);
#endif
}
if (phIconLarge)
{
/* extract n large icons */
cxicon = GetSystemMetrics(SM_CXICON);
cyicon = GetSystemMetrics(SM_CYICON);
#ifdef __REACTOS__
Doug-Lyons marked this conversation as resolved.
Show resolved Hide resolved
ret = ICO_ExtractIconExW(lpwstrFile, phIconLarge, nIndex, nIcons, cxicon,
cyicon, NULL, LR_DEFAULTCOLOR, FALSE);
#else
ret = ICO_ExtractIconExW(lpwstrFile, phIconLarge, nIndex, nIcons, cxicon,
cyicon, NULL, LR_DEFAULTCOLOR);
#endif
}
return ret;
}
Expand Down