Fix a number more issues by enabling Dart code metrics.

Author: jacob314

analysis_options.yaml

@@ -1,7 +1,4 @@
 analyzer:
-  #strong-mode:
-    #implicit-casts: false # 367 issues
-    #implicit-dynamic: false # 169 issues
   #language:
     #strict-inference: true # 34 issues
     #strict-raw-types: true # 103 issues
@@ -144,3 +141,94 @@ linter:
     # - use_to_and_as_if_applicable # has false positives, so we prefer to catch this by code-review
     - valid_regexps
     # - void_checks # not yet tested
+
+dart_code_metrics:
+  metrics:
+    cyclomatic-complexity: 20
+    number-of-parameters: 4
+    maximum-nesting-level: 5
+  metrics-exclude:
+    - test/**
+  rules:
+#    - arguments-ordering Too strict
+    - avoid-banned-imports
+    - avoid-cascade-after-if-null
+    - avoid-collection-methods-with-unrelated-types
+    - avoid-duplicate-exports
+    - avoid-dynamic
+#    - avoid-global-state   TODO(jacobr): bunch of false positives around boolean flags.
+#    - avoid-ignoring-return-values
+#    - avoid-late-keyword
+    - avoid-missing-enum-constant-in-map
+    - avoid-nested-conditional-expressions
+#    - avoid-non-ascii-symbols  TODO(jacobr): probably worth enabling.
+#    - avoid-non-null-assertion
+#    - avoid-passing-async-when-sync-expected TODO(jacobr): consider re-enabliing.
+    - avoid-redundant-async
+    - avoid-throw-in-catch-block
+
+#    - avoid-top-level-members-in-tests Doesn't seem to match our style.
+    - avoid-unnecessary-type-assertions
+    - avoid-unnecessary-type-casts
+    - avoid-unrelated-type-assertions
+    - avoid-unused-parameters
+    - ban-name
+    # - binary-expression-operand-order Some nice catches but too many false positives to enable.
+    - double-literal-format
+    # - format-comment TODO(jacobr): enable this one after fixing violations.
+    # TODO(jacobr): enable member-ordering. This catches a bunch of real style
+    # issues but would be alot of work to migrate.
+    # - member-ordering
+    # - newline-before-return TODO(jacobr): should be in the formatter if it was a rule to adopt.
+    - no-boolean-literal-compare
+    # - no-empty-block Too many false positives. However it does flag a bunch of code smells so possibly worth re-enabling.
+    # This one seems interesting but has too many false positives. Gave it a try.
+    # - no-equal-arguments:
+    #    ignored-parameters:
+    #      - height
+    #      - width
+    #      - double-literal-format
+    #      - defaultSortColumn
+    #      - left
+    #      - right
+    #      - top
+    #      - bottom
+    #      - bottomLeft
+    #      - topLeft
+    #      - enabledBorder
+    - no-equal-then-else
+ #   - no-magic-number
+ #   - no-object-declaration Too difficult to use along with avoiding dynamic particular for JSON decoding logic.
+ #   - prefer-async-await TODO(jacobr): evaluate enabling.
+    - prefer-commenting-analyzer-ignores
+  #  - prefer-conditional-expressions Too many false positives involving large conditional expressions.
+  #  - prefer-correct-identifier-length Too many false positives with fine names like i and id.
+  #  - prefer-correct-test-file-name TODO(jacobr): enable and fix violations.
+    - prefer-correct-type-name
+  #  - prefer-enums-by-name Cannot able unless lint adds a special case for orElse
+  #  - prefer-first TODO(jacobr): enable as a follow up PR.
+  #  - prefer-immediate-return TODO(jacobr): enable as a follow up PR.
+    - prefer-iterable-of
+    - prefer-last
+    # - prefer-match-file-name
+    # TODO(jacobr): consider enabling or enabling to periodically audit.
+    # This one has a lot of false positives but is also quite nice.
+    - prefer-moving-to-variable:
+        allowed-duplicated-chains: 3
+    # - prefer-static-class
+    # - prefer-trailing-comma This one suggests trailing commas in some places they don't need to be. Reasonable but a bit verbose.
+    - tag-name
+    - always-remove-listener
+    # - avoid-border-all Micro-optimization to avoid a const constructor.
+    # - avoid-returning-widgets  This one is nice but has a lot of false positives.
+    - avoid-shrink-wrap-in-lists
+    - avoid-unnecessary-setstate
+    - avoid-expanded-as-spacer
+    - avoid-wrapping-in-padding
+    - check-for-equals-in-render-object-setters
+    - consistent-update-render-object
+    # - prefer-const-border-radius TODO(jacobr): enable.
+    - prefer-correct-edge-insets-constructor
+    # - prefer-extracting-callbacks I'm not clear this is always a good idea. Seems like a workaround.
+    # - prefer-single-widget-per-file
+    - prefer-using-list-view

packages/devtools_app/analysis_options.yaml

@@ -10,3 +10,8 @@ analyzer:
     # these files if you want to ensure code is not invalid except where
     # expected.
     - test/test_infra/test_data/syntax_highlighting/**
+
+dart_code_metrics:
+  metrics-exclude:
+    - test/**
+    - test/test_infra/test_data/**

packages/devtools_app/lib/src/app.dart

@@ -112,21 +112,21 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
     });
 
     _isDarkThemeEnabled = preferences.darkModeTheme.value;
-    preferences.darkModeTheme.addListener(() {
+    addAutoDisposeListener(preferences.darkModeTheme, () {
       setState(() {
         _isDarkThemeEnabled = preferences.darkModeTheme.value;
       });
     });
 
     _vmDeveloperModeEnabled = preferences.vmDeveloperModeEnabled.value;
-    preferences.vmDeveloperModeEnabled.addListener(() {
+    addAutoDisposeListener(preferences.vmDeveloperModeEnabled, () {
       setState(() {
         _vmDeveloperModeEnabled = preferences.vmDeveloperModeEnabled.value;
       });
     });
 
     _denseModeEnabled = preferences.denseModeEnabled.value;
-    preferences.denseModeEnabled.addListener(() {
+    addAutoDisposeListener(preferences.denseModeEnabled, () {
       setState(() {
         _denseModeEnabled = preferences.denseModeEnabled.value;
       });
@@ -191,10 +191,10 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
   }
 
   Widget _buildTabbedPage(
-    BuildContext context,
+    BuildContext _,
     String? page,
     Map<String, String?> params,
-    DevToolsNavigationState? state,
+    DevToolsNavigationState? __,
   ) {
     final vmServiceUri = params['uri'];
 
@@ -234,7 +234,8 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
                 .where((p) => !hide.contains(p.screenId))
                 .toList();
             if (screens.isEmpty) return child ?? const SizedBox.shrink();
-            return _providedControllers(
+            return MultiProvider(
+              providers: _providedControllers(),
               child: DevToolsScaffold(
                 embed: embed,
                 ideTheme: ideTheme,
@@ -277,8 +278,8 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
         return DevToolsScaffold.withChild(
           key: UniqueKey(),
           ideTheme: ideTheme,
-          child: _providedControllers(
-            offline: true,
+          child: MultiProvider(
+            providers: _providedControllers(offline: true),
             child: SnapshotScreenBody(snapshotArgs, _screens),
           ),
         );
@@ -292,7 +293,8 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
             ReportFeedbackButton(),
             OpenAboutAction(),
           ],
-          child: _providedControllers(
+          child: MultiProvider(
+            providers: _providedControllers(),
             child: const AppSizeBody(),
           ),
         );
@@ -308,18 +310,13 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
 
   List<Screen> _visibleScreens() => _screens.where(shouldShowScreen).toList();
 
-  Widget _providedControllers({required Widget child, bool offline = false}) {
-    final _providers = widget.screens
+  List<Provider> _providedControllers({bool offline = false}) {
+    return widget.screens
         .where(
           (s) => s.providesController && (offline ? s.supportsOffline : true),
         )
         .map((s) => s.controllerProvider(routerDelegate))
         .toList();
-
-    return MultiProvider(
-      providers: _providers,
-      child: child,
-    );
   }
 
   @override
@@ -456,7 +453,7 @@ class OpenSettingsAction extends StatelessWidget {
     return DevToolsTooltip(
       message: 'Settings',
       child: InkWell(
-        onTap: () async {
+        onTap: () {
           unawaited(
             showDialog(
               context: context,
@@ -505,7 +502,8 @@ class SettingsDialog extends StatelessWidget {
             CheckboxSetting(
               label: const Text('Enable analytics'),
               listenable: analyticsController.analyticsEnabled,
-              toggle: analyticsController.toggleAnalyticsEnabled,
+              toggle: (enable) =>
+                  unawaited(analyticsController.toggleAnalyticsEnabled(enable)),
               gaItem: gac.analytics,
             ),
           CheckboxSetting(
@@ -537,7 +535,7 @@ class CheckboxSetting extends StatelessWidget {
 
   final ValueListenable<bool> listenable;
 
-  final Function(bool) toggle;
+  final void Function(bool) toggle;
 
   final String gaItem;
 

packages/devtools_app/lib/src/framework/about_dialog.dart

@@ -140,7 +140,7 @@ class OpenAboutAction extends StatelessWidget {
     return DevToolsTooltip(
       message: 'About DevTools',
       child: InkWell(
-        onTap: () async {
+        onTap: () {
           unawaited(
             showDialog(
               context: context,

packages/devtools_app/lib/src/framework/app_error_handling.dart

@@ -21,7 +21,9 @@ import '../shared/config_specific/logger/logger.dart';
 /// application.
 void setupErrorHandling(Future Function() appStartCallback) {
   // First, run all our code in a new zone.
-  return runZonedGuarded(() async {
+  return runZonedGuarded(
+      // ignore: avoid-passing-async-when-sync-expected this ignore should be fixed.
+      () async {
     WidgetsFlutterBinding.ensureInitialized();
 
     final FlutterExceptionHandler? oldHandler = FlutterError.onError;

packages/devtools_app/lib/src/framework/framework_core.dart

@@ -19,8 +19,9 @@ import '../shared/primitives/utils.dart';
 import '../shared/scripts/script_manager.dart';
 import '../shared/survey.dart';
 
-typedef ErrorReporter = void Function(String title, dynamic error);
+typedef ErrorReporter = void Function(String title, Object error);
 
+// TODO(jacobr): refactor this class to not use static members.
 // ignore: avoid_classes_with_only_static_members
 class FrameworkCore {
   static void initGlobals() {

packages/devtools_app/lib/src/framework/landing_screen.dart

@@ -130,29 +130,9 @@ class _ConnectDialogState extends State<ConnectDialog>
 
   @override
   Widget build(BuildContext context) {
-    return LandingScreenSection(
-      title: 'Connect',
-      child: Column(
-        crossAxisAlignment: CrossAxisAlignment.start,
-        children: [
-          Text(
-            'Connect to a Running App',
-            style: Theme.of(context).textTheme.titleMedium,
-          ),
-          const SizedBox(height: denseRowSpacing),
-          Text(
-            'Enter a URL to a running Dart or Flutter application',
-            style: Theme.of(context).textTheme.bodySmall,
-          ),
-          const Padding(padding: EdgeInsets.only(top: 20.0)),
-          _buildConnectInput(),
-        ],
-      ),
-    );
-  }
+    final textTheme = Theme.of(context).textTheme;
 
-  Widget _buildConnectInput() {
-    return Column(
+    final connectorInput = Column(
       crossAxisAlignment: CrossAxisAlignment.stretch,
       children: [
         Row(
@@ -177,7 +157,7 @@ class _ConnectDialogState extends State<ConnectDialog>
             ),
             const SizedBox(width: defaultSpacing),
             ElevatedButton(
-              onPressed: actionInProgress ? null : _connect,
+              onPressed: actionInProgress ? null : () => unawaited(_connect()),
               child: const Text('Connect'),
             ),
           ],
@@ -192,6 +172,26 @@ class _ConnectDialogState extends State<ConnectDialog>
         ),
       ],
     );
+
+    return LandingScreenSection(
+      title: 'Connect',
+      child: Column(
+        crossAxisAlignment: CrossAxisAlignment.start,
+        children: [
+          Text(
+            'Connect to a Running App',
+            style: textTheme.titleMedium,
+          ),
+          const SizedBox(height: denseRowSpacing),
+          Text(
+            'Enter a URL to a running Dart or Flutter application',
+            style: textTheme.bodySmall,
+          ),
+          const Padding(padding: EdgeInsets.only(top: 20.0)),
+          connectorInput,
+        ],
+      ),
+    );
   }
 
   Future<void> _connect() async {
@@ -251,20 +251,21 @@ class ImportFileInstructions extends StatelessWidget {
 
   @override
   Widget build(BuildContext context) {
+    final textTheme = Theme.of(context).textTheme;
     return LandingScreenSection(
       title: 'Load DevTools Data',
       child: Column(
         crossAxisAlignment: CrossAxisAlignment.start,
         children: [
           Text(
             'Import a data file to use DevTools without an app connection.',
-            style: Theme.of(context).textTheme.titleMedium,
+            style: textTheme.titleMedium,
           ),
           const SizedBox(height: denseRowSpacing),
           Text(
             'At this time, DevTools only supports importing files that were originally'
             ' exported from DevTools.',
-            style: Theme.of(context).textTheme.bodySmall,
+            style: textTheme.bodySmall,
           ),
           const SizedBox(height: defaultSpacing),
           ElevatedButton(
@@ -300,20 +301,21 @@ class AppSizeToolingInstructions extends StatelessWidget {
 
   @override
   Widget build(BuildContext context) {
+    final textTheme = Theme.of(context).textTheme;
     return LandingScreenSection(
       title: 'App Size Tooling',
       child: Column(
         crossAxisAlignment: CrossAxisAlignment.start,
         children: [
           Text(
             'Analyze and view diffs for your app\'s size',
-            style: Theme.of(context).textTheme.titleMedium,
+            style: textTheme.titleMedium,
           ),
           const SizedBox(height: denseRowSpacing),
           Text(
             'Load Dart AOT snapshots or app size analysis files to '
             'track down size issues in your app.',
-            style: Theme.of(context).textTheme.bodySmall,
+            style: textTheme.bodySmall,
           ),
           const SizedBox(height: defaultSpacing),
           ElevatedButton(