Skip to content

[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582

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

Merged
auto-submit merged 15 commits into from
Apr 1, 2025
Merged

[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3403990
fix(url_launcher): When not fully loaded, clicking close causes the c…
StanleyCocos Feb 7, 2025
6082e8c
fix: change log
StanleyCocos Feb 7, 2025
13e3058
fix: format
StanleyCocos Feb 7, 2025
87a4a3a
fix: chenge log
StanleyCocos Feb 7, 2025
940b311
fix: test
StanleyCocos Feb 7, 2025
4f733e5
fix: type
StanleyCocos Feb 8, 2025
2c0d46b
fix: test
StanleyCocos Feb 10, 2025
4c258ab
Merge branch 'main' into fix/url_launcher_ios
StanleyCocos Feb 10, 2025
2a095f2
fix: changelog
StanleyCocos Feb 13, 2025
39ef600
Merge branch 'main' into fix/url_launcher_ios
StanleyCocos Feb 13, 2025
99359e7
fix: comment
StanleyCocos Feb 18, 2025
a2c8559
fix: comment
StanleyCocos Feb 21, 2025
1b53da5
Merge branch 'main' into fix/url_launcher_ios
StanleyCocos Feb 21, 2025
68e865a
Merge branch 'main' into fix/url_launcher_ios
StanleyCocos Mar 17, 2025
5e7b70e
add dismissed
StanleyCocos Mar 17, 2025
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
Next Next commit
fix(url_launcher): When not fully loaded, clicking close causes the c… 
…allback to not be triggered correctly.
  • Loading branch information
@StanleyCocos
StanleyCocos committed Feb 7, 2025
commit 3403990cb3b21df0def69a739e43606f86cadc18
3 changes: 2 additions & 1 deletion packages/url_launcher/url_launcher/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## NEXT
## 6.3.2

* Updates minimum supported SDK version to Flutter 3.22/Dart 3.4.
* Fixes When not fully loaded, clicking close causes the callback to not be triggered correctly.

## 6.3.1

Expand Down
5 changes: 5 additions & 0 deletions ...her/url_launcher_ios/ios/url_launcher_ios/Sources/url_launcher_ios/URLLaunchSession.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ final class URLLaunchSession: NSObject, SFSafariViewControllerDelegate {

private let completion: OpenInSafariCompletionHandler
private let url: URL
private var isLoadCompleted: Bool?
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Feb 7, 2025

Choose a reason for hiding this comment

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

A nullable boolean should only be used when there are three conceptually different states. The code in this PR only actually recognizes two.


/// The Safari view controller used for displaying the URL.
let safariViewController: SFSafariViewController
Expand Down Expand Up @@ -46,12 +47,16 @@ final class URLLaunchSession: NSObject, SFSafariViewControllerDelegate {
} else {
completion(.success(.failedToLoad))
}
isLoadCompleted = didLoadSuccessfully;
}

/// Called when the user finishes using the Safari view controller.
///
/// - Parameter controller: The Safari view controller.
func safariViewControllerDidFinish(_ controller: SFSafariViewController) {
if isLoadCompleted == nil {
completion(.success(.failedToLoad))
Copy link
Member

@loic-sharma loic-sharma Feb 12, 2025

Choose a reason for hiding this comment

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

Is .failedToLoad the correct result here? Or we should add a new InAppLoadResult.dismissed result?

If this is the correct result, should we update this comment to indicate this case can happen if the user dismisses the Safari view before the initial load completes?

packages/packages/url_launcher/url_launcher_ios/pigeons/messages.dart

Lines 30 to 31 in fd53793

/// The URL did not load successfully.
failedToLoad,

Copy link
Contributor Author

@StanleyCocos StanleyCocos Feb 13, 2025

Choose a reason for hiding this comment

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

At first, I did consider adding a new InAppLoadResult to describe the close action, but it seems it could lead to confusion among developers. This is because, currently, after a successful load, clicking the close button does not trigger any callback.

Additionally, when the user clicks close before the loading is complete, triggering the failedToLoad callback seems reasonable in a broader sense, as the interruption of the loading process can be considered a loading failure.

Copy link
Contributor Author

@StanleyCocos StanleyCocos Feb 13, 2025

Choose a reason for hiding this comment

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

If this is the correct result, should we update this comment to indicate this case can happen if the user dismisses the Safari view before the initial load completes?

Do you think it’s possible to modify the comment like this:

// The URL did not load successfully (or close the SFSafariViewController earlier)

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Mar 7, 2025

Choose a reason for hiding this comment

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

Returning this will throw an exception saying there was an error loading the page, which seems misleading. I think a new Pigeon return type that Dart interprets to return false would be better.

Copy link
Contributor Author

@StanleyCocos StanleyCocos Mar 17, 2025

Choose a reason for hiding this comment

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

I created a new dismissed state to represent the current status.

Copy link
Contributor Author

@StanleyCocos StanleyCocos Mar 25, 2025

Choose a reason for hiding this comment

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

cc @stuartmorgan
Could you take another look?

}
controller.dismiss(animated: true, completion: nil)
didFinish?()
}
Expand Down