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.

Sign up for GitHub

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

Jump to bottom

Always call launchUrlVSCode, error or not #6780

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Always call launchUrlVSCode, error or not #6780

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions packages/devtools_app/lib/src/shared/config_specific/launch_url/launch_url.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,20 @@ import '_launch_url_desktop.dart'
Future<void> launchUrl(String url) async {
final parsedUrl = Uri.tryParse(url);

if (parsedUrl != null && await url_launcher.canLaunchUrl(parsedUrl)) {
await url_launcher.launchUrl(parsedUrl);
} else {
notificationService.push('Unable to open $url.');
try {
if (parsedUrl != null && await url_launcher.canLaunchUrl(parsedUrl)) {
await url_launcher.launchUrl(parsedUrl);
} else {
notificationService.push('Unable to open $url.');
}
} finally {
// Always pass the request up to VS Code because we could fail both silently
// (the usual behaviour) or with another error like
// "Attempted to call Window.open with a null window"
// https://github.com/flutter/devtools/issues/6105.
//
// In the case where we are not in VS Code, there will be nobody listening
// to the postMessage this sends.
launchUrlVSCode(url);
}
Comment on lines +14 to 29
Copy link
Member

Choose a reason for hiding this comment

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

would it be better to just call launchUrlVSCode by default when ideTheme.embed == true? instead of attempting to call something we expect to fail?

Copy link
Member

Choose a reason for hiding this comment

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

for example, what if at some point, url_launcher.launchUrl starts working for the embedded environment? then we'd be launching the url twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't expect launchUrlVSCode to fail, it will just silently do nothing (it'll postMessage but nobody is listening).

We can check that though - is there any possibility of ending up here without it being on globals? (I don't think so, but it feels slightly riskier than the change to just call this when an exception occurs for a cherry-pick)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe my comment "we could fail" above is a bit misleading - it's referring to the existing url_launcher code.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, yeah since we were already always calling launchUrlVSCode, then putting this in finally is probably safer. So the real change here is just wrapping in a try block to swallow the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear - the launchUrlVSCode method does not throw, the issue was that url_launcher.launchUrl would sometimes throw (in the previous version, not the current stable).

This code was originally written to always call launchUrlVSCode, but in the case where url_launcher.launchUrl throws (something we never expected but was happening in the previous stable), it would be skipped.

My change here just moves launchUrlVSCode into a finally block so that it will be called also if url_launcher.launchUrl throws, and not only if it completed without exception.

I didn't use a catch because I didn't want to mask the original (unexpected) exception if it comes back, but this way we'll at least still open the URL from VS Code instead of silently doing nothing.


// When embedded in VSCode, url_launcher will silently fail, so we send a
// command to DartCode to launch the URL. This will do nothing when not
// embedded in VSCode.
launchUrlVSCode(url);
}
Loading