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

Doug-Lyons · 2023-03-30T04:29:41Z

Fix icon regression of `cfdbccf` and fix a shell32:ExtractIconEx test.

Even more failing tests will be fixed after #5169 is committed.

Modify some return values in exticon.c based on from where they are called.

Allow different icon count return values from shell32:ExtractIconEx and user32:PrivateExtractIcons.

Change some return values in common function ICO_ExtractIconExW based on from where they are called.

JoachimHenze · 2023-03-30T11:37:44Z

The bug that you do intend to fix here... does that happen also on Wine? If not, why not?
It seems as if parts or most of exticon.c was taken from Wine.

win32ss/user/user32/misc/exticon.c

HBelusca

Do we also have tests (maybe in an apitest or winetest for user32) for the PrivateExtractIconsW and PrivateExtractIconExW (undocumented) functions?

win32ss/user/user32/misc/exticon.c

SergeGautherie · 2023-03-30T14:09:53Z

dll/win32/shell32/folders/CFSFolder.cpp

@@ -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);


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

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)

SergeGautherie · 2023-03-30T14:14:54Z

win32ss/user/user32/misc/exticon.c

+     * is zero (0) for PNG ones using ExtractIconEx and
+     * one (1) for PNG icons using PrivateExtractIcons. 
+     * We can handle the difference using the fUser32 flag.*/
+    BOOL fUser32)


This explanation does not look fully clear to me:
your code difference seems to be between PrivateExtractIconsW() and PrivateExtractIconsExW(),
not ExtractIconExW() and PrivateExtractIconsW().
Then, param name looks somewhat odd too.

Please review comments elsewhere from you regarding:
There are other ExtractIconExW() calls in shell32 (and elsewhere).
Does only this one need to be changed? (Why?)

and from @HBeluse regarding:
Do we also have tests (maybe in an apitest or winetest for user32) for the PrivateExtractIconsW and PrivateExtractIconExW (undocumented) functions?

You can let me know if you still have questions. Thanks.

win32ss/user/user32/misc/exticon.c

Doug-Lyons · 2023-03-30T16:28:27Z

The bug that you do intend to fix here... does that happen also on Wine? If not, why not? It seems as if parts or most of exticon.c was taken from Wine.

I am not sure how to know if it happens on Wine. When I run explorer.exe on wine, I see the following on my 'icon' directory. It seems that nothing is shown at all for the icon image other than a default. On second thought, I think that with the present code, that is what is expected before these fixes.

Doug-Lyons · 2023-03-30T16:59:39Z

Do we also have tests (maybe in an apitest or winetest for user32) for the PrivateExtractIconsW and PrivateExtractIconExW (undocumented) functions?

We do have user32_apitest.exe for PrivateExtractIconsEx(W) and soon I will have a PR for additional tests for this. This is a documented API. For the PrivateExtractIconExW this is not a documented API, but only a ReactOS function, so there is probably no need for tests on it. It is actually the endpoint for the documented API for ExtractIconEx which is tested by our shell32_apitest:ExtractIconEx with more recent tests after the merge of my PR. The calls for ExtractIconEx are as follows:

Shell32 has ExtractIconExW in dll/win32/shell32/iconcache.cpp at about Line # 866. This calls PrivateExtractIconExW in win32/user/user32/misc/exticon.c at about Line# 884 using a GetProcAddress on User32.dll. PrivateExtractIconExW then calls ICO_ExtractIconExW at about existing Line# 838 in exticon.c.

The common function that is called from both Shell32 and User32 is ICO_ExtractIconExW in exticon.c. It either gets called from the API functions of ExtractIconEx(W) (in Shell32) or PrivateExtractIcons(W) (in User32). This was the reason for the 'fUser32' flag name. I am open to any suggestion for a new name for it if you please. Thanks.

win32ss/user/user32/misc/exticon.c

Co-authored-by: Joachim Henze <joachim.henze@reactos.org>

…to icon_fixes

learn-more · 2023-05-21T09:26:32Z

Thanks for your contribution!

…RE-18897 by porting back: 0.4.15-dev-6030-g 8a7b5a9 [SHELL32][USER32] Fix icon regression and one test (#5207) CORE-18897 It regressed by 0.4.14-dev-459-g cfdbccf I actually am not a huge fan of the code-flow-splitup and all those ifdefs, but it fixes the regression. Therefore I will not port that back further than releases/0.4.14 for now. ------------ Also partially pick 0.4.15-dev-3642-g 83be315 [SHELL32] Use wide char string literals. (only the part in CFSFolder.cpp) I might pick the other parts later. ------------ Binary size remains under control: 0.4.14 shell32.dll GCC4.7.2 RosBE2.1.6 dbg-cfg 9.345.024 -> 9.345.024 0.4.14 user32.dll GCC4.7.2 RosBE2.1.6 dbg-cfg 1.448.448 -> 1.448.448

github-actions bot added the Win32SS For Win32 subsystem (Win32k, GDI/USER DLLs, etc.) related components PRs. label Mar 30, 2023

Doug-Lyons force-pushed the icon_fixes branch from d505f05 to 47c839d Compare March 30, 2023 05:01

JoachimHenze requested review from ThFabba, tkreuzer and katahiromz March 30, 2023 11:34

JoachimHenze approved these changes Mar 30, 2023

View reviewed changes

JoachimHenze reviewed Mar 30, 2023

View reviewed changes

win32ss/user/user32/misc/exticon.c Show resolved Hide resolved

JoachimHenze reviewed Mar 30, 2023

View reviewed changes

win32ss/user/user32/misc/exticon.c Outdated Show resolved Hide resolved

JoachimHenze reviewed Mar 30, 2023

View reviewed changes

win32ss/user/user32/misc/exticon.c Outdated Show resolved Hide resolved

HBelusca reviewed Mar 30, 2023

View reviewed changes

win32ss/user/user32/misc/exticon.c Outdated Show resolved Hide resolved

HBelusca added the review pending For PRs undergoing review. label Mar 30, 2023

SergeGautherie reviewed Mar 30, 2023

View reviewed changes

ThFabba reviewed Mar 31, 2023

View reviewed changes

win32ss/user/user32/misc/exticon.c Outdated Show resolved Hide resolved

Doug-Lyons force-pushed the icon_fixes branch from baeb448 to 74f5ab5 Compare March 31, 2023 05:16

[WIN32SS] Fix icon regression and tests

ed95d0e

Doug-Lyons force-pushed the icon_fixes branch from 74f5ab5 to ed95d0e Compare March 31, 2023 05:22

Changes based on reviewer comments.

1c32342

Doug-Lyons requested review from learn-more and yagoulas as code owners April 25, 2023 12:26

github-actions bot added the shell All PR's related to the shell (and shell extensions) label Apr 25, 2023

Doug-Lyons requested review from HBelusca and ThFabba April 25, 2023 12:35

JoachimHenze reviewed Apr 25, 2023

View reviewed changes

win32ss/user/user32/misc/exticon.c Outdated Show resolved Hide resolved

Update win32ss/user/user32/misc/exticon.c

27dbbba

Co-authored-by: Joachim Henze <joachim.henze@reactos.org>

Doug-Lyons requested a review from JoachimHenze April 25, 2023 18:36

learn-more approved these changes Apr 26, 2023

View reviewed changes

JoachimHenze approved these changes Apr 26, 2023

View reviewed changes

katahiromz changed the title ~~[WIN32SS] Fix icon regression and one test~~ [SHELL32][USER32] Fix icon regression and one test May 10, 2023

Doug-Lyons added 2 commits May 10, 2023 19:51

Changes based on reviewer comments.

a7101f1

Merge branch 'icon_fixes' of https://github.com/Doug-Lyons/reactos in…

9e651a1

…to icon_fixes

katahiromz approved these changes May 11, 2023

View reviewed changes

learn-more merged commit 8a7b5a9 into reactos:master May 21, 2023
33 of 34 checks passed

Doug-Lyons deleted the icon_fixes branch May 21, 2023 10:30

binarymaster added bugfix For bugfix PRs. and removed review pending For PRs undergoing review. labels Jun 11, 2023

binarymaster added this to New PRs in ReactOS PRs via automation Jun 11, 2023

binarymaster moved this from New PRs to Done in ReactOS PRs Jun 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Doug-Lyons commented Mar 30, 2023

JoachimHenze commented Mar 30, 2023

HBelusca left a comment

SergeGautherie Mar 30, 2023

Doug-Lyons Mar 31, 2023 •

edited

SergeGautherie Mar 30, 2023

Doug-Lyons Mar 31, 2023

Doug-Lyons commented Mar 30, 2023 •

edited

Doug-Lyons commented Mar 30, 2023 •

edited

learn-more commented May 21, 2023

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

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

Conversation

Doug-Lyons commented Mar 30, 2023

Fix icon regression of cfdbccf and fix a shell32:ExtractIconEx test.

Allow different icon count return values from shell32:ExtractIconEx and user32:PrivateExtractIcons.

JoachimHenze commented Mar 30, 2023

HBelusca left a comment

Choose a reason for hiding this comment

SergeGautherie Mar 30, 2023

Choose a reason for hiding this comment

Doug-Lyons Mar 31, 2023 • edited

Choose a reason for hiding this comment

SergeGautherie Mar 30, 2023

Choose a reason for hiding this comment

Doug-Lyons Mar 31, 2023

Choose a reason for hiding this comment

Doug-Lyons commented Mar 30, 2023 • edited

Doug-Lyons commented Mar 30, 2023 • edited

learn-more commented May 21, 2023

Fix icon regression of `cfdbccf` and fix a shell32:ExtractIconEx test.

Doug-Lyons Mar 31, 2023 •

edited

Doug-Lyons commented Mar 30, 2023 •

edited

Doug-Lyons commented Mar 30, 2023 •

edited