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

feat(share_plus): remove direct dependence of url_launcher #1295

Merged
merged 2 commits into from
Oct 31, 2022
Merged

feat(share_plus): remove direct dependence of url_launcher #1295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
364d35c
feat(share_plus): remove direct dependence of url_launcher
SirusCodes Oct 26, 2022
167c604
fix(share_plus): breaking tests
SirusCodes Oct 28, 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
15 changes: 12 additions & 3 deletions packages/share_plus/share_plus/lib/src/share_plus_linux.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@ library share_plus_linux;
import 'dart:ui';

import 'package:share_plus_platform_interface/share_plus_platform_interface.dart';
import 'package:url_launcher/url_launcher.dart';
import 'package:url_launcher_linux/url_launcher_linux.dart';
import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart';

/// The Linux implementation of SharePlatform.
class SharePlusLinuxPlugin extends SharePlatform {
SharePlusLinuxPlugin(this.urlLauncher);

final UrlLauncherPlatform urlLauncher;

/// Register this dart class as the platform implementation for linux
static void registerWith() {
SharePlatform.instance = SharePlusLinuxPlugin();
SharePlatform.instance = SharePlusLinuxPlugin(UrlLauncherLinux());
}

/// Share text.
Expand All @@ -35,7 +40,11 @@ class SharePlusLinuxPlugin extends SharePlatform {
.join('&'),
);

await launchUrl(uri);
if (await urlLauncher.canLaunch(uri.toString())) {
await urlLauncher.launchUrl(uri.toString(), const LaunchOptions());
} else {
throw Exception('Unable to share on web');
}
}
Copy link

Choose a reason for hiding this comment

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

FYI: changing to this construction will break functionality, and has no advantages; see https://pub.dev/packages/url_launcher#checking-supported-schemes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, it's better to have only launchUrl in try catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking the return of launchUrl is better construct?

Copy link

Choose a reason for hiding this comment

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

So, it's better to have only launchUrl in try catch block?

I'm not sure what the goal of a try/catch would be given that you are currently throwing.

Checking the return of launchUrl is better construct?

If you don't have a specific need to know in advance of trying (which is rare, and certainly isn't the case given the code here), checking what actually happened is better than using a potentially inaccurate prediction of what might happen, yes.

Copy link
Member

Choose a reason for hiding this comment

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

@miquelbeltran FYI

Copy link
Member

Choose a reason for hiding this comment

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

If anyone can send a PR with the fix, I can review and merge asap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I can do that.


/// Share files.
Expand Down
17 changes: 11 additions & 6 deletions packages/share_plus/share_plus/lib/src/share_plus_web.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,25 @@ import 'package:flutter/widgets.dart';
import 'package:flutter_web_plugins/flutter_web_plugins.dart';
import 'package:mime/mime.dart' show lookupMimeType;
import 'package:share_plus_platform_interface/share_plus_platform_interface.dart';
import 'package:url_launcher/url_launcher.dart';
import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart';
import 'package:url_launcher_web/url_launcher_web.dart';

/// The web implementation of [SharePlatform].
class SharePlusWebPlugin extends SharePlatform {
final UrlLauncherPlatform urlLauncher;

/// Registers this class as the default instance of [SharePlatform].
static void registerWith(Registrar registrar) {
SharePlatform.instance = SharePlusWebPlugin();
SharePlatform.instance = SharePlusWebPlugin(UrlLauncherPlugin());
}

final html.Navigator _navigator;

/// A constructor that allows tests to override the window object used by the plugin.
SharePlusWebPlugin({@visibleForTesting html.Navigator? debugNavigator})
: _navigator = debugNavigator ?? html.window.navigator;
SharePlusWebPlugin(
this.urlLauncher, {
@visibleForTesting html.Navigator? debugNavigator,
}) : _navigator = debugNavigator ?? html.window.navigator;

/// Share text
@override
Expand All @@ -45,8 +50,8 @@ class SharePlusWebPlugin extends SharePlatform {
.join('&'),
);

if (await canLaunchUrl(uri)) {
await launchUrl(uri);
if (await urlLauncher.canLaunch(uri.toString())) {
await urlLauncher.launchUrl(uri.toString(), const LaunchOptions());
} else {
throw Exception('Unable to share on web');
}
Expand Down
13 changes: 9 additions & 4 deletions packages/share_plus/share_plus/lib/src/share_plus_windows.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,21 @@ import 'dart:ui';

import 'package:share_plus/src/windows_version_helper.dart';
import 'package:share_plus_platform_interface/share_plus_platform_interface.dart';
import 'package:url_launcher/url_launcher.dart';
import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart';
import 'package:url_launcher_windows/url_launcher_windows.dart';

/// The fallback Windows implementation of [SharePlatform], for older Windows versions.
///
class SharePlusWindowsPlugin extends SharePlatform {
SharePlusWindowsPlugin(this.urlLauncher);

final UrlLauncherPlatform urlLauncher;

/// If the modern Share UI i.e. `DataTransferManager` is not available, then use this Dart class instead of platform specific implementation.
///
static void registerWith() {
if (!VersionHelper.instance.isWindows10RS5OrGreater) {
SharePlatform.instance = SharePlusWindowsPlugin();
SharePlatform.instance = SharePlusWindowsPlugin(UrlLauncherWindows());
}
}

Expand All @@ -39,8 +44,8 @@ class SharePlusWindowsPlugin extends SharePlatform {
.join('&'),
);

if (await canLaunchUrl(uri)) {
await launchUrl(uri);
if (await urlLauncher.canLaunch(uri.toString())) {
await urlLauncher.launchUrl(uri.toString(), const LaunchOptions());
} else {
throw Exception('Unable to share on windows');
}
Expand Down
6 changes: 4 additions & 2 deletions packages/share_plus/share_plus/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,17 @@ dependencies:
sdk: flutter
share_plus_platform_interface: ^3.1.2
file: ^6.0.0
url_launcher: ^6.1.2
url_launcher_web: ^2.0.13
url_launcher_windows: ^3.0.1
url_launcher_linux: ^3.0.1
url_launcher_platform_interface: ^2.1.1
ffi: ^2.0.1
win32: ^3.0.0

dev_dependencies:
flutter_test:
sdk: flutter
flutter_lints: ^2.0.1
url_launcher_platform_interface: ^2.0.2

environment:
sdk: ">=2.12.0 <3.0.0"
Expand Down
9 changes: 3 additions & 6 deletions packages/share_plus/share_plus/test/share_plus_linux_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,26 @@ void main() {
});
test('url encoding is correct for &', () async {
final mock = MockUrlLauncherPlatform();
UrlLauncherPlatform.instance = mock;

await SharePlusLinuxPlugin().share('foo&bar', subject: 'bar&foo');
await SharePlusLinuxPlugin(mock).share('foo&bar', subject: 'bar&foo');

expect(mock.url, 'mailto:?subject=bar%26foo&body=foo%26bar');
});

// see https://github.com/dart-lang/sdk/issues/43838#issuecomment-823551891
test('url encoding is correct for spaces', () async {
final mock = MockUrlLauncherPlatform();
UrlLauncherPlatform.instance = mock;

await SharePlusLinuxPlugin().share('foo bar', subject: 'bar foo');
await SharePlusLinuxPlugin(mock).share('foo bar', subject: 'bar foo');

expect(mock.url, 'mailto:?subject=bar%20foo&body=foo%20bar');
});

test('throws when url_launcher can\'t launch uri', () async {
final mock = MockUrlLauncherPlatform();
mock.canLaunchMockValue = false;
UrlLauncherPlatform.instance = mock;

expect(() async => await SharePlusLinuxPlugin().share('foo bar'),
expect(() async => await SharePlusLinuxPlugin(mock).share('foo bar'),
throwsException);
});
}
Expand Down
9 changes: 3 additions & 6 deletions packages/share_plus/share_plus/test/share_plus_windows_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,8 @@ void main() {
'url encoding is correct for &',
() async {
final mock = MockUrlLauncherPlatform();
UrlLauncherPlatform.instance = mock;

await SharePlusWindowsPlugin().share('foo&bar', subject: 'bar&foo');
await SharePlusWindowsPlugin(mock).share('foo&bar', subject: 'bar&foo');

expect(mock.url, 'mailto:?subject=bar%26foo&body=foo%26bar');
},
Expand All @@ -45,9 +44,8 @@ void main() {
'url encoding is correct for spaces',
() async {
final mock = MockUrlLauncherPlatform();
UrlLauncherPlatform.instance = mock;

await SharePlusWindowsPlugin().share('foo bar', subject: 'bar foo');
await SharePlusWindowsPlugin(mock).share('foo bar', subject: 'bar foo');

expect(mock.url, 'mailto:?subject=bar%20foo&body=foo%20bar');
},
Expand All @@ -59,9 +57,8 @@ void main() {
() async {
final mock = MockUrlLauncherPlatform();
mock.canLaunchMockValue = false;
UrlLauncherPlatform.instance = mock;

expect(() async => await SharePlusWindowsPlugin().share('foo bar'),
expect(() async => await SharePlusWindowsPlugin(mock).share('foo bar'),
throwsException);
},
skip: VersionHelper.instance.isWindows10RS5OrGreater,
Expand Down