-
Notifications
You must be signed in to change notification settings - Fork 594
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 and rework Android error notifications #1051
Fix and rework Android error notifications #1051
Conversation
This reverts commit 7504395.
Use a proper alert dialog displaying the last error message instead of a toast that just links to the log file This method also does not require on any permissions (unlike toasts) Fixes KhronosGroup#1050
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant test on android but LGTM. This should provide better errors than what was there before!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well, thanks a lot! Noticed that for the same error (extension not present), after hitting 'close', some samples return to the launcher, while others crash the application.
# Conflicts: # framework/platform/platform.cpp
700249b
Merging now that conflicts are resolved (per discussion on Monday's call) |
Description
This PR completely overhauls the way our framework displays errors on Android. Initially the framework used a notification that would only display "something has crashed" with a link to the log file but no reason as to why a sample crashed or wasn't able to run. Displaying such notifications isn't a great way to display errors and also requires permissions starting with Android 13. Since we never requested those permissions, those messages weren't displayed and instead of gracefully closing, samples that don't work (e.g. because the device lacks a requested extension) would just result in a blank screen.
This PR replaces those notifications with a proper alert dialog, which is a better and cleaner way to display errors. The PR also adds a last error message member to the platform that is then displayed in this alert:
Note that error messages aren't optimal, but the way the framework currently handles errors is pretty convoluted, so this is the best I could do for now.
Fixes #1050
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batch
command line argument to make sure all samples still work properly