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

Feature/issue 217 #218

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Feature/issue 217 #218

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f00a75b
Create utility method for launching urls [#217]
dobromirdobrev Dec 6, 2022
a4233d5
Update CHANGELOG.md [#217]
dobromirdobrev Dec 6, 2022
cf61d5d
Merge branch 'develop' into feature/issue-217
Dec 9, 2022
280c4b0
Fixed CHANGELOG.md
Dec 9, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add additional group settings [#2619](https://github.com/rokwire/illinois-app/issues/2619).
- Delete survey responses request [#210](https://github.com/rokwire/app-flutter-plugin/issues/210).
- Support font family references in text styles [#213](https://github.com/rokwire/app-flutter-plugin/issues/213).
- Create utility method for launching urls [#217](https://github.com/rokwire/app-flutter-plugin/issues/217).
### Deleted
- Removed Auth2.canFavorite [#2325](https://github.com/rokwire/illinois-app/issues/2325).
- Removed UserRole.resident [#2547](https://github.com/rokwire/illinois-app/issues/2547).
Expand Down
43 changes: 43 additions & 0 deletions lib/utils/utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ import 'dart:collection';
import 'dart:convert';
import 'dart:io';
import 'dart:math';
import 'package:flutter/cupertino.dart';
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';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@shurwit shurwit Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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,

Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks again for the feedback @mihail-varbanov, @pmarkhennessy and I are discussing this and will get back to you soon.

import 'package:timezone/timezone.dart' as timezone;
import 'package:url_launcher/url_launcher.dart' as url_launcher;

class StringUtils {

Expand Down Expand Up @@ -452,8 +455,48 @@ class UrlUtils {
}
return url;
}

static Future<bool> canLaunchUrl(String url) async {
Uri? parsedUri = Uri.tryParse(url);
return (parsedUri != null) && (await url_launcher.canLaunchUrl(parsedUri));
}

///
/// Launches urls based on the mode. 'context' must be specified if the mode is UrlLaunchMode.internalWebPanel
///
/// returns true if the url is launched and false otherwise
///
static void launchUrl({required String url, BuildContext? context, UrlLaunchMode? mode}) {
switch (mode) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call canLaunchUrl on url at the beginning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right @shurwit , I also think we should call it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

case UrlLaunchMode.internalWebPanel:
if (context != null) {
Navigator.push(context, CupertinoPageRoute(builder: (context) => WebPanel(url: url)));
}
break;
case UrlLaunchMode.internalInAppWebView:
_launchParsedUri(url, url_launcher.LaunchMode.inAppWebView);
break;
case UrlLaunchMode.externalApp:
_launchParsedUri(url, url_launcher.LaunchMode.externalApplication);
break;
case UrlLaunchMode.externalNonBrowserApp:
_launchParsedUri(url, url_launcher.LaunchMode.externalNonBrowserApplication);
break;
default:
_launchParsedUri(url, url_launcher.LaunchMode.platformDefault);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to handle the internal launch using the WebPanel in this default case.

break;
}
}

static void _launchParsedUri(String url, url_launcher.LaunchMode launchMode) {
Uri? parsedUri = Uri.tryParse(url);
if (parsedUri != null) {
url_launcher.launchUrl(parsedUri, mode: launchMode);
}
}
}

enum UrlLaunchMode { internalWebPanel, internalInAppWebView, externalApp, externalNonBrowserApp, platformDefault }

class JsonUtils {

Expand Down