Add json prettyprinting detail view to the log details page. (#1375)
diff --git a/packages/devtools_app/lib/src/flutter/table.dart b/packages/devtools_app/lib/src/flutter/table.dart index b7a9424..74112d2 100644 --- a/packages/devtools_app/lib/src/flutter/table.dart +++ b/packages/devtools_app/lib/src/flutter/table.dart
@@ -19,9 +19,11 @@ @required this.columns, @required this.data, @required this.keyFactory, + @required this.onItemSelected, }) : assert(columns != null), assert(keyFactory != null), assert(data != null), + assert(onItemSelected != null), super(key: key); final List<ColumnData<T>> columns; @@ -31,6 +33,8 @@ /// Factory that creates keys for each row in this table. final Key Function(T data) keyFactory; + final ItemCallback<T> onItemSelected; + @override _FlatTableState<T> createState() => _FlatTableState<T>(); } @@ -80,7 +84,7 @@ return _TableRow<T>( key: widget.keyFactory(node), node: node, - onPressed: (_) {}, + onPressed: widget.onItemSelected, columns: widget.columns, columnWidths: columnWidths, backgroundColor: _TableRow.colorFor(context, index), @@ -370,7 +374,7 @@ final T node; final List<ColumnData<T>> columns; - final ItemCallback onPressed; + final ItemCallback<T> onPressed; final List<double> columnWidths; /// Which column, if any, should show expansion affordances
diff --git a/packages/devtools_app/lib/src/logging/flutter/logging_screen.dart b/packages/devtools_app/lib/src/logging/flutter/logging_screen.dart index b9d25f9..97ea18c 100644 --- a/packages/devtools_app/lib/src/logging/flutter/logging_screen.dart +++ b/packages/devtools_app/lib/src/logging/flutter/logging_screen.dart
@@ -7,6 +7,7 @@ import '../../flutter/controllers.dart'; import '../../flutter/screen.dart'; +import '../../flutter/split.dart'; import '../../flutter/table.dart'; import '../../table_data.dart'; import '../../ui/flutter/service_extension_widgets.dart'; @@ -37,6 +38,7 @@ class _LoggingScreenState extends State<LoggingScreenBody> { LoggingController controller; + LogData selected; @override void didChangeDependencies() { @@ -68,13 +70,23 @@ ], ), Expanded( - child: LogsTable( - data: controller.data, + child: Split( + axis: Split.axisFor(context, 1.0), + firstChild: LogsTable( + data: controller.data, + onItemSelected: _select, + ), + secondChild: LogDetails(log: selected), + initialFirstFraction: 0.6, ), ), ]); } + void _select(LogData log) { + setState(() => selected = log); + } + void _clearLogs() { setState(() { controller.clear(); @@ -83,8 +95,9 @@ } class LogsTable extends StatelessWidget { - const LogsTable({Key key, this.data}) : super(key: key); + const LogsTable({Key key, this.data, this.onItemSelected}) : super(key: key); final List<LogData> data; + final ItemCallback<LogData> onItemSelected; List<ColumnData<LogData>> get columns => [ _WhenColumn(), @@ -98,6 +111,114 @@ columns: columns, data: data, keyFactory: (LogData data) => ValueKey<LogData>(data), + onItemSelected: onItemSelected, + ); + } +} + +class LogDetails extends StatefulWidget { + const LogDetails({Key key, @required this.log}) : super(key: key); + final LogData log; + + @override + _LogDetailsState createState() => _LogDetailsState(); +} + +class _LogDetailsState extends State<LogDetails> + with SingleTickerProviderStateMixin { + AnimationController crossFade; + + @override + void initState() { + super.initState(); + crossFade = AnimationController( + vsync: this, + duration: const Duration(milliseconds: 200), + )..addStatusListener((status) { + if (status == AnimationStatus.completed) { + setState(() { + _oldLog = widget.log; + crossFade.value = 0.0; + }); + } + }); + // We'll use a linear curve for this animation, so no curve needed. + _computeLogDetails(); + } + + @override + void dispose() { + crossFade.dispose(); + super.dispose(); + } + + @override + void didUpdateWidget(LogDetails oldWidget) { + super.didUpdateWidget(oldWidget); + if (widget.log != oldWidget.log) { + _oldLog = oldWidget.log; + crossFade.forward(); + } + _computeLogDetails(); + } + + Future<void> _computeLogDetails() async { + if (widget.log?.needsComputing ?? false) { + await widget.log.compute(); + setState(() {}); + } + } + + LogData _oldLog; + + bool showInspector(LogData log) => log != null && log.node != null; + bool showSimple(LogData log) => + log != null && log.node == null && !log.needsComputing; + + @override + Widget build(BuildContext context) { + return AnimatedBuilder( + animation: crossFade, + builder: (context, _) { + return Container( + color: Theme.of(context).cardColor, + child: Stack(children: [ + Opacity( + opacity: crossFade.value, + child: _buildContent(context, widget.log), + ), + Opacity( + opacity: 1 - crossFade.value, + child: _buildContent(context, _oldLog), + ), + ]), + ); + }, + ); + } + + Widget _buildContent(BuildContext context, LogData log) { + if (log == null) return const SizedBox(); + if (log.needsComputing) { + return const Center(child: CircularProgressIndicator()); + } + if (showInspector(log)) return _buildInspector(context, log); + if (showSimple(log)) return _buildSimpleLog(context, log); + return const SizedBox(); + } + + // TODO(https://github.com/flutter/devtools/issues/1370): implement this. + Widget _buildInspector(BuildContext context, LogData log) => const SizedBox(); + + Widget _buildSimpleLog(BuildContext context, LogData log) { + // TODO(https://github.com/flutter/devtools/issues/1339): Present with monospaced fonts. + return Scrollbar( + child: SingleChildScrollView( + child: Text( + log.prettyPrinted ?? '', + style: Theme.of(context).textTheme.subhead, + ), + ), ); } } @@ -127,5 +248,6 @@ /// TODO(djshuckerow): Do better than showing raw HTML here. @override - String getValue(LogData dataObject) => render(dataObject); + String getValue(LogData dataObject) => + dataObject.summary ?? dataObject.details; }
diff --git a/packages/devtools_app/lib/src/logging/logging_controller.dart b/packages/devtools_app/lib/src/logging/logging_controller.dart index e9413f4..689437e 100644 --- a/packages/devtools_app/lib/src/logging/logging_controller.dart +++ b/packages/devtools_app/lib/src/logging/logging_controller.dart
@@ -276,14 +276,16 @@ final FrameInfo frame = FrameInfo.from(e.extensionData.data); final String frameId = '#${frame.number}'; - final String frameInfo = - '<span class="pre">$frameId ${frame.elapsedMs.toStringAsFixed(1).padLeft(4)}ms </span>'; + final String frameInfoText = + '$frameId ${frame.elapsedMs.toStringAsFixed(1).padLeft(4)}ms '; + final String frameInfo = '<span class="pre">$frameInfoText</span>'; final String div = _createFrameDivHtml(frame); _log(LogData( e.extensionKind.toLowerCase(), jsonEncode(e.extensionData.data), e.timestamp, + summary: frameInfoText, summaryHtml: '$frameInfo$div', )); } else if (e.extensionKind == NavigationInfo.eventName) { @@ -387,7 +389,7 @@ final InstanceRef stackTrace = InstanceRef.parse(logRecord['stackTrace']); final String details = summary; - Future<String> detailsComputer; + Future<String> Function() detailsComputer; // If the message string was truncated by the VM, or the error object or // stackTrace objects were non-null, we need to ask the VM for more @@ -396,7 +398,7 @@ if (messageRef.valueAsStringIsTruncated == true || _isNotNull(error) || _isNotNull(stackTrace)) { - detailsComputer = Future<String>(() async { + detailsComputer = () async { // Get the full string value of the message. String result = await _retrieveFullStringValue(service, e.isolate, messageRef); @@ -436,7 +438,7 @@ } return result; - }); + }; } const int severeIssue = 1000; @@ -656,17 +658,28 @@ final RemoteDiagnosticsNode node; String _details; - Future<String> detailsComputer; + Future<String> Function() detailsComputer; + + static const JsonEncoder prettyPrinter = JsonEncoder.withIndent(' '); String get details => _details; bool get needsComputing => detailsComputer != null; Future<void> compute() async { - _details = await detailsComputer; + _details = await detailsComputer(); detailsComputer = null; } + String get prettyPrinted { + if (needsComputing) return details; + try { + return prettyPrinter.convert(jsonDecode(details)); + } catch (_) { + return details; + } + } + @override String toString() => 'LogData($kind, $timestamp)'; }
diff --git a/packages/devtools_app/pubspec.yaml b/packages/devtools_app/pubspec.yaml index 01327bc..1a68ccc 100644 --- a/packages/devtools_app/pubspec.yaml +++ b/packages/devtools_app/pubspec.yaml
@@ -112,6 +112,7 @@ - assets/img/story_of_layout/empty_space.png # See https://github.com/flutter/flutter/wiki/Desktop-shells#fonts + # TODO(https://github.com/flutter/devtools/issues/1339): Include a monospaced font. fonts: - family: Roboto fonts:
diff --git a/packages/devtools_app/test/flutter/logging_screen_test.dart b/packages/devtools_app/test/flutter/logging_screen_test.dart index 6be434b..1931138 100644 --- a/packages/devtools_app/test/flutter/logging_screen_test.dart +++ b/packages/devtools_app/test/flutter/logging_screen_test.dart
@@ -51,6 +51,8 @@ testWidgets('builds with no data', (WidgetTester tester) async { await tester.pumpWidget(wrap(Builder(builder: screen.build))); expect(find.byType(LoggingScreenBody), findsOneWidget); + expect(find.byType(LogsTable), findsOneWidget); + expect(find.byType(LogDetails), findsOneWidget); expect(find.text('Clear logs'), findsOneWidget); expect(find.byType(StructuredErrorsToggle), findsOneWidget); }); @@ -81,26 +83,140 @@ // TODO(djshuckerow): Hook up fake extension state querying. }); - testWidgets('shows most recent logs first', (WidgetTester tester) async { - when(mockLoggingController.data).thenReturn(fakeLogData); - await tester.pumpWidget(wrap(Builder(builder: screen.build))); - await tester.pumpAndSettle(); - expect(find.byType(LogsTable), findsOneWidget); - expect( - find.byKey(ValueKey(fakeLogData.last)), - findsOneWidget, - reason: 'the most recently added logdata should show in the list by ' - 'default.', - ); - expect( - find.byKey(ValueKey(fakeLogData.first)), - findsNothing, - reason: 'the least recently added logdata should be at the top of the ' - 'list, hidden from view.', - ); + group('with data', () { + setUp(() { + when(mockLoggingController.data).thenReturn(fakeLogData); + }); + + testWidgets('shows most recent logs first', (WidgetTester tester) async { + await tester.pumpWidget(wrap(Builder(builder: screen.build))); + await tester.pumpAndSettle(); + expect(find.byType(LogsTable), findsOneWidget); + expect( + find.byKey(ValueKey(fakeLogData.last)), + findsOneWidget, + reason: 'the most recently added logdata should show in the list by ' + 'default.', + ); + expect( + find.byKey(ValueKey(fakeLogData.first)), + findsNothing, + reason: + 'the least recently added logdata should be at the top of the ' + 'list, hidden from view.', + ); + }); + + testWidgets('can show non-computing log data', + (WidgetTester tester) async { + await tester.pumpWidget(wrap(Builder(builder: screen.build))); + await tester.pumpAndSettle(); + await tester.tap(find.byKey(ValueKey(fakeLogData[996]))); + await tester.pumpAndSettle(); + expect( + find.text('log event 996'), + findsNWidgets(3), + reason: 'The log details should be visible both in the table and ' + 'the details section. The details view will have two text ' + 'widgets to support its cross-fade animation.', + ); + }); + + testWidgets('can show null log data', (WidgetTester tester) async { + await tester.pumpWidget(wrap(Builder(builder: screen.build))); + await tester.pumpAndSettle(); + await tester.tap(find.byKey(ValueKey(fakeLogData[997]))); + await tester.pumpAndSettle(); + }); + + testWidgets('can compute details of non-json log data', + (WidgetTester tester) async { + const index = 998; + final log = fakeLogData[index]; + + await tester.pumpWidget(wrap(Builder(builder: screen.build))); + await tester.pumpAndSettle(); + await tester.tap(find.byKey(ValueKey(log))); + await tester.pump(); + expect( + find.text(nonJsonOutput), + findsNothing, + reason: + "The details of the log haven't computed yet, so they shouldn't " + 'be available.', + ); + + await tester.pumpAndSettle(); + expect( + find.text(nonJsonOutput), + findsNWidgets(2), + reason: + 'The fade transition between details views will have two text widgets.', + ); + }); + + testWidgets('can show details of json log data', + (WidgetTester tester) async { + const index = 999; + bool containsJson(Widget widget) { + if (widget is! Text) return false; + final text = widget as Text; + return text.data.contains('{') && text.data.contains('}'); + } + + final findJson = find.descendant( + of: find.byType(LogDetails), + matching: find.byWidgetPredicate(containsJson), + ); + + await tester.pumpWidget(wrap(Builder(builder: screen.build))); + await tester.pumpAndSettle(); + await tester.tap(find.byKey(ValueKey(fakeLogData[index]))); + await tester.pump(); + + expect( + findJson, + findsNothing, + reason: + "The details of the log haven't computed yet, so they shouldn't be available.", + ); + + await tester.pumpAndSettle(); + expect( + findJson, + findsNWidgets(2), + reason: + 'The fade transition between details views will have two text widgets.', + ); + }); }); }); } -final fakeLogData = - List<LogData>.generate(1000, (i) => LogData('kind $i', 'log event $i', i)); +const totalLogs = 1000; +final fakeLogData = List<LogData>.generate(totalLogs, _generate); + +LogData _generate(int i) { + String details = 'log event $i'; + String computedDetails; + switch (i) { + case 999: + computedDetails = jsonOutput; + break; + case 998: + computedDetails = nonJsonOutput; + break; + case 997: + details = null; + break; + default: + break; + } + final detailsComputer = computedDetails == null + ? null + : () => Future.delayed(const Duration(seconds: 1), () => computedDetails); + return LogData('kind $i', details, i, detailsComputer: detailsComputer); +} + +const nonJsonOutput = 'Non-json details for log number 998'; +const jsonOutput = '{\n"Details": "of log event 999",\n"logEvent": "999"\n}\n';
diff --git a/packages/devtools_app/test/flutter/table_test.dart b/packages/devtools_app/test/flutter/table_test.dart index 2a507a8..46dbf27 100644 --- a/packages/devtools_app/test/flutter/table_test.dart +++ b/packages/devtools_app/test/flutter/table_test.dart
@@ -29,6 +29,7 @@ columns: [_FlatNameColumn()], data: [TestData('empty', 0)], keyFactory: (d) => Key(d.name), + onItemSelected: noop, ); await tester.pumpWidget(wrap(table)); expect(find.byWidget(table), findsOneWidget); @@ -43,6 +44,7 @@ _NumberColumn(), ], data: flatData, + onItemSelected: noop, keyFactory: (d) => Key(d.name), ); await tester.pumpWidget(wrap(table)); @@ -68,6 +70,7 @@ _CombinedColumn(), ], data: flatData, + onItemSelected: noop, keyFactory: (data) => Key(data.name), ); await tester.pumpWidget(wrap( @@ -80,6 +83,24 @@ expect(find.byWidget(table), findsOneWidget); }); + testWidgets('can select an item', (WidgetTester tester) async { + TestData selected; + final testData = TestData('empty', 0); + const key = Key('empty'); + final table = FlatTable<TestData>( + columns: [_FlatNameColumn()], + data: [testData], + keyFactory: (d) => Key(d.name), + onItemSelected: (item) => selected = item, + ); + await tester.pumpWidget(wrap(table)); + expect(find.byWidget(table), findsOneWidget); + expect(find.byKey(key), findsOneWidget); + expect(selected, isNull); + await tester.tap(find.byKey(key)); + expect(selected, testData); + }); + test('fails with no data', () { expect( () { @@ -87,6 +108,7 @@ columns: [_FlatNameColumn()], data: null, keyFactory: (d) => Key(d.name), + onItemSelected: noop, ); }, throwsAssertionError, @@ -99,6 +121,7 @@ columns: [_FlatNameColumn()], data: flatData, keyFactory: null, + onItemSelected: noop, ); }, throwsAssertionError); }); @@ -300,3 +323,5 @@ @override double get fixedWidthPx => 400.0; } + +void noop(TestData data) {}
diff --git a/packages/devtools_testing/lib/logging_controller_test.dart b/packages/devtools_testing/lib/logging_controller_test.dart index c4eaf59..11f6643 100644 --- a/packages/devtools_testing/lib/logging_controller_test.dart +++ b/packages/devtools_testing/lib/logging_controller_test.dart
@@ -283,6 +283,25 @@ await env.tearDownEnvironment(); }); }, timeout: const Timeout.factor(8)); + + group('LogData', () { + test( + 'pretty prints when details are json, and returns its details otherwise.', + () { + final nonJson = LogData('some kind', 'Not json', 0); + final json = LogData( + 'some kind', '{"firstValue": "value", "otherValue": "value2"}', 1); + final nullDetails = LogData('some kind', null, 1); + const prettyJson = '{\n' + ' "firstValue": "value",\n' + ' "otherValue": "value2"\n' + '}'; + + expect(json.prettyPrinted, prettyJson); + expect(nonJson.prettyPrinted, 'Not json'); + expect(nullDetails.prettyPrinted, null); + }); + }); } /// Normalize text in error messages that is likely unstable.