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 panic message boxes #2086

Merged
merged 1 commit into from
Apr 28, 2024
Merged

Fix panic message boxes #2086

merged 1 commit into from
Apr 28, 2024

Conversation

SniperJoe
Copy link
Contributor

Problem: when alvr-server panics, it displays a message box with the panic info. But the rfd::MessageDialog replaces the whole message box content with just "An error occuried" which is not really informative. I played around it a little bit and it seems that it does not accept the panic info as the message box content.

Solution: Shorten the info to display. After that the message box is displayed normally.

@Vixea
Copy link
Collaborator

Vixea commented Apr 23, 2024

Could you do a before and after

@SniperJoe
Copy link
Contributor Author

Before:
Выделение_161
After:
Выделение_160


#[cfg(all(not(target_os = "android"), feature = "enable-messagebox"))]
std::thread::spawn(move || {
rfd::MessageDialog::new()
.set_title("ALVR panicked")
.set_description(&err_str)
.set_description(&short_err_str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use panic_info directly here?

@zmerp
Copy link
Member

zmerp commented Apr 24, 2024

I believe the issue is that rfd does not support new lines in the string. Not actually about the panic info in particular. Try calling alvr_common::show_e() with a new line in it.

@zmerp
Copy link
Member

zmerp commented Apr 26, 2024

@SniperJoe ping. Please do more tests as I suggested, as the code of this PR doesn't really explain why it fixes the issue.

@SniperJoe
Copy link
Contributor Author

@zarik5 tested with the long error with removed line breaks. Still not showing correctly.

@SniperJoe
Copy link
Contributor Author

@zarik5 also tried to show a short error with one \n, all good
Выделение_164

@zmerp zmerp merged commit 7597938 into alvr-org:master Apr 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants