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

[device_info_platform_interface] handle null value from method channel #3609

Merged
cyanglaz merged 11 commits into from
Feb 23, 2021
Merged

[device_info_platform_interface] handle null value from method channel #3609

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6e9e671
migrate platform interface
Feb 19, 2021
9ba17a7
fix nullable types
Feb 23, 2021
c389a6a
format
Feb 23, 2021
a9f0a0a
revert test code in device info
Feb 23, 2021
c6a15a8
revert test code
Feb 23, 2021
7b5658b
Merge branch 'master' into nnbd_device_info
Feb 23, 2021
fefc6db
some cleanup
Feb 23, 2021
f69f03e
Merge branch 'nnbd_device_info' of github.com:cyanglaz/plugins into n…
Feb 23, 2021
dedd740
review
Feb 23, 2021
3d351f1
add docs to explain empty value
Feb 23, 2021
d7a8a6f
Update packages/device_info/device_info_platform_interface/test/metho…
Feb 23, 2021
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
4 changes: 0 additions & 4 deletions packages/device_info/device_info_platform_interface/lib/device_info_platform_interface.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,10 @@
// found in the LICENSE file.

import 'dart:async';

import 'package:plugin_platform_interface/plugin_platform_interface.dart';

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 23, 2021

Choose a reason for hiding this comment

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

Don't we normally leave blank lines between the dart:, package:, and local blocks?

Copy link
Contributor Author

@cyanglaz cyanglaz Feb 23, 2021

Choose a reason for hiding this comment

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

Oh, i didn't know that's a thing. I will add the line back.

Copy link
Contributor Author

@cyanglaz cyanglaz Feb 23, 2021

Choose a reason for hiding this comment

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

done

import 'method_channel/method_channel_device_info.dart';

import 'model/android_device_info.dart';
import 'model/ios_device_info.dart';

export 'model/android_device_info.dart';
export 'model/ios_device_info.dart';

Expand Down
17 changes: 9 additions & 8 deletions ...ce_info/device_info_platform_interface/lib/method_channel/method_channel_device_info.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import 'dart:async';
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';
import 'package:flutter/services.dart';
import 'package:meta/meta.dart';

import 'package:device_info_platform_interface/device_info_platform_interface.dart';

/// An implementation of [DeviceInfoPlatform] that uses method channels.
Expand All @@ -13,16 +15,15 @@ class MethodChannelDeviceInfo extends DeviceInfoPlatform {

// Method channel for Android devices
Future<AndroidDeviceInfo> androidInfo() async {
return AndroidDeviceInfo.fromMap(
(await channel.invokeMethod('getAndroidDeviceInfo'))
.cast<String, dynamic>(),
);
return AndroidDeviceInfo.fromMap((await channel
.invokeMapMethod<String, dynamic>('getAndroidDeviceInfo')) ??
<String, dynamic>{});
}

// Method channel for iOS devices
Future<IosDeviceInfo> iosInfo() async {
return IosDeviceInfo.fromMap(
(await channel.invokeMethod('getIosDeviceInfo')).cast<String, dynamic>(),
);
(await channel.invokeMapMethod<String, dynamic>('getIosDeviceInfo')) ??
<String, dynamic>{});
}
}
60 changes: 33 additions & 27 deletions packages/device_info/device_info_platform_interface/lib/model/android_device_info.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,34 +113,40 @@ class AndroidDeviceInfo {
/// Deserializes from the message received from [_kChannel].
static AndroidDeviceInfo fromMap(Map<String, dynamic> map) {
return AndroidDeviceInfo(
version:
AndroidBuildVersion._fromMap(map['version']!.cast<String, dynamic>()),
board: map['board']!,
bootloader: map['bootloader']!,
brand: map['brand']!,
device: map['device']!,
display: map['display']!,
fingerprint: map['fingerprint']!,
hardware: map['hardware']!,
host: map['host']!,
id: map['id']!,
manufacturer: map['manufacturer']!,
model: map['model']!,
product: map['product']!,
supported32BitAbis: _fromList(map['supported32BitAbis']!),
supported64BitAbis: _fromList(map['supported64BitAbis']!),
supportedAbis: _fromList(map['supportedAbis']!),
tags: map['tags']!,
type: map['type']!,
isPhysicalDevice: map['isPhysicalDevice']!,
androidId: map['androidId']!,
systemFeatures: _fromList(map['systemFeatures']!),
version: AndroidBuildVersion._fromMap(map['version'] != null
? map['version'].cast<String, dynamic>()
: <String, dynamic>{}),
board: map['board'] ?? '',
bootloader: map['bootloader'] ?? '',
brand: map['brand'] ?? '',
device: map['device'] ?? '',
Copy link

@blasten blasten Feb 23, 2021

Choose a reason for hiding this comment

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

The empty string means that there's a bug on the platform side right? I think we should have a tracking bug to address this with Pigeon, and add TODOs. We should really try to fix this with Pigeon because I can see this being a source of bugs.

Copy link
Contributor Author

@cyanglaz cyanglaz Feb 23, 2021

Choose a reason for hiding this comment

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

Why does it have anything to do with Pigeon? This plugin doesn't use pigeon.
Anyways, from dart perspective, we should always assume the method channel could return null. Even the native API doesn't return null now, it could in the future as we don't use null safe language on native side.

Copy link

@blasten blasten Feb 23, 2021

Choose a reason for hiding this comment

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

I guess I was referring to the Dart API. If the empty string is documented on the Android official website, then it's correct to provide empty string values. The point about Pigeon is about making this enforcement more explicit upstream, so you don't have to manually deal with it in Dart.

Copy link
Contributor Author

@cyanglaz cyanglaz Feb 23, 2021

Choose a reason for hiding this comment

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

The point about Pigeon is about making this enforcement more explicit upstream, so you don't have to manually deal with it in Dart.

Did you mean when we eventually migrate this plugin to pigeon?

If the empty string is documented on the Android official website, then it's correct to provide empty string values.

It's not documented. I also don't want to make everything nullable either. I think in a null safe language, it is usually the case that we don't want to make a string nullable if empty string doesn't have a specific meaning.

Copy link

@blasten blasten Feb 23, 2021

Choose a reason for hiding this comment

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

yes, eventual migration. The suggestion is to look at the official documentation for explicit mentions of "null". For example, the bug that Jason was fixing in Java. https://developer.android.com/reference/android/content/pm/FeatureInfo#name The docs explicitly say that it can be null.

I think that is the type of work that will match the intended behavior of the platform.

Copy link
Contributor Author

@cyanglaz cyanglaz Feb 23, 2021

Choose a reason for hiding this comment

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

Most of the properties in the documentation doesn't say it is nullable or non-null. See https://developer.android.com/reference/kotlin/android/os/Build

So since they are strings, I'd prefer making them non-null and empty string for "no value" on dart side.

display: map['display'] ?? '',
fingerprint: map['fingerprint'] ?? '',
hardware: map['hardware'] ?? '',
host: map['host'] ?? '',
id: map['id'] ?? '',
manufacturer: map['manufacturer'] ?? '',
model: map['model'] ?? '',
product: map['product'] ?? '',
supported32BitAbis: _fromList(map['supported32BitAbis']),
supported64BitAbis: _fromList(map['supported64BitAbis']),
supportedAbis: _fromList(map['supportedAbis']),
tags: map['tags'] ?? '',
type: map['type'] ?? '',
isPhysicalDevice: map['isPhysicalDevice'] ?? false,
androidId: map['androidId'] ?? '',
systemFeatures: _fromList(map['systemFeatures']),
);
}

/// Deserializes message as List<String>
static List<String> _fromList(dynamic message) {
final List<dynamic> list = message;
if (message == null) {
return <String>[];
}
assert(message is List<dynamic>);
final List<dynamic> list = List<dynamic>.from(message)
..removeWhere((value) => value == null);
return List<String>.from(list);
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 23, 2021

Choose a reason for hiding this comment

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

Couldn't this just be a .cast<String>() since you're copying in the new step just above?

Copy link
Contributor Author

@cyanglaz cyanglaz Feb 23, 2021

Choose a reason for hiding this comment

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

done

}
}
Expand Down Expand Up @@ -192,10 +198,10 @@ class AndroidBuildVersion {
baseOS: map['baseOS'],
previewSdkInt: map['previewSdkInt'],
securityPatch: map['securityPatch'],
codename: map['codename']!,
incremental: map['incremental']!,
release: map['release']!,
sdkInt: map['sdkInt']!,
codename: map['codename'] ?? '',
incremental: map['incremental'] ?? '',
release: map['release'] ?? '',
sdkInt: map['sdkInt'] ?? -1,
);
}
}
30 changes: 17 additions & 13 deletions packages/device_info/device_info_platform_interface/lib/model/ios_device_info.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,18 @@ class IosDeviceInfo {
/// Deserializes from the map message received from [_kChannel].
static IosDeviceInfo fromMap(Map<String, dynamic> map) {
return IosDeviceInfo(
name: map['name']!,
systemName: map['systemName']!,
systemVersion: map['systemVersion']!,
model: map['model']!,
localizedModel: map['localizedModel']!,
identifierForVendor: map['identifierForVendor']!,
isPhysicalDevice: map['isPhysicalDevice'] == 'true',
utsname: IosUtsname._fromMap(map['utsname']!.cast<String, dynamic>()),
name: map['name'] ?? '',
systemName: map['systemName'] ?? '',
systemVersion: map['systemVersion'] ?? '',
model: map['model'] ?? '',
localizedModel: map['localizedModel'] ?? '',
identifierForVendor: map['identifierForVendor'] ?? '',
isPhysicalDevice: map['isPhysicalDevice'] != null
? map['isPhysicalDevice'] == 'true'
: false,
utsname: IosUtsname._fromMap(map['utsname'] != null
? map['utsname'].cast<String, dynamic>()
: <String, dynamic>{}),
);
}
}
Expand Down Expand Up @@ -86,11 +90,11 @@ class IosUtsname {
/// Deserializes from the map message received from [_kChannel].
static IosUtsname _fromMap(Map<String, dynamic> map) {
return IosUtsname._(
sysname: map['sysname']!,
nodename: map['nodename']!,
release: map['release']!,
version: map['version']!,
machine: map['machine']!,
sysname: map['sysname'] ?? '',
nodename: map['nodename'] ?? '',
release: map['release'] ?? '',
version: map['version'] ?? '',
machine: map['machine'] ?? '',
);
}
}
216 changes: 216 additions & 0 deletions ...ages/device_info/device_info_platform_interface/test/method_channel_device_info_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,220 @@ void main() {
expect(result.utsname.machine, "x86_64");
});
});

group(
"$MethodChannelDeviceInfo handles null value in the map returned from method channel",
() {
MethodChannelDeviceInfo methodChannelDeviceInfo;

setUp(() async {
methodChannelDeviceInfo = MethodChannelDeviceInfo();

methodChannelDeviceInfo.channel
.setMockMethodCallHandler((MethodCall methodCall) async {
switch (methodCall.method) {
case 'getAndroidDeviceInfo':
return ({
"version": null,
"board": null,
"bootloader": null,
"brand": null,
"device": null,
"display": null,
"fingerprint": null,
"hardware": null,
"host": null,
"id": null,
"manufacturer": null,
"model": null,
"product": null,
"supported32BitAbis": null,
"supported64BitAbis": null,
"supportedAbis": null,
"tags": null,
"type": null,
"isPhysicalDevice": null,
"androidId": null,
"systemFeatures": null,
});
case 'getIosDeviceInfo':
return ({
"name": null,
"systemName": null,
"systemVersion": null,
"model": null,
"localizedModel": null,
"identifierForVendor": null,
"isPhysicalDevice": null,
"utsname": null,
});
default:
return null;
}
});
});

test("androidInfo hanels null", () async {
final AndroidDeviceInfo result =
await methodChannelDeviceInfo.androidInfo();

expect(result.version.securityPatch, null);
expect(result.version.sdkInt, -1);
expect(result.version.release, '');
expect(result.version.previewSdkInt, null);
expect(result.version.incremental, '');
expect(result.version.codename, '');
expect(result.board, '');
expect(result.bootloader, '');
expect(result.brand, '');
expect(result.device, '');
expect(result.display, '');
expect(result.fingerprint, '');
expect(result.hardware, '');
expect(result.host, '');
expect(result.id, '');
expect(result.manufacturer, '');
expect(result.model, '');
expect(result.product, '');
expect(result.supported32BitAbis, <String>[]);
expect(result.supported64BitAbis, <String>[]);
expect(result.supportedAbis, <String>[]);
expect(result.tags, '');
expect(result.type, '');
expect(result.isPhysicalDevice, false);
expect(result.androidId, '');
expect(result.systemFeatures, <String>[]);
});

test("iosInfo handles null", () async {
final IosDeviceInfo result = await methodChannelDeviceInfo.iosInfo();
expect(result.name, '');
expect(result.systemName, '');
expect(result.systemVersion, '');
expect(result.model, '');
expect(result.localizedModel, '');
expect(result.identifierForVendor, '');
expect(result.isPhysicalDevice, false);
expect(result.utsname.sysname, '');
expect(result.utsname.nodename, '');
expect(result.utsname.release, '');
expect(result.utsname.version, '');
expect(result.utsname.machine, '');
});
});

group("$MethodChannelDeviceInfo handles method channel returns null", () {
MethodChannelDeviceInfo methodChannelDeviceInfo;

setUp(() async {
methodChannelDeviceInfo = MethodChannelDeviceInfo();

methodChannelDeviceInfo.channel
.setMockMethodCallHandler((MethodCall methodCall) async {
switch (methodCall.method) {
case 'getAndroidDeviceInfo':
return null;
case 'getIosDeviceInfo':
return null;
default:
return null;
}
});
});

test("androidInfo hanels null", () async {
final AndroidDeviceInfo result =
await methodChannelDeviceInfo.androidInfo();

expect(result.version.securityPatch, null);
expect(result.version.sdkInt, -1);
expect(result.version.release, '');
expect(result.version.previewSdkInt, null);
expect(result.version.incremental, '');
expect(result.version.codename, '');
expect(result.board, '');
expect(result.bootloader, '');
expect(result.brand, '');
expect(result.device, '');
expect(result.display, '');
expect(result.fingerprint, '');
expect(result.hardware, '');
expect(result.host, '');
expect(result.id, '');
expect(result.manufacturer, '');
expect(result.model, '');
expect(result.product, '');
expect(result.supported32BitAbis, <String>[]);
expect(result.supported64BitAbis, <String>[]);
expect(result.supportedAbis, <String>[]);
expect(result.tags, '');
expect(result.type, '');
expect(result.isPhysicalDevice, false);
expect(result.androidId, '');
expect(result.systemFeatures, <String>[]);
});

test("iosInfo handles null", () async {
final IosDeviceInfo result = await methodChannelDeviceInfo.iosInfo();
expect(result.name, '');
expect(result.systemName, '');
expect(result.systemVersion, '');
expect(result.model, '');
expect(result.localizedModel, '');
expect(result.identifierForVendor, '');
expect(result.isPhysicalDevice, false);
expect(result.utsname.sysname, '');
expect(result.utsname.nodename, '');
expect(result.utsname.release, '');
expect(result.utsname.version, '');
expect(result.utsname.machine, '');
});
});

group("$MethodChannelDeviceInfo android handles null values in list", () {
MethodChannelDeviceInfo methodChannelDeviceInfo;

setUp(() async {
methodChannelDeviceInfo = MethodChannelDeviceInfo();

methodChannelDeviceInfo.channel
.setMockMethodCallHandler((MethodCall methodCall) async {
switch (methodCall.method) {
case 'getAndroidDeviceInfo':
return ({
"supported32BitAbis": <String>["x86", null],
"supported64BitAbis": <String>["x86_64", null],
"supportedAbis": <String>["x86_64", "x86", null],
"systemFeatures": <String>[
"android.hardware.sensor.proximity",
"android.software.adoptable_storage",
"android.hardware.sensor.accelerometer",
"android.hardware.faketouch",
"android.software.backup",
"android.hardware.touchscreen",
null
],
});
default:
return null;
}
});
});

test("androidInfo hanels null in list", () async {
final AndroidDeviceInfo result =
await methodChannelDeviceInfo.androidInfo();
expect(result.supported32BitAbis, <String>['x86']);
expect(result.supported64BitAbis, <String>['x86_64']);
expect(result.supportedAbis, <String>['x86_64', 'x86']);
expect(result.systemFeatures, <String>[
"android.hardware.sensor.proximity",
"android.software.adoptable_storage",
"android.hardware.sensor.accelerometer",
"android.hardware.faketouch",
"android.software.backup",
"android.hardware.touchscreen"
]);
});
});
}