-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
Show error dialog if WebView fails to load #2781
Conversation
@@ -75,14 +75,16 @@ def winforms_initialization_completed(self, sender, args): | |||
settings = self.native.CoreWebView2.Settings | |||
self.default_user_agent = settings.UserAgent | |||
|
|||
debug = True | |||
debug = False |
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.
I disabled debug mode. Was it intentionally left enabled? Does this deserve a changenote?
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.
It was intentionally enabled, mostly to ensure that the developer tools are available. I think it makes sense that these are disabled by default; but we do need a better way to enable debug functionality for development purposes.
An additional removal
changenote is probably called for, just in case anyone was depending on the behaviour.
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.
Soo, this doesn't appear to be limited to WinForms. I see that a "debug mode" is enabled for macOS, iOS, and Gtk as well. (I think Android drives access to DevTools with the nature of the build; debug builds enable it while release builds disable it).
I think I'll descope this for a separate effort to make this configurable.
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.
Looks good; the extra changenote is the only thing needed to land this PR, unless you want to tackle improving making the failure more user-visible as part of this PR.
I agree a more user-visible mechanism would be preferable, but I'm not sure what that would be. Some ideas:
- On error, replace the webview container with a textview that contains the error message,
- Pop up a dialog with the error condition
- Post an error to the app's event loop that raises the exception in the main app context.
Assuming (3) works, that's probably the best case, because it would surface as a text log when there's a console, but should be caught by the Briefcase stub when it's a GUI app.
@@ -75,14 +75,16 @@ def winforms_initialization_completed(self, sender, args): | |||
settings = self.native.CoreWebView2.Settings | |||
self.default_user_agent = settings.UserAgent | |||
|
|||
debug = True | |||
debug = False |
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.
It was intentionally enabled, mostly to ensure that the developer tools are available. I think it makes sense that these are disabled by default; but we do need a better way to enable debug functionality for development purposes.
An additional removal
changenote is probably called for, just in case anyone was depending on the behaviour.
So, I actually tried to do this with something like this in the callback: async def raise_exception():
raise RuntimeError("failed to load WebView")
asyncio.create_task(raise_exception()) This didn't work for me, though; can you imagine a different approach that might work? |
My original thought was to go non-async, with Out of interest - what is the failure mode? Does it not process the async handler, or process it but in a way that isn't fatal to the app? |
This did print an exception to the console; however, the app continued running. def winforms_initialization_completed(self, sender, args):
def raise_exception():
raise RuntimeError("failed to load WebView")
self.interface.app.loop.call_soon_threadsafe(raise_exception) Adding this callback as well stopped the app though.... self.interface.app.loop.call_soon_threadsafe(self.interface.app.exit)
With |
I guess whether we exit the app or not, I don't think pushing the Exception on to the loop really does us any good....unless we only expect this issue to happen during development. Otherwise, users are just going to have an app that opens and immediately closes; even if the dev asks the user to fish the log out of AppData, I'm not sure this So, maybe showing a dialog like how the missing Edge Runtime is handled is best; at least then the user can tell something is obviously failing and it involves something called a WebView. |
That's not entirely unexpected. The Winforms asyncio integration is a bit weird, because of the winforms STA apartment state. The app itself runs in a thread; exceptions are caught and the stack trace is manually unwound for printing purposes, but won't necessarily terminate the app itself. I'm not 100% convinced this implementation is correct - the intention was that an exception in an event handler would be reported, but wouldn't kill the app; but it's possible this is being a little too enthusiastic with surpassing exit conditions, and actual errors in the app mainline aren't being surfaced. Explicitly calling exit() in the
It's more likely that the task is being created, but it's being put on an event loop that is never started. The default |
That's a good point - the existing dialog behavior is a good precedent for how "This app ain't working" should be handled int this case, so we might as well lean into that. |
d5872cf
to
2f3ab89
Compare
I added a dialog explaining the failure. Open to wording critique. Since I haven't rebased on This obviously isn't immediately useful to users...but I did find it interesting if you press CTRL+C, it puts this on the clipboard:
|
- On Windows, if the callback for WebView initialization raises an exception, that exception is swallowed in .NET and not reported to the user. Now, the user is presented with a dialog box explaining the failure.
2f3ab89
to
4ae9b7e
Compare
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.
Looks great! And the clipboard thing is a nice trick... I guess the question is whether users know that feature exists.
So, my interest remain piqued since you said this. To recap, I had put a task on the event loop that would raise a I tracked down this behavior to def default_exception_handler(self, context):
print("this is your friendly exception handler")
super().default_exception_handler(context)
So, given this, what is the desired behavior when Toga itself raises an exception? If I simulate raising the
|
@rmartin16 I've opened #2806 to continue this discussion, rather than burying it on a resolved Webview ticket. |
Changes
PR Checklist: