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

Fix shell_open not returning errors on Windows #52842

Merged
merged 1 commit into from
Sep 21, 2021

Conversation

rsubtil
Copy link
Contributor

@rsubtil rsubtil commented Sep 19, 2021

Fixes #52794

Followed ShellExecuteW docs and tried to match most error codes.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

Had no chance to build and test, but from the docs and my memory this looks good.

@mhilbrunner
Copy link
Member

Any reason for not checking the other errors?

@rsubtil
Copy link
Contributor Author

rsubtil commented Sep 20, 2021

Any reason for not checking the other errors?

Mostly not understanding what those situations are in detail and/or Godot not having a good error code to match it.

SE_ERR_FNF/SE_ERR_PNF are the same error codes as ERROR_FILE_NOT_FOUND/ERROR_PATH_NOT_FOUND, so they're implicitly matched too.

@akien-mga
Copy link
Member

SE_ERR_FNF/SE_ERR_PNF are the same error codes as ERROR_FILE_NOT_FOUND/ERROR_PATH_NOT_FOUND, so they're implicitly matched too.

That's a bit risky if we ever change our error codes, might be better to match explicit with a switch.

@mhilbrunner
Copy link
Member

Yeah, code should not depend on error enum members having a specific value, those will change.

Otherwise this looks good to me, I'm fine with not mapping every value for now, returning a general error for some cases is fine and this is still a big improvement. I'll make a note to maybe have a look and add more when my error overhaul is far enough along.

@rsubtil
Copy link
Contributor Author

rsubtil commented Sep 20, 2021

ERROR_FILE_NOT_FOUND/ERROR_PATH_NOT_FOUND is from Windows API, not from Godot 😅

@rsubtil
Copy link
Contributor Author

rsubtil commented Sep 20, 2021

Also originally I added both like so:

switch (ret) {
	case ERROR_FILE_NOT_FOUND:
	case SE_ERR_DLLNOTFOUND:
	case SE_ERR_FNF:
		return ERR_FILE_NOT_FOUND;
	case ERROR_PATH_NOT_FOUND:
	case SE_ERR_PNF:
		return ERR_FILE_BAD_PATH;
	....

But because they have the same error codes the compiler fails with duplicated case statements.

@mhilbrunner
Copy link
Member

Ah, yeah thats fine then, no way to avoid that. We could add a debug assert to ensure those are identical, but they're probably unlikely to change.

@akien-mga akien-mga merged commit a412011 into godotengine:master Sep 21, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 21, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 21, 2021
@rsubtil rsubtil deleted the fix_win_open_errcode branch November 26, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OS_Windows::shell_open returns an incorrect Error type
4 participants