Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions packages/device_info/device_info/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.5.0-nullsafety

* Migrate to null safety.

## 0.4.2+9

* Update android compileSdkVersion to 29.
Expand Down
4 changes: 4 additions & 0 deletions packages/device_info/device_info/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
include: ../../../analysis_options.yaml
analyzer:
enable-experiment:
- non-nullable
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

// TODO(cyanglaz): Remove once https://github.com/flutter/plugins/pull/3158 is landed.
// @dart = 2.9
import 'dart:io';
import 'package:flutter_test/flutter_test.dart';
import 'package:device_info/device_info.dart';
Expand Down
2 changes: 1 addition & 1 deletion packages/device_info/device_info/example/lib/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class _MyAppState extends State<MyApp> {
}

Future<void> initPlatformState() async {
Map<String, dynamic> deviceData;
Map<String, dynamic> deviceData = <String, dynamic>{};

try {
if (Platform.isAndroid) {
Expand Down
5 changes: 4 additions & 1 deletion packages/device_info/device_info/example/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ dev_dependencies:
sdk: flutter
integration_test:
path: ../../../integration_test
pedantic: ^1.8.0
pedantic: ^1.10.0-nullsafety.1

flutter:
uses-material-design: true

environment:
sdk: '>=2.10.0-56.0.dev <3.0.0'
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

// TODO(cyanglaz): Remove once https://github.com/flutter/flutter/issues/59879 is fixed.
// @dart = 2.9
import 'dart:async';
import 'dart:convert';
import 'dart:io';
Expand Down
4 changes: 2 additions & 2 deletions packages/device_info/device_info/lib/device_info.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class DeviceInfoPlugin {
DeviceInfoPlugin();

/// This information does not change from call to call. Cache it.
AndroidDeviceInfo _cachedAndroidDeviceInfo;
AndroidDeviceInfo? _cachedAndroidDeviceInfo;

/// Information derived from `android.os.Build`.
///
Expand All @@ -25,7 +25,7 @@ class DeviceInfoPlugin {
await DeviceInfoPlatform.instance.androidInfo();

/// This information does not change from call to call. Cache it.
IosDeviceInfo _cachedIosDeviceInfo;
IosDeviceInfo? _cachedIosDeviceInfo;

/// Information derived from `UIDevice`.
///
Expand Down
14 changes: 9 additions & 5 deletions packages/device_info/device_info/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ homepage: https://github.com/flutter/plugins/tree/master/packages/device_info
# 0.4.y+z is compatible with 1.0.0, if you land a breaking change bump
# the version to 2.0.0.
# See more details: https://github.com/flutter/flutter/wiki/Package-migration-to-1.0.0
version: 0.4.2+9
version: 0.5.0-nullsafety

# Don't publish this package until null safety is stable or the package is in the allow list.
publish_to: none

flutter:
plugin:
Expand All @@ -19,14 +22,15 @@ flutter:
dependencies:
flutter:
sdk: flutter
device_info_platform_interface: ^1.0.0
device_info_platform_interface:
path: ../device_info_platform_interface
Copy link

Choose a reason for hiding this comment

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

We are going with git dependencies, but feel free to change it after this PR is merged.


dev_dependencies:
test: ^1.3.0
test: ^1.10.0-nullsafety.1
flutter_test:
sdk: flutter
pedantic: ^1.8.0
pedantic: ^1.10.0-nullsafety.1

environment:
sdk: ">=2.1.0<3.0.0"
sdk: '>=2.10.0-56.0.dev <3.0.0'
flutter: ">=1.12.13+hotfix.5 <2.0.0"
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 1.1.0-nullsafety

* Migrate to null safety.

## 1.0.1

- Documentation typo fixed.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
include: ../../../analysis_options.yaml
analyzer:
enable-experiment:
- non-nullable
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,28 @@
class AndroidDeviceInfo {
/// Android device Info class.
AndroidDeviceInfo({
this.version,
this.board,
this.bootloader,
this.brand,
this.device,
this.display,
this.fingerprint,
this.hardware,
this.host,
this.id,
this.manufacturer,
this.model,
this.product,
List<String> supported32BitAbis,
List<String> supported64BitAbis,
List<String> supportedAbis,
this.tags,
this.type,
this.isPhysicalDevice,
this.androidId,
List<String> systemFeatures,
}) : supported32BitAbis = List<String>.unmodifiable(supported32BitAbis),
required this.version,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have forked this package internally and my migration followed a different path. I am not a fan of making all these fields required for two reasons:

  • Constructing the object (for tests) becomes painful. Consider an app test that only needs to set manufacturer. They now have to initialize all the other fields in their tests or use the obscure Map constructor.
  • Semantically, these fields are not required to exist. They can be empty. To reflect that, we are initializing them to empty string. As an app developer, I would much rather have a signal that a field is nullable rather than having it be an empty string which does not provide compile-time safety.

In my migration, I marked all these fields as nullable. However, this failed tests because gallery uses this code and assumes they are not nullable.

Copy link

Choose a reason for hiding this comment

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

Semantically, these fields are not required to exist. They can be empty. To reflect that, we are initializing them to empty string.

If it's referring to usage of Map, this is a limitation of the implementation.

When looking at this, we were trying to answer the question of what is a valid AndroidDeviceInfo based on the information available from the platform. If something changes in future versions of Android, then that is a breaking change to the API exposed to Dart.

In my opinion, if everything is nullable by default, then the API becomes some sort of "best-effort". "we think version is available, but we can't guarantee that".

This was the rationale behind it. That said, could you point to the cases where testing was painful?

Copy link
Contributor

Choose a reason for hiding this comment

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

we were trying to answer the question of what is a valid AndroidDeviceInfo based on the information available from the platform.

That means these fields are not supposed to be nullable. In that case, it would be far better to construct them via map values like so:

   someField: map[key]!,
   // rather than
   // someField: map[key] ?? '',

The code, today, reads like any one of these values can be non-existent and that's OK.

The test pain is a secondary reason. If these are truly non-nullable values, you can always expose a forTest constructor that effectively does what your map constructor is doing.

Copy link

Choose a reason for hiding this comment

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

someField: map[key]! makes sense to me. We should fix it. This is an area where I think Pigeon could help, but that is a different discussion 😃

required this.board,
required this.bootloader,
required this.brand,
required this.device,
required this.display,
required this.fingerprint,
required this.hardware,
required this.host,
required this.id,
required this.manufacturer,
required this.model,
required this.product,
required List<String> supported32BitAbis,
required List<String> supported64BitAbis,
required List<String> supportedAbis,
required this.tags,
required this.type,
required this.isPhysicalDevice,
required this.androidId,
required List<String> systemFeatures,
}) : supported32BitAbis = List<String>.unmodifiable(supported32BitAbis),
supported64BitAbis = List<String>.unmodifiable(supported64BitAbis),
supportedAbis = List<String>.unmodifiable(supportedAbis),
systemFeatures = List<String>.unmodifiable(systemFeatures);
Expand Down Expand Up @@ -115,25 +115,25 @@ class AndroidDeviceInfo {
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'],
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'],
tags: map['tags'] ?? '',
type: map['type'] ?? '',
isPhysicalDevice: map['isPhysicalDevice'] ?? false,
androidId: map['androidId'] ?? '',
systemFeatures: _fromList(map['systemFeatures'] ?? []),
);
}
Expand All @@ -151,13 +151,13 @@ class AndroidDeviceInfo {
/// See: https://developer.android.com/reference/android/os/Build.VERSION.html
class AndroidBuildVersion {
AndroidBuildVersion._({
this.baseOS,
this.codename,
this.incremental,
this.previewSdkInt,
this.release,
this.sdkInt,
this.securityPatch,
required this.baseOS,
required this.codename,
required this.incremental,
required this.previewSdkInt,
required this.release,
required this.sdkInt,
required this.securityPatch,
});

/// The base OS build the product is based on.
Expand Down Expand Up @@ -186,13 +186,13 @@ class AndroidBuildVersion {
/// Deserializes from the map message received from [_kChannel].
static AndroidBuildVersion _fromMap(Map<String, dynamic> map) {
return AndroidBuildVersion._(
baseOS: map['baseOS'],
codename: map['codename'],
incremental: map['incremental'],
previewSdkInt: map['previewSdkInt'],
release: map['release'],
sdkInt: map['sdkInt'],
securityPatch: map['securityPatch'],
baseOS: map['baseOS'] ?? '',
codename: map['codename'] ?? '',
incremental: map['incremental'] ?? '',
previewSdkInt: map['previewSdkInt'] ?? 0,
release: map['release'] ?? '',
sdkInt: map['sdkInt'] ?? 0,
securityPatch: map['securityPatch'] ?? '',
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@
class IosDeviceInfo {
/// IOS device info class.
IosDeviceInfo({
this.name,
this.systemName,
this.systemVersion,
this.model,
this.localizedModel,
this.identifierForVendor,
this.isPhysicalDevice,
this.utsname,
required this.name,
required this.systemName,
required this.systemVersion,
required this.model,
required this.localizedModel,
required this.identifierForVendor,
required this.isPhysicalDevice,
required this.utsname,
});

/// Device name.
Expand Down Expand Up @@ -45,12 +45,12 @@ 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'],
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>() ?? {}),
Expand All @@ -62,11 +62,11 @@ class IosDeviceInfo {
/// See http://pubs.opengroup.org/onlinepubs/7908799/xsh/sysutsname.h.html for details.
class IosUtsname {
IosUtsname._({
this.sysname,
this.nodename,
this.release,
this.version,
this.machine,
required this.sysname,
required this.nodename,
required this.release,
required this.version,
required this.machine,
});

/// Operating system name.
Expand All @@ -87,11 +87,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'] ?? '',
);
}
}
16 changes: 10 additions & 6 deletions packages/device_info/device_info_platform_interface/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,24 @@ description: A common platform interface for the device_info plugin.
homepage: https://github.com/flutter/plugins/tree/master/packages/device_info
# 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.0.1
version: 1.1.0-nullsafety

# Don't publish this package until null safety is stable or the package is in the allow list.
publish_to: none

dependencies:
flutter:
sdk: flutter
meta: ^1.1.8
plugin_platform_interface: ^1.0.2
meta: ^1.3.0-nullsafety.3
plugin_platform_interface:
path: ../../plugin_platform_interface

dev_dependencies:
flutter_test:
sdk: flutter
mockito: ^4.1.1
pedantic: ^1.8.0
test: ^1.10.0-nullsafety.1
pedantic: ^1.10.0-nullsafety.1

environment:
sdk: ">=2.7.0 <3.0.0"
sdk: '>=2.10.0-56.0.dev <3.0.0'
flutter: ">=1.9.1+hotfix.4 <2.0.0"
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// TODO(cyanglaz): Remove once https://github.com/flutter/flutter/issues/59879 is fixed.
// @dart = 2.9
import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';

Expand Down
2 changes: 2 additions & 0 deletions script/incremental_build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ CUSTOM_ANALYSIS_PLUGINS=(
"video_player/video_player_web"
"google_maps_flutter/google_maps_flutter_web"
"url_launcher/url_launcher_platform_interface"
"device_info/device_info_platform_interface"
"device_info/device_info"
)
# Comma-separated string of the list above
readonly CUSTOM_FLAG=$(IFS=, ; echo "${CUSTOM_ANALYSIS_PLUGINS[*]}")
Expand Down