From ffd4451bf9ff4a0dc78a08e04ab4eb0cfd66b523 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Tue, 12 May 2020 14:11:33 -0700 Subject: [PATCH 1/2] Add descriptions to Timeline configurations --- .../lib/src/debugger/flutter/call_stack.dart | 2 +- .../lib/src/debugger/flutter/common.dart | 8 -- .../lib/src/debugger/flutter/variables.dart | 2 +- .../lib/src/flutter/common_widgets.dart | 13 +++ .../timeline/flutter/timeline_controller.dart | 55 ++++----- .../src/timeline/flutter/timeline_screen.dart | 106 ++++++++++++------ .../timeline/flutter/timeline_streams.dart | 92 +++++++++++++++ 7 files changed, 208 insertions(+), 70 deletions(-) create mode 100644 packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart diff --git a/packages/devtools_app/lib/src/debugger/flutter/call_stack.dart b/packages/devtools_app/lib/src/debugger/flutter/call_stack.dart index 0803fe230b2..bcef76c800a 100644 --- a/packages/devtools_app/lib/src/debugger/flutter/call_stack.dart +++ b/packages/devtools_app/lib/src/debugger/flutter/call_stack.dart @@ -6,8 +6,8 @@ import 'package:flutter/material.dart' hide Stack; import 'package:provider/provider.dart'; import 'package:vm_service/vm_service.dart'; +import '../../flutter/common_widgets.dart'; import '../../flutter/theme.dart'; -import 'common.dart'; import 'debugger_controller.dart'; import 'debugger_model.dart'; diff --git a/packages/devtools_app/lib/src/debugger/flutter/common.dart b/packages/devtools_app/lib/src/debugger/flutter/common.dart index 6501bca4545..c42ca78d126 100644 --- a/packages/devtools_app/lib/src/debugger/flutter/common.dart +++ b/packages/devtools_app/lib/src/debugger/flutter/common.dart @@ -45,11 +45,3 @@ Widget createAnimatedCircleWidget(double radius, Color color) { decoration: BoxDecoration(color: color, shape: BoxShape.circle), ); } - -extension DebuggerTextStyleExtension on ThemeData { - TextStyle get regularTextStyle => TextStyle(color: textTheme.bodyText2.color); - - TextStyle get subtleTextStyle => TextStyle(color: unselectedWidgetColor); - - TextStyle get selectedTextStyle => TextStyle(color: textSelectionColor); -} diff --git a/packages/devtools_app/lib/src/debugger/flutter/variables.dart b/packages/devtools_app/lib/src/debugger/flutter/variables.dart index d15852fb9c6..70f6e9fd87d 100644 --- a/packages/devtools_app/lib/src/debugger/flutter/variables.dart +++ b/packages/devtools_app/lib/src/debugger/flutter/variables.dart @@ -5,8 +5,8 @@ import 'package:flutter/material.dart' hide Stack; import 'package:provider/provider.dart'; +import '../../flutter/common_widgets.dart'; import '../../flutter/tree.dart'; -import 'common.dart'; import 'debugger_controller.dart'; import 'debugger_model.dart'; diff --git a/packages/devtools_app/lib/src/flutter/common_widgets.dart b/packages/devtools_app/lib/src/flutter/common_widgets.dart index 9bf295650ac..137601b7885 100644 --- a/packages/devtools_app/lib/src/flutter/common_widgets.dart +++ b/packages/devtools_app/lib/src/flutter/common_widgets.dart @@ -29,6 +29,13 @@ List headerInColumn(TextTheme textTheme, String title) { ]; } +List subHeaderInColumn(TextTheme textTheme, String title) { + return [ + Text(title, style: textTheme.subtitle2), + const PaddedDivider(padding: EdgeInsets.only(bottom: denseRowSpacing)), + ]; +} + /// Convenience [Divider] with [Padding] that provides a good divider in forms. class PaddedDivider extends StatelessWidget { const PaddedDivider({ @@ -672,4 +679,10 @@ extension ColorExtension on Color { extension ThemeDataExtension on ThemeData { /// Returns whether we are currently using a dark theme. bool get isDarkTheme => brightness == Brightness.dark; + + TextStyle get regularTextStyle => TextStyle(color: textTheme.bodyText2.color); + + TextStyle get subtleTextStyle => TextStyle(color: unselectedWidgetColor); + + TextStyle get selectedTextStyle => TextStyle(color: textSelectionColor); } diff --git a/packages/devtools_app/lib/src/timeline/flutter/timeline_controller.dart b/packages/devtools_app/lib/src/timeline/flutter/timeline_controller.dart index df3be1163e8..52fb0686433 100644 --- a/packages/devtools_app/lib/src/timeline/flutter/timeline_controller.dart +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_controller.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'dart:async'; +import 'package:flutter/foundation.dart'; import 'package:pedantic/pedantic.dart'; import 'package:vm_service/vm_service.dart' as vm_service; @@ -18,10 +19,10 @@ import '../../profiler/cpu_profile_transformer.dart'; import '../../profiler/profile_granularity.dart'; import '../../service_manager.dart'; import '../../trace_event.dart'; -import '../../ui/fake_flutter/fake_flutter.dart'; import 'timeline_model.dart'; import 'timeline_processor.dart'; import 'timeline_screen.dart'; +import 'timeline_streams.dart'; /// This class contains the business logic for [timeline_screen.dart]. /// @@ -108,18 +109,17 @@ class TimelineController // TODO(kenz): switch to use VmFlagManager-like pattern once // https://github.com/dart-lang/sdk/issues/41822 is fixed. /// Recorded timeline stream values. - Map> get recordedStreams => _recordedStreams; - final _recordedStreams = { - 'Dart': ValueNotifier(false), - 'Embedder': ValueNotifier(false), - 'GC': ValueNotifier(false), - 'API': ValueNotifier(false), - 'Compiler': ValueNotifier(false), - 'CompilerVerbose': ValueNotifier(false), - 'Debugger': ValueNotifier(false), - 'Isolate': ValueNotifier(false), - 'VM': ValueNotifier(false), - }; + final recordedStreams = [ + dartTimelineStream, + embedderTimelineStream, + gcTimelineStream, + apiTimelineStream, + compilerTimelineStream, + compilerVerboseTimelineStream, + debuggerTimelineStream, + isolateTimelineStream, + vmTimelineStream, + ]; /// Active timeline data. /// @@ -154,7 +154,11 @@ class TimelineController _cpuProfilerService.setProfilePeriod(mediumProfilePeriod), logError: false, )); - await setTimelineStreams(['GC', 'Dart', 'Embedder']); + await setTimelineStreams([ + dartTimelineStream, + embedderTimelineStream, + gcTimelineStream, + ]); await toggleHttpRequestLogging(true); } @@ -428,27 +432,28 @@ class TimelineController _httpTimelineLoggingEnabledNotifier.value = state; } - Future setTimelineStreams(List streams) async { + Future setTimelineStreams(List streams) async { for (final stream in streams) { - assert(_recordedStreams.containsKey(stream)); - _recordedStreams[stream].value = true; + assert(recordedStreams.contains(stream)); + stream.toggle(true); } - await serviceManager.service.setVMTimelineFlags(streams); + await serviceManager.service + .setVMTimelineFlags(streams.map((s) => s.name).toList()); } // TODO(kenz): this is not as robust as we'd like. Revisit once // https://github.com/dart-lang/sdk/issues/41822 is addressed. - Future toggleTimelineStream(String name, [bool value]) async { - value ??= !_recordedStreams[name].value; + Future toggleTimelineStream(RecordedTimelineStream stream) async { + final newValue = !stream.enabled.value; final timelineFlags = (await serviceManager.service.getVMTimelineFlags()).recordedStreams; - if (timelineFlags.contains(name) && !value) { - timelineFlags.remove(name); - } else if (!timelineFlags.contains(name) && value) { - timelineFlags.add(name); + if (timelineFlags.contains(stream.name) && !newValue) { + timelineFlags.remove(stream.name); + } else if (!timelineFlags.contains(stream.name) && newValue) { + timelineFlags.add(stream.name); } await serviceManager.service.setVMTimelineFlags(timelineFlags); - _recordedStreams[name].value = value; + stream.toggle(newValue); } /// Clears the timeline data currently stored by the controller. diff --git a/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart b/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart index 27be7a9493f..51273ae0f5e 100644 --- a/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart @@ -4,6 +4,7 @@ import 'dart:async'; +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:provider/provider.dart'; @@ -331,12 +332,13 @@ class TimelineScreenBodyState extends State class TimelineConfigurationsDialog extends StatelessWidget { const TimelineConfigurationsDialog(this.controller); - static const dialogWidth = 300.0; + static const dialogWidth = 500.0; final TimelineController controller; @override Widget build(BuildContext context) { + final theme = Theme.of(context); return Dialog( shape: RoundedRectangleBorder( borderRadius: BorderRadius.circular(defaultDialogRadius)), @@ -348,59 +350,93 @@ class TimelineConfigurationsDialog extends StatelessWidget { mainAxisAlignment: MainAxisAlignment.start, crossAxisAlignment: CrossAxisAlignment.start, children: [ - ...headerInColumn(Theme.of(context).textTheme, 'Recorded Streams'), - ..._recordedStreams(), + ...headerInColumn(theme.textTheme, 'Recorded Streams'), + ..._defaultRecordedStreams(theme), + const SizedBox(height: denseSpacing), + ...subHeaderInColumn(theme.textTheme, 'Advanced'), + ..._advancedStreams(theme), ], ), ), ); } - List _recordedStreams() { + List _defaultRecordedStreams(ThemeData theme) { + return [ + ..._timelineStreams(theme, advanced: false), + // Special case "Network Traffic" because it is not implemented as a + // Timeline recorded stream in the VM. The user does not need to be aware of + // the distinction, however. + _buildStream( + name: 'Network', + description: ' • Log Http traffic', + listenable: controller.httpTimelineLoggingEnabled, + onChanged: controller.toggleHttpRequestLogging, + theme: theme, + ), + ]; + } + + List _advancedStreams(ThemeData theme) { + return _timelineStreams(theme, advanced: true); + } + + List _timelineStreams( + ThemeData theme, { + @required bool advanced, + }) { final settings = []; - for (final streamName in controller.recordedStreams.keys) { - // TODO(kenz): add subtly styles description for each stream. - final title = Text(streamName); - final checkbox = ValueListenableBuilder( - valueListenable: controller.recordedStreams[streamName], - builder: (context, enabled, _) { - return Checkbox( - value: enabled, - onChanged: (_) => controller.toggleTimelineStream(streamName), - ); - }, - ); - settings.add( - Row( - mainAxisSize: MainAxisSize.min, - mainAxisAlignment: MainAxisAlignment.start, - children: [ - checkbox, - title, - ], - ), - ); + final streams = controller.recordedStreams + .where((s) => s.advanced == advanced) + .toList(); + for (final stream in streams) { + settings.add(_buildStream( + name: stream.name, + description: ' • ${stream.description}', + listenable: stream.enabled, + onChanged: (_) => controller.toggleTimelineStream(stream), + theme: theme, + )); } + return settings; + } - // Special case "Network Traffic" because it is not implemented as a - // Timeline recorded stream in the VM. The user does not need to be aware of - // the distinction, however. - settings.add(Row( + Widget _buildStream({ + @required String name, + @required String description, + @required ValueListenable listenable, + @required void Function(bool) onChanged, + @required ThemeData theme, + }) { + return Row( mainAxisSize: MainAxisSize.min, mainAxisAlignment: MainAxisAlignment.start, children: [ ValueListenableBuilder( - valueListenable: controller.httpTimelineLoggingEnabled, + valueListenable: listenable, builder: (context, value, _) { return Checkbox( value: value, - onChanged: controller.toggleHttpRequestLogging, + onChanged: onChanged, ); }, ), - const Text('Network Traffic'), + Flexible( + child: RichText( + overflow: TextOverflow.visible, + text: TextSpan( + text: name, + style: theme.regularTextStyle, + children: [ + TextSpan( + text: description, + style: theme.subtleTextStyle, + ), + ], + ), + ), + ), ], - )); - return settings; + ); } } diff --git a/packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart b/packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart new file mode 100644 index 00000000000..65c77b8ed6a --- /dev/null +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart @@ -0,0 +1,92 @@ +// Copyright 2020 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 'package:flutter/foundation.dart'; + +class RecordedTimelineStream { + RecordedTimelineStream({ + @required this.name, + @required this.description, + this.advanced = false, + }); + + final String name; + + final String description; + + final bool advanced; + + ValueListenable get enabled => _enabled; + + final _enabled = ValueNotifier(false); + + void toggle(bool value) { + _enabled.value = value; + } + + @override + bool operator ==(other) { + return name == other.name; + } + + @override + int get hashCode => name.hashCode; +} + +final dartTimelineStream = RecordedTimelineStream( + name: 'Dart', + description: + 'Events emitted from dart:developer Timeline or Timeline Task, including' + ' Flutter framework events', +); + +final embedderTimelineStream = RecordedTimelineStream( + name: 'Embedder', + description: + 'Events emitted from Dart._TimelineEvent, often emitted from the Flutter' + ' Engine', +); + +final gcTimelineStream = RecordedTimelineStream( + name: 'GC', + description: 'Garbage collection', +); + +final apiTimelineStream = RecordedTimelineStream( + name: 'API', + description: 'Calls to the VM\'s embedding API (e.g. Dart._XYZ)', + advanced: true, +); + +final compilerTimelineStream = RecordedTimelineStream( + name: 'Compiler', + description: + 'Compiler phases (loading code, compilation, optimization, etc.)', + advanced: true, +); + +final compilerVerboseTimelineStream = RecordedTimelineStream( + name: 'CompilerVerbose', + description: 'More detailed compiler phases', + advanced: true, +); + +final debuggerTimelineStream = RecordedTimelineStream( + name: 'Debugger', + description: 'Debugger paused events', + advanced: true, +); + +final isolateTimelineStream = RecordedTimelineStream( + name: 'Isolate', + description: + 'Isolate-scoped events (startup, shutdown, snapshot loading, etc.)', + advanced: true, +); + +final vmTimelineStream = RecordedTimelineStream( + name: 'VM', + description: 'Dart VM events (startup, shutdown, snapshot loading, etc.)', + advanced: true, +); From 4bb98cb8219f53e17301154cd9eebf28529cba08 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 13 May 2020 08:32:10 -0700 Subject: [PATCH 2/2] review comments --- .../lib/src/timeline/flutter/timeline_screen.dart | 4 ++-- .../lib/src/timeline/flutter/timeline_streams.dart | 12 +++++------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart b/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart index 51273ae0f5e..82d2b33a105 100644 --- a/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart @@ -332,7 +332,7 @@ class TimelineScreenBodyState extends State class TimelineConfigurationsDialog extends StatelessWidget { const TimelineConfigurationsDialog(this.controller); - static const dialogWidth = 500.0; + static const dialogWidth = 700.0; final TimelineController controller; @@ -369,7 +369,7 @@ class TimelineConfigurationsDialog extends StatelessWidget { // the distinction, however. _buildStream( name: 'Network', - description: ' • Log Http traffic', + description: ' • Http traffic', listenable: controller.httpTimelineLoggingEnabled, onChanged: controller.toggleHttpRequestLogging, theme: theme, diff --git a/packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart b/packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart index 65c77b8ed6a..7e4fad0e050 100644 --- a/packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_streams.dart @@ -37,15 +37,14 @@ class RecordedTimelineStream { final dartTimelineStream = RecordedTimelineStream( name: 'Dart', description: - 'Events emitted from dart:developer Timeline or Timeline Task, including' - ' Flutter framework events', + 'Events emitted from dart:developer Timeline APIs (including Flutter ' + 'framework events)', ); final embedderTimelineStream = RecordedTimelineStream( name: 'Embedder', description: - 'Events emitted from Dart._TimelineEvent, often emitted from the Flutter' - ' Engine', + 'Additional platform events (often emitted from the Flutter engine)', ); final gcTimelineStream = RecordedTimelineStream( @@ -55,7 +54,7 @@ final gcTimelineStream = RecordedTimelineStream( final apiTimelineStream = RecordedTimelineStream( name: 'API', - description: 'Calls to the VM\'s embedding API (e.g. Dart._XYZ)', + description: 'Calls to the VM embedding API', advanced: true, ); @@ -80,8 +79,7 @@ final debuggerTimelineStream = RecordedTimelineStream( final isolateTimelineStream = RecordedTimelineStream( name: 'Isolate', - description: - 'Isolate-scoped events (startup, shutdown, snapshot loading, etc.)', + description: 'Isolate events (startup, shutdown, snapshot loading, etc.)', advanced: true, );