Skip to content

[device_info_plus] add toMap#618

Merged
miquelbeltran merged 5 commits intofluttercommunity:mainfrom
alexbatalov:device-info-to-map
Dec 15, 2021
Merged

[device_info_plus] add toMap#618
miquelbeltran merged 5 commits intofluttercommunity:mainfrom
alexbatalov:device-info-to-map

Conversation

@alexbatalov
Copy link
Copy Markdown
Contributor

@alexbatalov alexbatalov commented Nov 21, 2021

Description

This PR adds a new superclass of platform-specific device info objects. BaseDeviceInfo exposes one generic toMap method to be provided by platform-specific subclasses. Missing implementations provided for Linux and Windows. A new convenience accessor deviceInfo returns platform-specific device info. It can either be narrow-typed using is or serialized to map.

final deviceInfoPlugin = DeviceInfoPlugin();
final deviceInfo = await deviceInfoPlugin.deviceInfo;
if (deviceInfo is AndroidDeviceInfo) {
  // Android
} else if (deviceInfo is IosDeviceInfo) {
  // iOS
} else {
  // Other
}

final deviceInfoMap = deviceInfo.toMap();
// Push `deviceInfoMap` to server as a part of bug report, telemetry, etc.

Related Issues

#615

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

// found in the LICENSE file.

import 'dart:async';
import 'dart:io';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't this throw on web?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not anymore. There is still a problem with some cached compiler info, you cannot mix web and non-web builds (technically speaking runs, not builds) on a single cache. For example if you do:

$ flutter run --device-id macos

then

$ flutter run --device-id chrome

fails with a bunch of errors referring not only to dart:io, but hundreds of errors for dart:ffi and package:win32 which are transient dependencies from device_info_plus_windows. This is a result of merging #598. To mitigate it you have to flutter clean every time you switch running web vs. non-web platforms. When you do flutter build web this is not a problem, because it uses correct compiler every time it's invoked and does not depend on cached stuff.

}
}

throw UnsupportedError('Unsupported platform');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The error message probably should tell the user which the current platform is. We could also tell the user to create an issue or PR in this repo for unsupported platforms.

Also, does this need tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the idea of more detailed explanation. Most UnsupportedError exceptions in flutter/plugins or this repo simply states that current platform is well not supported, without any further explanation. Thats contrasting with assertion messages throughout Flutter, which are very verbose, suggesting fixes, or further actions.

@alexbatalov
Copy link
Copy Markdown
Contributor Author

I've added tests for new methods, missing test case for MacOsDeviceInfo and integration tests.

One thing to note on integration tests for web. There are two devices - web-server and chrome. With current driver's setup web-server simply succeeds with no additional messages:

$ flutter drive --driver=test_driver/device_info_plus_e2e_test.dart --target=./test_driver/device_info_plus_e2e.dart --device-id web-server
Running "flutter pub get" in example...                            713ms
Launching ./test_driver/device_info_plus_e2e.dart on Web Server in debug mode...
Waiting for connection from debug service on Web Server...         20.2s
./test_driver/device_info_plus_e2e.dart is being served at http://localhost:51498
The web-server device requires the Dart Debug Chrome extension for debugging. Consider using the Chrome or Edge devices for an improved development workflow.
Application finished.

That is misleading because in fact these tests were not even executed. If you run the same with chrome device you'll see that there are errors in the tests themselves:

$ flutter drive --driver=test_driver/device_info_plus_e2e_test.dart --target=./test_driver/device_info_plus_e2e.dart --device-id chrome
Running "flutter pub get" in example...                            778ms
Launching ./test_driver/device_info_plus_e2e.dart on Chrome in debug mode...
Waiting for connection from debug service on Chrome...             17.8s
This app is linked to the debug service: ws://127.0.0.1:53361/eZTcAD6QRiA=/ws
Debug service listening on ws://127.0.0.1:53361/eZTcAD6QRiA=/ws

Running with unsound null safety
For more information see https://dart.dev/null-safety/unsound-null-safety
00:00 +0: (setUpAll)
00:00 +0: (setUpAll) [E]
  Unsupported operation: Platform._operatingSystem
  // Long stack trace...
00:00 +0 -1: (tearDownAll)
00:00 +1 -1: Some tests failed.
Application finished.

The same error can be seen at some point when running chromedriver with logs on:

$ chromedriver --port=4444 --log-level=INFO
[1637571135.923][INFO]: [961691ef1023330da68da0c1606e647d] RESPONSE GetLog [ {
   "level": "WARNING",
   "message": "http://localhost:51357/dart_sdk.js 52372:16 \"registerExtension() from dart:developer is only supported in build/run/test environments where the developer event method hooks have been set.\"",
   "source": "console-api",
   "timestamp": 1.637571133822e+12
}, {
   "level": "WARNING",
   "message": "http://localhost:51357/dart_sdk.js 52351:16 \"postEvent() from dart:developer is only supported in build/run/test environments where the developer event method hooks have been set.\"",
   "source": "console-api",
   "timestamp": 1.637571134916e+12
}, {
   "level": "INFO",
   "message": "http://localhost:51357/dart_sdk.js 29445:14 \"00:00 +0: (setUpAll)\"",
   "source": "console-api",
   "timestamp": 1.637571134916e+12
}, {
   "level": "INFO",
   "message": "http://localhost:51357/dart_sdk.js 29445:14 \"00:00 +0: (setUpAll) [E]\"",
   "source": "console-api",
   "timestamp": 1.637571134916e+12
}, {
   "level": "INFO",
   "message": "http://localhost:51357/dart_sdk.js 29445:14 \"  Unsupported operation: Platform._operatingSystem\"",
   "source": "console-api",
   "timestamp": 1.637571134917e+12
}, {
   "level": "INFO",
   "message": "http://localhost:51357/dart_sdk.js 29445:14 \"  dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 251:49      throw_\\n  dart-sdk/lib/_internal/js_dev_runtime/patch/io_patch.dart ...",
   "source": "console-api",
   "timestamp": 1.63757113504e+12
}, {
   "level": "INFO",
   "message": "http://localhost:51357/dart_sdk.js 29445:14 \"Consider enabling the flag chain-stack-traces to receive more detailed exceptions.\\nFor example, 'dart test --chain-stack-traces'.\"",
   "source": "console-api",
   "timestamp": 1.63757113504e+12
}, {
   "level": "INFO",
   "message": "http://localhost:51357/dart_sdk.js 29445:14 \"00:00 +0 -1: (tearDownAll)\"",
   "source": "console-api",
   "timestamp": 1.637571135041e+12
}, {
   "level": "INFO",
   "message": "http://localhost:51357/dart_sdk.js 29445:14 \"00:00 +1 -1: Some tests failed.\"",
   "source": "console-api",
   "timestamp": 1.637571135116e+12
} ]

That's because integration test itself used Platform getters without first checking if running on the web (via kIsWeb). I've fixed that as well.

Copy link
Copy Markdown
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

I like the idea! not having a common class was something that always annoyed me a bit when using this plugin.

The PR should include also the change in the Changelog and increasing the version number by a minor version (since it is not a breaking change) Could you add that before we merge?

@miquelbeltran miquelbeltran merged commit 8b2f049 into fluttercommunity:main Dec 15, 2021
@alexbatalov alexbatalov deleted the device-info-to-map branch January 19, 2022 12:39
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants