Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[image_picker_platform_interface] migrate to nnbd #3492

Merged
merged 21 commits into from
Feb 4, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.0.0-nullsafety

* Migrate to NNBD.

## 1.1.6

* Fix test asset file location.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
include: ../../../analysis_options.yaml

analyzer:
enable-experiment:
- non-nullable
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ class MethodChannelImagePicker extends ImagePickerPlatform {
MethodChannel get channel => _channel;

@override
Future<PickedFile> pickImage({
@required ImageSource source,
double maxWidth,
double maxHeight,
int imageQuality,
Future<PickedFile?> pickImage({
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use Optional, instead of nullable here? Future<Optional<PickedFile>>? I've seen Optional classes in package:quiver docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I'm not sure what's the general guideline here. Maybe @blasten knows?

Copy link

Choose a reason for hiding this comment

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

Quiver appears to be a workaround for the lack of explicit nullability in the type system. With nnbd, I presume that package might get deprecated.

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 Future<PickedFile?> is ok? There is no preferred way?

Copy link

@blasten blasten Feb 2, 2021

Choose a reason for hiding this comment

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

I guess my question would be can the future be completed with error? Why yes or why not?

For example, is null being returned when an error is more appropriate?

future.catchError(....)
// or
try {
 await future;
} on ... catch {
}

// vs

if (await future == null) {
 /// 
}

required ImageSource source,
double? maxWidth,
double? maxHeight,
int? imageQuality,
CameraDevice preferredCameraDevice = CameraDevice.rear,
}) async {
String path = await pickImagePath(
String? path = await pickImagePath(
source: source,
maxWidth: maxWidth,
maxHeight: maxHeight,
Expand All @@ -38,14 +38,13 @@ class MethodChannelImagePicker extends ImagePickerPlatform {
}

@override
Future<String> pickImagePath({
@required ImageSource source,
double maxWidth,
double maxHeight,
int imageQuality,
Future<String?> pickImagePath({
required ImageSource source,
double? maxWidth,
double? maxHeight,
int? imageQuality,
CameraDevice preferredCameraDevice = CameraDevice.rear,
}) {
assert(source != null);
if (imageQuality != null && (imageQuality < 0 || imageQuality > 100)) {
throw ArgumentError.value(
imageQuality, 'imageQuality', 'must be between 0 and 100');
Expand All @@ -72,12 +71,12 @@ class MethodChannelImagePicker extends ImagePickerPlatform {
}

@override
Future<PickedFile> pickVideo({
@required ImageSource source,
Future<PickedFile?> pickVideo({
required ImageSource source,
CameraDevice preferredCameraDevice = CameraDevice.rear,
Duration maxDuration,
Duration? maxDuration,
}) async {
String path = await pickVideoPath(
String? path = await pickVideoPath(
source: source,
maxDuration: maxDuration,
preferredCameraDevice: preferredCameraDevice,
Expand All @@ -86,12 +85,11 @@ class MethodChannelImagePicker extends ImagePickerPlatform {
}

@override
Future<String> pickVideoPath({
@required ImageSource source,
Future<String?> pickVideoPath({
required ImageSource source,
CameraDevice preferredCameraDevice = CameraDevice.rear,
Duration maxDuration,
Duration? maxDuration,
}) {
assert(source != null);
return _channel.invokeMethod<String>(
'pickVideo',
<String, dynamic>{
Expand All @@ -104,7 +102,7 @@ class MethodChannelImagePicker extends ImagePickerPlatform {

@override
Future<LostData> retrieveLostData() async {
final Map<String, dynamic> result =
final Map<String, dynamic>? result =
await _channel.invokeMapMethod<String, dynamic>('retrieve');

if (result == null) {
Expand All @@ -113,23 +111,23 @@ class MethodChannelImagePicker extends ImagePickerPlatform {

assert(result.containsKey('path') ^ result.containsKey('errorCode'));

final String type = result['type'];
final String? type = result['type'];
assert(type == kTypeImage || type == kTypeVideo);

RetrieveType retrieveType;
RetrieveType? retrieveType;
if (type == kTypeImage) {
retrieveType = RetrieveType.image;
} else if (type == kTypeVideo) {
retrieveType = RetrieveType.video;
}

PlatformException exception;
PlatformException? exception;
if (result.containsKey('errorCode')) {
exception = PlatformException(
code: result['errorCode'], message: result['errorMessage']);
}

final String path = result['path'];
final String? path = result['path'];

return LostData(
file: path != null ? PickedFile(path) : null,
Expand All @@ -141,31 +139,31 @@ class MethodChannelImagePicker extends ImagePickerPlatform {
@override
// ignore: deprecated_member_use_from_same_package
Future<LostDataResponse> retrieveLostDataAsDartIoFile() async {
final Map<String, dynamic> result =
final Map<String, dynamic>? result =
await _channel.invokeMapMethod<String, dynamic>('retrieve');
if (result == null) {
// ignore: deprecated_member_use_from_same_package
return LostDataResponse.empty();
}
assert(result.containsKey('path') ^ result.containsKey('errorCode'));

final String type = result['type'];
final String? type = result['type'];
assert(type == kTypeImage || type == kTypeVideo);

RetrieveType retrieveType;
RetrieveType? retrieveType;
if (type == kTypeImage) {
retrieveType = RetrieveType.image;
} else if (type == kTypeVideo) {
retrieveType = RetrieveType.video;
}

PlatformException exception;
PlatformException? exception;
if (result.containsKey('errorCode')) {
exception = PlatformException(
code: result['errorCode'], message: result['errorMessage']);
}

final String path = result['path'];
final String? path = result['path'];

// ignore: deprecated_member_use_from_same_package
return LostDataResponse(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ abstract class ImagePickerPlatform extends PlatformInterface {
/// In Android, the MainActivity can be destroyed for various reasons. If that happens, the result will be lost
/// in this call. You can then call [retrieveLostDataAsDartIoFile] when your app relaunches to retrieve the lost data.
@Deprecated('Use pickImage instead.')
Future<String> pickImagePath({
@required ImageSource source,
double maxWidth,
double maxHeight,
int imageQuality,
Future<String?> pickImagePath({
Copy link

Choose a reason for hiding this comment

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

The docs for this method does not specify what to do when the future returns null, or what it means.

required ImageSource source,
double? maxWidth,
double? maxHeight,
int? imageQuality,
CameraDevice preferredCameraDevice = CameraDevice.rear,
}) {
throw UnimplementedError('legacyPickImage() has not been implemented.');
Expand All @@ -86,10 +86,10 @@ abstract class ImagePickerPlatform extends PlatformInterface {
/// In Android, the MainActivity can be destroyed for various fo reasons. If that happens, the result will be lost
/// in this call. You can then call [retrieveLostDataAsDartIoFile] when your app relaunches to retrieve the lost data.
@Deprecated('Use pickVideo instead.')
Future<String> pickVideoPath({
@required ImageSource source,
Future<String?> pickVideoPath({
required ImageSource source,
CameraDevice preferredCameraDevice = CameraDevice.rear,
Duration maxDuration,
Duration? maxDuration,
}) {
throw UnimplementedError('pickVideoPath() has not been implemented.');
}
Expand Down Expand Up @@ -141,11 +141,11 @@ abstract class ImagePickerPlatform extends PlatformInterface {
///
/// In Android, the MainActivity can be destroyed for various reasons. If that happens, the result will be lost
/// in this call. You can then call [retrieveLostData] when your app relaunches to retrieve the lost data.
Future<PickedFile> pickImage({
@required ImageSource source,
double maxWidth,
double maxHeight,
int imageQuality,
Future<PickedFile?> pickImage({
Copy link

Choose a reason for hiding this comment

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

Same comment about Future<PickedFile?>. We would need to either:

  1. (Preferred) Remove the nullability.
  2. Explain when the Future completes as null, and what to do in those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Added docs to explain the null value.

required ImageSource source,
double? maxWidth,
double? maxHeight,
int? imageQuality,
CameraDevice preferredCameraDevice = CameraDevice.rear,
}) {
throw UnimplementedError('pickImage() has not been implemented.');
Expand All @@ -165,10 +165,10 @@ abstract class ImagePickerPlatform extends PlatformInterface {
///
/// In Android, the MainActivity can be destroyed for various fo reasons. If that happens, the result will be lost
/// in this call. You can then call [retrieveLostData] when your app relaunches to retrieve the lost data.
Future<PickedFile> pickVideo({
@required ImageSource source,
Future<PickedFile?> pickVideo({
required ImageSource source,
CameraDevice preferredCameraDevice = CameraDevice.rear,
Duration maxDuration,
Duration? maxDuration,
}) {
throw UnimplementedError('pickVideo() has not been implemented.');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class LostDataResponse {
/// The file that was lost in a previous [pickImage] or [pickVideo] call due to MainActivity being destroyed.
///
/// Can be null if [exception] exists.
final File file;
final File? file;

/// The exception of the last [pickImage] or [pickVideo].
///
Expand All @@ -43,10 +43,10 @@ class LostDataResponse {
/// You should handle this exception as if the [pickImage] or [pickVideo] got an exception when the MainActivity was not destroyed.
///
/// Note that it is not the exception that caused the destruction of the MainActivity.
final PlatformException exception;
final PlatformException? exception;

/// Can either be [RetrieveType.image] or [RetrieveType.video];
final RetrieveType type;
final RetrieveType? type;

bool _empty = false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ abstract class PickedFileBase {
/// If `end` is present, only up to byte-index `end` will be read. Otherwise, until end of file.
///
/// In order to make sure that system resources are freed, the stream must be read to completion or the subscription on the stream must be cancelled.
Stream<Uint8List> openRead([int start, int end]) {
Stream<Uint8List> openRead([int? start, int? end]) {
Copy link

Choose a reason for hiding this comment

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

Should this be named parameters instead? openRead(end: 10) is better than openRead(null, 10)

Copy link
Member

Choose a reason for hiding this comment

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

These follow the File API. Anyway, this code will eventually be removed (it's available, and reusable in the cross_file package now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's unrelated to NNBD and I'd prefer not to break the API. Also it's going to be removed soon (probably the very next patch), let's not introduce 2 consecutive breaking changes for people to migrate.

Copy link

Choose a reason for hiding this comment

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

sgtm

throw UnimplementedError('openRead() has not been implemented.');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,19 @@ import './base.dart';
/// It wraps the bytes of a selected file.
class PickedFile extends PickedFileBase {
final String path;
final Uint8List _initBytes;
final Uint8List? _initBytes;

/// Construct a PickedFile object from its ObjectUrl.
///
/// Optionally, this can be initialized with `bytes`
/// so no http requests are performed to retrieve files later.
PickedFile(this.path, {Uint8List bytes})
PickedFile(this.path, {Uint8List? bytes})
: _initBytes = bytes,
super(path);

Future<Uint8List> get _bytes async {
if (_initBytes != null) {
return Future.value(UnmodifiableUint8ListView(_initBytes));
return Future.value(UnmodifiableUint8ListView(_initBytes!));
}
return http.readBytes(Uri.parse(path));
}
Expand All @@ -38,7 +38,7 @@ class PickedFile extends PickedFileBase {
}

@override
Stream<Uint8List> openRead([int start, int end]) async* {
Stream<Uint8List> openRead([int? start, int? end]) async* {
final bytes = await _bytes;
yield bytes.sublist(start ?? 0, end ?? bytes.length);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class PickedFile extends PickedFileBase {
}

@override
Stream<Uint8List> openRead([int start, int end]) {
Stream<Uint8List> openRead([int? start, int? end]) {
return _file
.openRead(start ?? 0, end)
.map((chunk) => Uint8List.fromList(chunk));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class LostData {
/// The file that was lost in a previous [pickImage] or [pickVideo] call due to MainActivity being destroyed.
///
/// Can be null if [exception] exists.
final PickedFile file;
final PickedFile? file;

/// The exception of the last [pickImage] or [pickVideo].
///
Expand All @@ -40,10 +40,10 @@ class LostData {
/// You should handle this exception as if the [pickImage] or [pickVideo] got an exception when the MainActivity was not destroyed.
///
/// Note that it is not the exception that caused the destruction of the MainActivity.
final PlatformException exception;
final PlatformException? exception;

/// Can either be [RetrieveType.image] or [RetrieveType.video];
final RetrieveType type;
final RetrieveType? type;
Copy link

Choose a reason for hiding this comment

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

the docs doesn't say null though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


bool _empty = false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@ description: A common platform interface for the image_picker plugin.
homepage: https://github.com/flutter/plugins/tree/master/packages/image_picker/image_picker_platform_interface
# NOTE: We strongly prefer non-breaking changes, even at the expense of a
# less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes
version: 1.1.6
version: 2.0.0-nullsafety

dependencies:
flutter:
sdk: flutter
meta: ^1.1.8
http: ^0.12.1
plugin_platform_interface: ^1.0.2
meta: ^1.3.0-nullsafety.6
http: ^0.13.0-nullsafety.0
plugin_platform_interface: ^1.1.0-nullsafety.2

dev_dependencies:
flutter_test:
sdk: flutter
mockito: ^4.1.1
pedantic: ^1.8.0+1
mockito: ^5.0.0-nullsafety.5
Copy link
Member

Choose a reason for hiding this comment

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

I checked, and mockito doesn't seem to be used in this package. It can probably be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pedantic: ^1.10.0-nullsafety.3

environment:
sdk: ">=2.5.0 <3.0.0"
sdk: ">=2.12.0-0 <3.0.0"
flutter: ">=1.10.0"
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ void main() {
final LostDataResponse response =
await picker.retrieveLostDataAsDartIoFile();
expect(response.type, RetrieveType.image);
expect(response.file.path, '/example/path');
expect(response.file?.path, '/example/path');
});

test('retrieveLostData get error response', () async {
Expand All @@ -318,8 +318,8 @@ void main() {
final LostDataResponse response =
await picker.retrieveLostDataAsDartIoFile();
expect(response.type, RetrieveType.video);
expect(response.exception.code, 'test_error_code');
expect(response.exception.message, 'test_error_message');
expect(response.exception?.code, 'test_error_code');
expect(response.exception?.message, 'test_error_message');
});

test('retrieveLostData get null response', () async {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ void main() {
// ignore: deprecated_member_use_from_same_package
final LostData response = await picker.retrieveLostData();
expect(response.type, RetrieveType.image);
expect(response.file.path, '/example/path');
expect(response.file?.path, '/example/path');
Copy link

Choose a reason for hiding this comment

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

ditto. check response.file isn't null.

});

test('retrieveLostData get error response', () async {
Expand All @@ -326,8 +326,8 @@ void main() {
// ignore: deprecated_member_use_from_same_package
final LostData response = await picker.retrieveLostData();
expect(response.type, RetrieveType.video);
expect(response.exception.code, 'test_error_code');
expect(response.exception.message, 'test_error_message');
expect(response.exception?.code, 'test_error_code');
Copy link

Choose a reason for hiding this comment

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

nit: check that response.exception is not null, and then response.exception!.code

expect(response.exception?.message, 'test_error_message');
});

test('retrieveLostData get null response', () async {
Expand Down
Loading