-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[url_launcher] Add support for setting show title on Chrome Custom Tabs #5166
Conversation
@@ -62,17 +62,42 @@ class UrlLauncherAndroid extends UrlLauncherPlatform { | |||
required Map<String, String> headers, | |||
String? webOnlyWindowName, | |||
}) async { | |||
final WebViewOptions webViewOptions = WebViewOptions( |
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.
There is a fair share of code duplication here, but it could only be resolved after flutter/flutter#66721 unfortunately (could not make it a passthrough because there is no way to pass the useWebView parameter to launchUrl).
Did my best to reduce it
This cannot be implemented on the web, since the browser controls how tabs/windows are displayed. (Feel free to @ me if I can be of any help with the rest of the PR!) |
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.
All the packages where the only change is dependency_overrides
can be reverted.
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.
Did not think of that, done ✅
packages/url_launcher/url_launcher_platform_interface/lib/src/types.dart
Outdated
Show resolved
Hide resolved
*Will update docs once #5155 lands |
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.
Generally looks good, just small comments. Thanks for your patience!
/// Creates a new InAppBrowserConfiguration with given settings. | ||
const InAppBrowserConfiguration({this.showTitle = false}); | ||
|
||
/// Whether or not to show the webpage title when using Chrome Custom Tabs. |
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.
Please remove "when using Chrome Custom Tabs."
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.
Done
|
||
/// Whether or not to show the webpage title when using Chrome Custom Tabs. | ||
/// | ||
/// Only works on Android. |
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.
"May not be supported on all platforms."
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.
Done
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.
The changes in this file will need a simple test (e.g., testing the default InAppBrowserConfiguration
behavior).
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.
Done. Did not quite think of a lot of testcases, you can find those in packages/url_launcher/url_launcher_platform_interface/test/in_app_browser_configuration_test.dart and packages/url_launcher/url_launcher_platform_interface/test/launch_options_test.dart
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.
This package will need a version bump and changelog.
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.
Done
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.
This will need a version change.
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.
Done
private static boolean openCustomTab( | ||
@NonNull Context context, @NonNull Uri uri, @NonNull Bundle headersBundle) { | ||
CustomTabsIntent customTabsIntent = new CustomTabsIntent.Builder().build(); | ||
@VisibleForTesting |
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.
Why was this made public for testing? Testing should be done via public interface whenever possible to make tests easier to maintain and less likely to miss important behavior.
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.
Reverted this change as well
@@ -169,7 +179,8 @@ private static boolean containsRestrictedHeader(Map<String, String> headersMap) | |||
return false; | |||
} | |||
|
|||
private static @NonNull Bundle extractBundle(Map<String, String> headersMap) { | |||
@VisibleForTesting |
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.
Same question here.
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.
Done
String url = "https://flutter.dev"; | ||
HashMap<String, String> headers = new HashMap<>(); | ||
String headerKey = "Content-Type"; | ||
headers.put(headerKey, "text/plain"); |
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.
Headers don't seem relevant to this test, so shouldn't need to be set up.
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.
Yes, you are right. Removed this
String url = "https://flutter.dev"; | ||
HashMap<String, String> headers = new HashMap<>(); | ||
String headerKey = "Content-Type"; | ||
headers.put(headerKey, "text/plain"); |
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.
Same.
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.
Done
|
||
/// Whether or not to show the webpage title when using Chrome Custom Tabs. | ||
/// | ||
/// Only works on Android. |
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.
Same comments here as on the platform interface; platform details shouldn't be documented at this level.
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.
Made this comment platform-independent
Thank you for your valuable feedback and your time! |
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.
This looks good with a few minor nits; please feel free to split out the first sub-PR with just the platform interface changes.
onPressed: () => setState(() { | ||
_launched = _launchInAppWithBrowserOptions(toLaunch); | ||
}), | ||
child: const Text('Launch in app with showTitle'), |
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.
"[...] with title displayed"
|
||
* Implement taking in `BrowserConfiguration` parameter |
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.
These need to follow the CHANGELOG style guide linked from the PR checklist (verb form, punctuation).
|
||
/// Whether or not to show the webpage title. | ||
/// | ||
/// May not be supported on all platforms. |
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.
This file is internal to the Android implementation, so this sentence doesn't make sense here.
@@ -50,13 +50,26 @@ class InAppWebViewConfiguration { | |||
final Map<String, String> headers; | |||
} | |||
|
|||
/// Additional configuration options for [PreferredLaunchMode.inAppBrowserView] |
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.
Nit: Missing a period.
This is #5166 portion of platform interface changes. Adds `InAppBrowserConfiguration` parameter, as well as `InAppBrowserConfiguration.showTitle` parameter that configures whether to show or not to show webpage title
Update from triage: This is blocked on #5759, which is currently in review. |
This is flutter#5166 portion of platform interface changes. Adds `InAppBrowserConfiguration` parameter, as well as `InAppBrowserConfiguration.showTitle` parameter that configures whether to show or not to show webpage title
…ations (flutter#5759) Platform implementation portion of flutter#5166. Implements `InAppBrowserConfiguration` support on Android, as well as support for `InAppBrowserConfiguration.showTitle` parameter for hiding/showing webpage title in Android Custom Tabs.
(Notice title at the top of the page "CyLog Software - HTTP ...")
flutter/flutter#136784
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.