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

[WIN32] Migrate to ShellExecuteEx() #3852

Merged
merged 1 commit into from May 14, 2022

Conversation

carlo-bramini
Copy link
Contributor

@carlo-bramini carlo-bramini commented May 1, 2022

I would like to suggest to migrate from ShellExecute() to ShellExecuteEx().
That function allows to write a more readable code, without the ugly that needs to test if returned value is greater than 32.
Supported platforms are:

  • Windows 95: Supported.
  • Windows 98: Supported.
  • Windows NT: Required Windows NT 4.0 or later.
  • Windows 2000 and newer: Supported.
  • Windows CE: Requires Windows CE 1.0 or later.

This fix comes from my port of SCUMMVM for Windows CE/Embedded/Mobile since this family of OSs support ShellExecuteEx(), but not ShellExecute(). However, this fix will also work on all versions of Windows supported by SCUMMVM.

That function allows more readable code, without needing to test if the
returned value is greater than 32.

Supported platforms are:
    Windows 95: Supported.
    Windows 98: Supported.
    Windows NT: Required Windows NT 4.0 or later.
    Windows 2000 and newer: Supported.
    Windows CE: Requires Windows CE 1.0 or later.

This fix comes from my port of SCUMMVM for Windows CE/Embedded/Mobile since
this family of OSs support ShellExecuteEx(), but not ShellExecute().
@digitall
Copy link
Member

@digitall digitall commented May 3, 2022

@carlo-bramini : Thanks for your code contribution. I have reworded the commit message to align with our code formatting guidelines and to simplify the phrasing slightly.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented May 3, 2022

Hello! Thanks for the PR, your Win32 cleanup instincts are spot on. =)

I was looking at the old ShellExecuteEx MSDN page and it says there's only Win9x support if you install the ol' Microsoft Layer for Unicode (MSLU), which is an external component: https://web.archive.org/web/20071030060912/http://msdn2.microsoft.com/en-us/library/bb762154.aspx

That was sort of my distant memory too, but it was a long time ago. I'd be happy to setup a test, could be fun.

Are you still maintaining your port(s)? If it turns out that ShellExecuteEx isn't on default Win9x but you'd benefit from upstream not importing ShellExecute, we could make a wrapper function that GetProcAddress' them both and uses whichever is available.

@carlo-bramini
Copy link
Contributor Author

@carlo-bramini carlo-bramini commented May 3, 2022

Windows 95 supports ShellExecuteEx().
Here, the hex view of the content of SHELL32.DLL of Windows 95, with the reference to this function, taken with HxD and the virtual disk attached to the local machine.

ShellExecuteExA

Tested also with a very simple test application.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented May 3, 2022

Great, thanks for verifying that

@lotharsm
Copy link
Member

@lotharsm lotharsm commented May 14, 2022

Thanks for your contribution @carlo-bramini and thanks to @sluicebox for diving into the depths of ancient MSDN (again) ;-)

Any objections or open questions before I merge this?

@bluegr
Copy link
Member

@bluegr bluegr commented May 14, 2022

Great work! Just went through this, it's a welcome change.
+1 from me
@lotharsm I believe it should be OK to merge, it's been 2 weeks that this has been open, and there haven't been any objections

@lotharsm
Copy link
Member

@lotharsm lotharsm commented May 14, 2022

Alright, let's give it a go. Merging!

@lotharsm lotharsm merged commit b195cbb into scummvm:master May 14, 2022
8 checks passed
@carlo-bramini carlo-bramini deleted the fix-shellexecute branch May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants