Skip to content
This repository has been 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
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 10 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
2 changes: 0 additions & 2 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 @@ -7,10 +7,8 @@ import 'dart:async';
import 'package:plugin_platform_interface/plugin_platform_interface.dart';

Copy link
Contributor

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

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

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
16 changes: 9 additions & 7 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,11 @@
// 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 +16,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>{});
}
}
104 changes: 75 additions & 29 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 @@ -38,39 +38,63 @@ class AndroidDeviceInfo {
final AndroidBuildVersion version;

/// The name of the underlying board, like "goldfish".
///
/// The value is an empty String if it is not available.
final String board;

/// The system bootloader version number.
///
/// The value is an empty String if it is not available.
final String bootloader;

/// The consumer-visible brand with which the product/hardware will be associated, if any.
///
/// The value is an empty String if it is not available.
final String brand;

/// The name of the industrial design.
///
/// The value is an empty String if it is not available.
final String device;

/// A build ID string meant for displaying to the user.
///
/// The value is an empty String if it is not available.
final String display;

/// A string that uniquely identifies this build.
///
/// The value is an empty String if it is not available.
final String fingerprint;

/// The name of the hardware (from the kernel command line or /proc).
///
/// The value is an empty String if it is not available.
final String hardware;

/// Hostname.
///
/// The value is an empty String if it is not available.
final String host;

/// Either a changelist number, or a label like "M4-rc20".
///
/// The value is an empty String if it is not available.
final String id;

/// The manufacturer of the product/hardware.
///
/// The value is an empty String if it is not available.
final String manufacturer;

/// The end-user-visible name for the end product.
///
/// The value is an empty String if it is not available.
final String model;

/// The name of the overall product.
///
/// The value is an empty String if it is not available.
final String product;

/// An ordered list of 32 bit ABIs supported by this device.
Expand All @@ -83,15 +107,23 @@ class AndroidDeviceInfo {
final List<String> supportedAbis;

/// Comma-separated tags describing the build, like "unsigned,debug".
///
/// The value is an empty String if it is not available.
final String tags;

/// The type of build, like "user" or "eng".
///
/// The value is an empty String if it is not available.
final String type;

/// `false` if the application is running in an emulator, `true` otherwise.
/// The value is `true` if the application is running on a physical device.
///
/// The value is `false` when the application is running on a emulator, or the value is unavailable.
final bool isPhysicalDevice;

/// The Android hardware device ID that is unique between the device + user and app signing.
///
/// The value is an empty String if it is not available.
final String androidId;

/// Describes what features are available on the current device.
Expand All @@ -113,35 +145,41 @@ 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

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

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

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

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

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

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;
return List<String>.from(list);
if (message == null) {
return <String>[];
}
assert(message is List<dynamic>);
final List<dynamic> list = List<dynamic>.from(message)
..removeWhere((value) => value == null);
return list.cast<String>();
}
}

Expand Down Expand Up @@ -173,17 +211,25 @@ class AndroidBuildVersion {
final String? securityPatch;

/// The current development codename, or the string "REL" if this is a release build.
///
/// The value is an empty String if it is not available.
final String codename;

/// The internal value used by the underlying source control to represent this build.
///
/// The value is an empty String if it is not available.
final String incremental;

/// The user-visible version string.
///
/// The value is an empty String if it is not available.
final String release;

/// The user-visible SDK version of the framework.
///
/// Possible values are defined in: https://developer.android.com/reference/android/os/Build.VERSION_CODES.html
///
/// The value is -1 if it is unavailable.
final int sdkInt;

/// Deserializes from the map message received from [_kChannel].
Expand All @@ -192,10 +238,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,
);
}
}
58 changes: 44 additions & 14 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 @@ -19,40 +19,60 @@ class IosDeviceInfo {
});

/// Device name.
///
/// The value is an empty String if it is not available.
final String name;

/// The name of the current operating system.
///
/// The value is an empty String if it is not available.
final String systemName;

/// The current operating system version.
///
/// The value is an empty String if it is not available.
final String systemVersion;

/// Device model.
///
/// The value is an empty String if it is not available.
final String model;

/// Localized name of the device model.
///
/// The value is an empty String if it is not available.
final String localizedModel;

/// Unique UUID value identifying the current device.
///
/// The value is an empty String if it is not available.
final String identifierForVendor;

/// `false` if the application is running in a simulator, `true` otherwise.
/// The value is `true` if the application is running on a physical device.
///
/// The value is `false` when the application is running on a simulator, or the value is unavailable.
final bool isPhysicalDevice;

/// Operating system information derived from `sys/utsname.h`.
///
/// The value is an empty String if it is not available.
final IosUtsname utsname;

/// 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 All @@ -69,28 +89,38 @@ class IosUtsname {
});

/// Operating system name.
///
/// The value is an empty String if it is not available.
final String sysname;

/// Network node name.
///
/// The value is an empty String if it is not available.
final String nodename;

/// Release level.
///
/// The value is an empty String if it is not available.
final String release;

/// Version level.
///
/// The value is an empty String if it is not available.
final String version;

/// Hardware type (e.g. 'iPhone7,1' for iPhone 6 Plus).
///
/// The value is an empty String if it is not available.
final String machine;

/// 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'] ?? '',
);
}
}
Loading