[device_info_platform_interface][device_info] nnbd

[cyanglaz]

Migrate to NNBD

[blasten]

We would need to opt out the driver tests in the PR for device_info until integration_test and vm_service are migrated to null safety.

FYI @dnfield

[dnfield]

@bkonyi what's the plan for null safe vm_service?

[blasten]

similar comment about nullability.

[blasten]

It seems like only baseOS, previewSdkInt, and securityPatch should be nullable:

plugins/packages/device_info/device_info/android/src/main/java/io/flutter/plugins/deviceinfo/MethodCallHandlerImpl.java

Line 39 in 42b33ec

if (call.method.equals("getAndroidDeviceInfo")) {

[cyanglaz]

@blasten Updated per review comments, PTAL

[blasten]

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

[blasten]

LGTM

[mehmetf]

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.

[blasten]

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?

[mehmetf]

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.

[blasten]

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 😃