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

[Linux/X11] Add a default error handler for X11 to avoid crashes. #75099

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

mxnemu
Copy link

@mxnemu mxnemu commented Mar 19, 2023

The default behaviour for X11 is to crash even on non-fatal errors when there is no error handler set. This change allows the window to stay open and may enable users to save their work when things go wrong.

This acts as a workaround for #65425 and #68471

@mxnemu mxnemu requested a review from a team as a code owner March 19, 2023 13:20
@Rubonnek Rubonnek added this to the 4.1 milestone Mar 19, 2023
@mxnemu
Copy link
Author

mxnemu commented Mar 19, 2023

clang-format makes the multiline string unreadable. Any advice on that?

@Riteo
Copy link
Contributor

Riteo commented Mar 19, 2023

@mxnemu the rest of the code uses vformat's variadic arguments to workaround this issue:

OS::get_singleton()->alert(
		vformat("Your video card drivers seem not to support the required Vulkan version.\n\n"
				"If possible, consider updating your video card drivers or using the OpenGL 3 driver.\n\n"
				"You can enable the OpenGL 3 driver by starting the engine from the\n"
				"command line with the command:\n\n    \"%s\" --rendering-driver opengl3\n\n"
				"If you recently updated your video card drivers, try rebooting.",
				executable_name),
		"Unable to initialize Vulkan video driver");

vformat returns a String and ERR_PRINT should support it, as its internal method (_err_print_error) has lots of overloads for both Strings and char *, so this should work fine.

Edit: just noticed that those aren't variadic arguments lol. Glad to have put the example as otherwise I would have messed up things further.

@mxnemu mxnemu force-pushed the add-default-x11-error-handler branch from 31f4bd2 to 1434ad2 Compare March 20, 2023 00:12
Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

I haven't tested this but it looks fine!

@mxnemu
Copy link
Author

mxnemu commented Mar 20, 2023

Since the bugs aren't super easy to reproduce without installing a new window manager and configuring smart-borders, here are screenshots of my tests on Ubuntu 22.04 xmonad 0.15.

This branch - The godot window stays open and usable when an error occures:

2023-03-20-210206_784x294_scrot

This branch - Using xkill still ends godot, just the same as in the official release. So no unintended effects here.

2023-03-20-205415_769x736_scrot

Godot 4.0 before the patch - the window crashes on focus change as described in the issue reports:

2023-03-20-204148_777x684_scrot

@mxnemu
Copy link
Author

mxnemu commented May 17, 2023

Can this get merged now, so it will be in the next dev-snapshot and people can report anything unexpected?

@clayjohn clayjohn requested a review from bruvzg May 30, 2023 08:39
"\n Major opcode of failed request: %d"
"\n Serial number of failed request: %d"
"\n Current serial number in output stream: %d",
String(message), error->request_code, error->minor_code, error->serial));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String(message), error->request_code, error->minor_code, error->serial));
String::utf8(message), error->request_code, error->minor_code, error->serial));

According to docs: "The returned text is in the encoding of the current locale.", so it's likely UTF-8 in most cases, String(x) is for ASCII only.

Copy link
Member

Choose a reason for hiding this comment

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

@mxnemu Are you able to do this change? This seems to be the only step left before merging.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just tested it locally and amended my commit. 👍

Sorry for my delay and thanks for the help everyone.

@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

The default behaviour for X11 is to crash even on non-fatal errors
when there is no error handler set. This change allows the window to
stay open and may enable users to save their work when things go
wrong.

This acts as a workaround for godotengine#65425 and godotengine#68471
@mxnemu mxnemu force-pushed the add-default-x11-error-handler branch from 1434ad2 to b13c82e Compare June 5, 2023 15:31
@akien-mga akien-mga merged commit 0010b34 into godotengine:master Jun 5, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

5 participants