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.

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

[USER32] Hackfix for CORE-17902 - The cursoricon (copyimage) that returns NULL to render - LR_COPYFROMRESOURCE #6886

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

julenuri
Copy link
Contributor

@julenuri julenuri commented May 14, 2024

Hackfix for CORE-17902 - The cursoricon (copyimage) that returns NULL to render - LR_COPYFROMRESOURCE

Purpose

In this case, the aim is to render some VB5/6 software icons to show up. Those elements are icons rendered in a way that is common under Visual Basic icons loaded from a .ocx .

ThunderRT6FormDC->ThunderRT6UserControlDC->RebarWindows32->Toolbar20WndClass->msvb_lib_toolbar

Quoting Simone:

Unable to render embedded object: File (ReactOS_workaround) not found.

it is making a CopyImage call with the LR_COPYFROMRESOURCE|LB_COPYRETURNORG flags. All of these are returning NULL.
LB_COPYRETURNORG flag is only managed by bitmap copy function (BITMAP_CopyImage), not by the cursoricon copy function (CURSORICON_CopyImage) but it's not the critical one.
The critical flag is the LR_COPYFROMRESOURCE.

More info here: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-copyimage

By making it to return the original bitmap in the case of failure, makes the icon shown as expected.
The issue so resides on application of api ("tries to" means "tries to" so the fallback to return the original image isneeded), or there is a failure on child NtUserGetIconInfo.

Proposed changes

Minor (fix? or hackfix?) that should make it to work.

BEFORE:
Before

AFTER:
After

TODO

  • Investigate if the behavior is correct comparing to Windows.
  • Investigate if there is any better way to implement it/or if there are similar failures under a more simple software to test.

@julenuri julenuri changed the title [USER32] Hackfix for CORE-17902 - The cursoricon (copyimage) that ret… [USER32] Hackfix for CORE-17902 - The cursoricon (copyimage) that returns NULL to render - LR_COPYFROMRESOURCE May 14, 2024
@github-actions github-actions bot added the Win32SS For Win32 subsystem (Win32k, GDI/USER DLLs, etc.) related components PRs. label May 14, 2024
@oleg-dubinskiy oleg-dubinskiy added the hackfix PRs that look like hacks/workarounds for deeper problems label May 14, 2024
@binarymaster binarymaster added this to New PRs in ReactOS PRs via automation May 14, 2024
@julenuri julenuri marked this pull request as ready for review May 14, 2024 21:45
win32ss/user/user32/windows/cursoricon.c Outdated Show resolved Hide resolved
win32ss/user/user32/windows/cursoricon.c Outdated Show resolved Hide resolved
switch(uType)
{
case IMAGE_BITMAP:
return BITMAP_CopyImage(hImage, cxDesired, cyDesired, fuFlags);
case IMAGE_CURSOR:
case IMAGE_ICON:
return CURSORICON_CopyImage(hImage, uType == IMAGE_ICON, cxDesired, cyDesired, fuFlags);
handle = CURSORICON_CopyImage(hImage, uType == IMAGE_ICON, cxDesired, cyDesired, fuFlags);
if(handle == NULL && (fuFlags & (LR_COPYFROMRESOURCE|LR_COPYRETURNORG)))
Copy link
Contributor

Choose a reason for hiding this comment

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

When you run the VB-based app in ReactOS, which flag(s) amongst these two are being specified? Both? or only one of these?

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition I would suggest you add here in the code a // HACK or // HACKFIX comment explaining what's happening, with the CORE-xxxx number mentioned too.

I also wonder whether this hack could be added instead into the CURSORICON_CopyImage function instead? In any case I guess running the test bots on that would be necessary.

@@ -2031,7 +2031,15 @@ HANDLE WINAPI CopyImage(
return BITMAP_CopyImage(hImage, cxDesired, cyDesired, fuFlags);
case IMAGE_CURSOR:
case IMAGE_ICON:
return CURSORICON_CopyImage(hImage, uType == IMAGE_ICON, cxDesired, cyDesired, fuFlags);
/* HACK: Returning the old bitmap in LR_COPYFROMRESOURCE caused
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* HACK: Returning the old bitmap in LR_COPYFROMRESOURCE caused
// HACK: Copying bitmaps with LR_COPYFROMRESOURCE flag fails. CORE-17902.

ReactOS PRs automation moved this from New PRs to Approved by reviewers May 21, 2024
…urns NULL to render - LR_COPYFROMRESOURCE

[USER32] Fix the previous commit based on the suggested comments

[USER32] Minor fixes and fixing identation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hackfix PRs that look like hacks/workarounds for deeper problems Win32SS For Win32 subsystem (Win32k, GDI/USER DLLs, etc.) related components PRs.
Projects
ReactOS PRs
  
Approved by reviewers
4 participants