Feature/issue 217 #218
Feature/issue 217 #218
Conversation
There was a problem hiding this comment.
Thanks @dobromirdobrev, this looks like it is what I had in mind! I left a couple questions below.
| /// returns true if the url is launched and false otherwise | ||
| /// | ||
| static void launchUrl({required String url, BuildContext? context, UrlLaunchMode? mode}) { | ||
| switch (mode) { |
There was a problem hiding this comment.
Should we call canLaunchUrl on url at the beginning here?
There was a problem hiding this comment.
Yes, you are right @shurwit , I also think we should call it.
| import 'package:intl/intl.dart'; | ||
| import 'package:path/path.dart' as path_package; | ||
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter/services.dart'; | ||
| import 'package:fluttertoast/fluttertoast.dart'; | ||
| import 'package:rokwire_plugin/ui/panels/web_panel.dart'; |
There was a problem hiding this comment.
@mihail-varbanov, are you ok with importing UI components in utils.dart? I think we wanted to avoid this, but I'm not currently sure where it would make more sense to put these functions. Let me know if you have any thoughts. Thanks!
There was a problem hiding this comment.
Hi @shurwit and @dobromirdobrev,
I am sorry for the delayed response, I did not notice this mention across the sea of GitHub notifications that I get every day.
Adding dependency of utils from plugin UI components does not look good to me. The relation should be opposite - UI components should depend on utils and not vice versa.
Moreover, I see almost no real advantage of UrlUtils.launchUrl, except maybe parsing the Uri from String and applying the null check verification. For me this is some kind of attempt for universalism that brings more harm than value. I would remove internalWebPanel as an option for UrlUtils.launchUrl, as well as the entire UrlLaunchMode definition as it would appear as 100% duplication of url_launcher.LaunchMode, but would keep UrlUtils.launchUrl(String? url) as convenient shortcut to the url_launcher.
There was a problem hiding this comment.
Hi @mihail-varbanov, the point of this PR is to use the internal web panel instead of the launchUri internal web view. Please refer back to the original issue (rokwire/illinois-app#2623) to see why this is necessary,
There was a problem hiding this comment.
Hi @shurwit,
I do not see a clear relation between the problem that you describe in rokwire/illinois-app#2623 and the UrlUtils.launchUrl helper as it is. All that is known is that we should not load web URLs in an in-app browser on Android platforms. Whether we should load such URLs in an external browser or a web panel is a matter of design, both methods bring their benefits and drawbacks. I consider the necessity of UrlUtils.launchUrl helper only in order to avoid the following construction repeated many times:
LaunchMode launchMode = Platform.isAndroid ? LaunchMode.externalApplication : LaunchMode.platformDefault;
launchUrl(uri, mode: launchMode);
This does not require involving WebPanel in this helper. If this was up to me I would do the helper in a way like this:
static Future<bool?> launchUrl(String url, { url_launcher.LaunchMode? mode}) {
Uri? uri = Uri.tryParse(url);
mode ??= Platform.isAndroid ? url_launcher.LaunchMode.externalApplication : url_launcher.LaunchMode.inAppWebView;
return (uri != null) ? url_launcher.launchUrl(uri, mode: mode) : Future.value(null);
}
There was a problem hiding this comment.
Thanks again for the feedback @mihail-varbanov, @pmarkhennessy and I are discussing this and will get back to you soon.
* develop: Fix missing close button from ModalImagePanel[#227] Make make_image_holder to work with local file urls Remove "appointmentsCanDisplay" from plugin storage - store it in the app. update changelog update changelog fix privacy level saving, fix privacy styles Update CHANGELOG.md [#223] Do not pass image url to ModelImageHolder - show already build image widget [#223] Fixed crash in header bar widget [rokwire/illinois-app/#2654] add offlineWidget to survey panel and survey widget [#219] Updated CHANGELOG.md [#220]. ExploreLocation.fromJSON renamed to fromJson, added misc helpers to ExploreLocation [#220]. check mounted when calling setState on SurveyWidget [#219] Added Explore.exploreLocationDescription interface [rokwire/illinois-app#2633]. make GroupSettings fields not final # Conflicts: # CHANGELOG.md
| _launchParsedUri(url, url_launcher.LaunchMode.externalNonBrowserApplication); | ||
| break; | ||
| default: | ||
| _launchParsedUri(url, url_launcher.LaunchMode.platformDefault); |
There was a problem hiding this comment.
We need to handle the internal launch using the WebPanel in this default case.
| /// returns true if the url is launched and false otherwise | ||
| /// | ||
| static void launchUrl({required String url, BuildContext? context, UrlLaunchMode? mode}) { | ||
| switch (mode) { |
There was a problem hiding this comment.
I just noticed that there are several places in the app where we are have a nearly idential function that also checks for deeplinks internal to the app (eg. https://github.com/rokwire/illinois-app/blob/268525747cb5d6a848550775c1c03e415505445f/lib/ui/BrowsePanel.dart#L748). If we do decide to keep this function I think it would be good to do this check as well so we can get rid of all of these duplicated functions.
|
If this is the case I would to place this logic in public static WebPanel.launchUrl method. However, there is another problem. We have to create WebPanel instance that is defined in application, not an instance that is defined in the plugin. While in Services we did the trick with single static instance this does not seem possible here. The only way I could think of is to fire a notification that RootPanel should handle, create the proper WebPanel instance and push it in the navigation stack. Too complex indeed. Or, to place the logic in application's WebPanel instance? |
Description
Create utility method for launching urls.
Fixes #217