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

[NTGDI][GDI32] Icon fixes for Office 2000, VB6, and Hoyle Cards #5227

Merged
merged 8 commits into from Jul 2, 2023

Conversation

Doug-Lyons
Copy link
Contributor

@Doug-Lyons Doug-Lyons commented Apr 6, 2023

Many thanks to Simone Lombardo for pointing to the code needing attention and providing a working proof-of-concept solution.

Purpose

Fix toolbar icons in MS Office 2000 and other programs.

JIRA issues:
CORE-12377
CORE-18084
CORE-13889

Proposed changes

Modify SetDIBitsToDevice and NtGdiSetDIBitsToDeviceInternal to correctly handle these icons.

Word 2000 Before:

Word_2K_Bad

Word 2000 After:

O-2000-Word_All_Icons

Excel 2000 Before:

Excel_2K_Bad

Excel 2000 After:

O-2000-Excel_All_Icons

VB6 Before:

VB6_Bad

VB6 After:

VB6_Good

Hoyle Cards Before:

Hoyle_Bad

Hoyle Cards After:

Hoyle_Good

Word '97 Before:

Word_97_Bad1

Word '97 After:

Word_97_Good

Excel '97 Before:

Excel_97_Bad1

Excel '97 After:

Excel_97_Good

@github-actions github-actions bot added the Win32SS For Win32 subsystem (Win32k, GDI/USER DLLs, etc.) related components PRs. label Apr 6, 2023
@HBelusca
Copy link
Contributor

HBelusca commented Apr 6, 2023

In the "after" pictures of Word/Excel, there are still buttons -- in the second toolbar (font and stuff) that now don't display anything. Any idea why this is happening?

win32ss/gdi/ntgdi/dibobj.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
@HBelusca HBelusca added the bugfix For bugfix PRs. label Apr 6, 2023
@HBelusca
Copy link
Contributor

HBelusca commented Apr 6, 2023

It would be of great interest to see whether there are existing api/winetests for those functions, and if not, add one.

@julenuri
Copy link
Contributor

julenuri commented Apr 6, 2023

In the "after" pictures of Word/Excel, there are still buttons -- in the second toolbar (font and stuff) that now don't display anything. Any idea why this is happening?

Weirdly in the ticket CORE-12377 I attached the image of those icons working with a previous WIP patch: https://jira.reactos.org/secure/attachment/61882/VirtualBox_ReactOS%20master_09_04_2022_16_29_01.png (why are they failing now?)

@Doug-Lyons
Copy link
Contributor Author

Doug-Lyons commented Apr 7, 2023

@HBelusca and @julenuri, Thanks for pointing out the missing icons. I have been concentrating on the first six or eight icons on the toolbar and did not even notice these were missing. Let me take a look at this.

Well, the answer is somewhat disappointing. @julenuri, I noticed that you were testing on Excel '97 and I was illustrating Excel 2000. I have added screen shots of both Word '97 and Excel '97 before and after and these show all good after this patch. As far as I know, ReactOS has never been able to show all the icons of Office 2000 products. This is something that Maybe Simone and I can look at further. Thanks.

@Doug-Lyons
Copy link
Contributor Author

@HBelusca and @julenuri, Thanks for pointing out the missing icons. I have been concentrating on the first six or eight icons on the toolbar and did not even notice these were missing. Let me take a look at this.

Well, the answer is somewhat disappointing. @julenuri, I noticed that you were testing on Excel '97 and I was illustrating Excel 2000. I have added screen shots of both Word '97 and Excel '97 before and after and these show all good after this patch. As far as I know, ReactOS has never been able to show all the icons of Office 2000 products. This is something that Maybe Simone and I can look at further. Thanks.

I have found a possible solution to the missing icons in the toolbars of Word 2000 and Excel 2000 and it is now included. Enjoy.

@JoachimHenze JoachimHenze changed the title [WIN32K][GDI32] Icon fixes for Office 2000, VB6, and Hoyle Cards. [NTGDI][GDI32] Icon fixes for Office 2000, VB6, and Hoyle Cards Apr 9, 2023
win32ss/gdi/eng/surface.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/eng/surface.c Outdated Show resolved Hide resolved
@jeditobe
Copy link
Contributor

CORE-13019 may have some relation too

@Doug-Lyons
Copy link
Contributor Author

CORE-13019 may have some relation too

@jeditobe, Thank you for your comments. I believe that these two issues are not related. This one is just a position translation problem where we are moving a bitmap from one (X1, Y1) position to another (X2, Y2) location. CORE-13019 that you pointed out is a transparency, ROP, or GDI Blend problem, I believe. I might be able to look into this soon though. Thanks again.

@Doug-Lyons
Copy link
Contributor Author

Current PR testbot results:

MSO_Icons_fix9.patch JID65618 on top of 0.4.15-dev-5931-g8f2c2c1

VBox: https://reactos.org/testman/compare.php?ids=86808,86820 LGTM
KVM: https://reactos.org/testman/compare.php?ids=86809,86819 LTGM

gdi32:bitmap: 374 fewer failed tests

@Doug-Lyons
Copy link
Contributor Author

Current PR testbot results:
MSO_Icons_fix12.patch JID65698 on top of 0.4.15-dev-5964-g00c69bc

VBox: https://reactos.org/testman/compare.php?ids=86941,86943 LGTM
KVM: https://reactos.org/testman/compare.php?ids=86940,86942 LGTM

gdi32:bitmap: 435 fewer failed tests
gdi32:SetDIBitsToDevice: 4 failed tests shown. All due to 'succeeded inside todo block'

@reactos reactos deleted a comment from JoachimHenze Apr 21, 2023
@reactos reactos deleted a comment from JoachimHenze Apr 21, 2023
win32ss/gdi/eng/surface.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
Many thanks for Simone Lombardo for pointing to the code needing attention
and providing a working proof-of-concept solution.

CORE-12377
@Doug-Lyons
Copy link
Contributor Author

Current PR testbot results:
MSO_Icons_fix19.patch JID65720 on top of 0.4.15-dev-5984-gaaeb131

VBox: https://reactos.org/testman/compare.php?ids=86993,86995
KVM: https://reactos.org/testman/compare.php?ids=86992,86994

gdi32:bitmap: 430 fewer failed tests
gdi32:SetDIBitsToDevice: 6 failed tests shown. 2 marked as todo's, still.
All 6 fails due to 'succeeded inside todo block', so no real fails.

I do not intend to make any further attempts to reduce the regression test failures.
This should be final until receiving feedback that probably will need changes. Thanks.

@jeditobe
Copy link
Contributor

jeditobe commented May 2, 2023

I belive this shold fix much more bugs/apps than claimed.

CORE-15536 GG Internet Communicator: Broken titlebar icon bitmap

@jeditobe
Copy link
Contributor

jeditobe commented May 3, 2023

Additional supects to be fixed by this PR
CORE-12407 Xwidget engine graphical problem (no visible icon in tray)
CORE-10726 ReactOS does not show the icon of F1 '97
CORE-18535 SwitchIt Control Panel Applet does not show its icon
CORE-18460 Task Manager loses its Taskbar icon
CORE-17902 Factusol 2002 3.0 - Buttons are not rendered/displayed
CORE-18753 KeePass 1.40.1 draws no icons in the toolbar

win32ss/gdi/ntgdi/dibobj.c Outdated Show resolved Hide resolved
win32ss/gdihelp.h Outdated Show resolved Hide resolved
@Doug-Lyons Doug-Lyons requested a review from ThFabba May 18, 2023 02:16
@ghost
Copy link

ghost commented May 18, 2023

@Doug-Lyons Do you need me to retest
CORE-18535 SwitchIt Control Panel Applet does not show its icon
CORE-18460 Task Manager loses its Taskbar icon
CORE-18753 KeePass 1.40.1 draws no icons in the toolbar
based on the latest iso of this PR or did you do it already ?

@ghost
Copy link

ghost commented May 19, 2023

CORE-18535 : Not fixed :
image

CORE-18753 : Resolved before this PR

CORE-18460 : Not fixed :
image

@julenuri
Copy link
Contributor

Additional supects to be fixed by this PR
...
CORE-17902 Factusol 2002 3.0 - Buttons are not rendered/displayed
...

That one has nothing to do with this PR, sorry.
After testing the patch under my local tweaking the access ntoskrnl se element to use the setup, my check said that this one is still failing. Sorry
VirtualBox_ReactOS_22_05_2023_20_44_31
:S

@binarymaster binarymaster added this to New PRs in ReactOS PRs via automation Jun 10, 2023
@binarymaster binarymaster moved this from New PRs to Approved by reviewers in ReactOS PRs Jun 10, 2023
@Doug-Lyons
Copy link
Contributor Author

@tkreuzer, it has been 3 weeks after your approval with no known unaddressed comments. Is there anything else that I can do to help move this along toward a commit? Thanks.

Copy link
Contributor

@HBelusca HBelusca left a comment

Choose a reason for hiding this comment

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

Final whitespace fixes...

win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
win32ss/gdi/gdi32/objects/bitmap.c Outdated Show resolved Hide resolved
@HBelusca
Copy link
Contributor

Since I see some "BI_RLE4/8` manipulations, can someone test installation of WinRAR and running some self-compressed executables made with it?
If I recall correctly (from old JIRA reports), the splash screen shown by it is RLE-encoded, and so in principle those modified code paths may be exerced now.

@SigmaTel71
Copy link
Member

Since I see some "BI_RLE4/8` manipulations, can someone test installation of WinRAR and running some self-compressed executables made with it?

I have a WinRAR self-extracting archive of QIP Infium beta 1 build 9000, so it could be used to test.

@Doug-Lyons
Copy link
Contributor Author

Since I see some "BI_RLE4/8` manipulations, can someone test installation of WinRAR and running some self-compressed executables made with it? If I recall correctly (from old JIRA reports), the splash screen shown by it is RLE-encoded, and so in principle those modified code paths may be exerced now.

@HBelusca, I believe that this was fixed in https://jira.reactos.org/browse/CORE-10553. But for testing, there is a WinRAR file using RLE4 there named empty.exe and it shows up fine with these changes. Thanks.

ReactOS-15-6164-winrar-empty

@HBelusca
Copy link
Contributor

@HBelusca, I believe that this was fixed in https://jira.reactos.org/browse/CORE-10553. But for testing, there is a WinRAR file using RLE4 there named empty.exe and it shows up fine with these changes. Thanks.

Looks good to me 👍

@HBelusca HBelusca removed the request for review from jimtabor June 21, 2023 09:25
@GeoB99 GeoB99 merged commit 12e1919 into reactos:master Jul 2, 2023
32 of 33 checks passed
ReactOS PRs automation moved this from Approved by reviewers to Done Jul 2, 2023
@Doug-Lyons
Copy link
Contributor Author

Thanks, @GeoB99 for the merge, and @JoachimHenze, @HBelusca, and @tkreuzer for your approvals toward getting this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix For bugfix PRs. Win32SS For Win32 subsystem (Win32k, GDI/USER DLLs, etc.) related components PRs.
Projects
ReactOS PRs
  
Done
9 participants