Implement new selection UI for Timeline page (#2067)
* Paint brackets around selected timeline frame events.
diff --git a/packages/devtools_app/lib/src/charts/flame_chart.dart b/packages/devtools_app/lib/src/charts/flame_chart.dart
index 96bc62e..2c2c3f5 100644
--- a/packages/devtools_app/lib/src/charts/flame_chart.dart
+++ b/packages/devtools_app/lib/src/charts/flame_chart.dart
@@ -769,7 +769,6 @@
onSelected = ((_) {}),
selectable = false;
- static const _selectedNodeColor = lightSelection;
static const _selectedTextColor = Colors.black;
// We would like this value to be smaller, but zoom performance does not allow
// for that. We should decrease this value if we can improve flame chart zoom
@@ -840,7 +839,7 @@
}
Color _backgroundColor(bool selected) {
- if (selected) return _selectedNodeColor;
+ if (selected) return timelineSelectionColor;
return backgroundColor;
}
@@ -965,3 +964,31 @@
return index;
}
}
+
+abstract class FlameChartPainter extends CustomPainter {
+ FlameChartPainter({
+ @required this.zoom,
+ @required this.constraints,
+ @required this.verticalScrollOffset,
+ @required this.horizontalScrollOffset,
+ @required this.chartStartInset,
+ }) : visible = Rect.fromLTWH(
+ horizontalScrollOffset,
+ verticalScrollOffset,
+ constraints.maxWidth,
+ constraints.maxHeight,
+ );
+
+ final double zoom;
+
+ final BoxConstraints constraints;
+
+ final double verticalScrollOffset;
+
+ final double horizontalScrollOffset;
+
+ final double chartStartInset;
+
+ /// The absolute coordinates of the flame chart's visible section.
+ final Rect visible;
+}
diff --git a/packages/devtools_app/lib/src/timeline/flutter_frames_chart.dart b/packages/devtools_app/lib/src/timeline/flutter_frames_chart.dart
index d8b6912..c2329e9 100644
--- a/packages/devtools_app/lib/src/timeline/flutter_frames_chart.dart
+++ b/packages/devtools_app/lib/src/timeline/flutter_frames_chart.dart
@@ -213,6 +213,11 @@
static const defaultFrameWidth = 32.0;
+ static const selectedIndicatorHeight = 8.0;
+
+ static const selectedFrameIndicatorKey =
+ Key('flutter frames chart - selected frame indicator');
+
final TimelineFrame frame;
final bool selected;
@@ -243,32 +248,42 @@
(frame.rasterDurationMs / msPerPx).clamp(0.0, availableChartHeight),
color: janky ? rasterJankColor : mainRasterColor,
);
- return Container(
- padding: const EdgeInsets.symmetric(horizontal: densePadding),
- color: selected ? selectedFrameBackgroundColor : null,
- child: Column(
- children: [
- // Dummy child so that the InkWell does not take up the entire column.
- const Expanded(child: SizedBox()),
- InkWell(
- // TODO(kenz): make tooltip to persist if the frame is selected.
- // TODO(kenz): change color on hover.
- onTap: onSelected,
- child: Tooltip(
- message: _tooltipText(frame),
- padding: const EdgeInsets.all(denseSpacing),
- preferBelow: false,
- child: Row(
- crossAxisAlignment: CrossAxisAlignment.end,
- children: [
- ui,
- raster,
- ],
+ return Stack(
+ children: [
+ Container(
+ padding: const EdgeInsets.symmetric(horizontal: densePadding),
+ color: selected ? selectedFrameBackgroundColor : null,
+ child: Column(
+ children: [
+ // Dummy child so that the InkWell does not take up the entire column.
+ const Expanded(child: SizedBox()),
+ InkWell(
+ // TODO(kenz): make tooltip to persist if the frame is selected.
+ // TODO(kenz): change color on hover.
+ onTap: onSelected,
+ child: Tooltip(
+ message: _tooltipText(frame),
+ padding: const EdgeInsets.all(denseSpacing),
+ preferBelow: false,
+ child: Row(
+ crossAxisAlignment: CrossAxisAlignment.end,
+ children: [
+ ui,
+ raster,
+ ],
+ ),
+ ),
),
- ),
+ ],
),
- ],
- ),
+ ),
+ if (selected)
+ Container(
+ key: selectedFrameIndicatorKey,
+ color: timelineSelectionColor,
+ height: selectedIndicatorHeight,
+ ),
+ ],
);
}
diff --git a/packages/devtools_app/lib/src/timeline/timeline_flame_chart.dart b/packages/devtools_app/lib/src/timeline/timeline_flame_chart.dart
index c550816..fe3ea22 100644
--- a/packages/devtools_app/lib/src/timeline/timeline_flame_chart.dart
+++ b/packages/devtools_app/lib/src/timeline/timeline_flame_chart.dart
@@ -103,7 +103,9 @@
final verticalScrollOffset =
eventGroupStartXValues[widget.data.eventGroups[TimelineData.uiKey]];
await verticalScrollController.animateTo(
- verticalScrollOffset,
+ // Subtract [rowHeightWithPadding] to ensure the selection brackets fit
+ // in view.
+ verticalScrollOffset - rowHeightWithPadding,
duration: shortDuration,
curve: defaultCurve,
);
@@ -272,10 +274,11 @@
@override
List<CustomPaint> buildCustomPaints(BoxConstraints constraints) {
+ final zoom = zoomController.value;
return [
CustomPaint(
painter: AsyncGuidelinePainter(
- zoom: zoomController.value,
+ zoom: zoom,
constraints: constraints,
verticalScrollOffset: verticalScrollOffset,
horizontalScrollOffset: horizontalScrollOffset,
@@ -286,7 +289,7 @@
),
CustomPaint(
painter: TimelineGridPainter(
- zoom: zoomController.value,
+ zoom: zoom,
constraints: constraints,
verticalScrollOffset: verticalScrollOffset,
horizontalScrollOffset: horizontalScrollOffset,
@@ -296,6 +299,23 @@
duration: widget.time.duration,
),
),
+ CustomPaint(
+ painter: SelectedFrameBracketPainter(
+ _selectedFrame,
+ zoom: zoom,
+ constraints: constraints,
+ verticalScrollOffset: verticalScrollOffset,
+ horizontalScrollOffset: horizontalScrollOffset,
+ chartStartInset: widget.startInset,
+ startTimeOffsetMicros: startTimeOffset,
+ startingPxPerMicro: startingPxPerMicro,
+ // Subtract [rowHeight] because [_calculateVerticalGuidelineStartY]
+ // returns the Y value at the bottom of the flame chart node, and we
+ // want the Y value at the top of the node.
+ yForEvent: (event) =>
+ _calculateVerticalGuidelineStartY(event) - rowHeight,
+ ),
+ ),
];
}
@@ -432,51 +452,40 @@
}
}
-class AsyncGuidelinePainter extends CustomPainter {
+class AsyncGuidelinePainter extends FlameChartPainter {
AsyncGuidelinePainter({
- @required this.zoom,
- @required this.constraints,
- @required this.verticalScrollOffset,
- @required this.horizontalScrollOffset,
+ @required double zoom,
+ @required BoxConstraints constraints,
+ @required double verticalScrollOffset,
+ @required double horizontalScrollOffset,
+ @required double chartStartInset,
@required this.verticalGuidelines,
@required this.horizontalGuidelines,
- @required this.chartStartInset,
- });
-
- final double zoom;
-
- final BoxConstraints constraints;
-
- final double verticalScrollOffset;
-
- final double horizontalScrollOffset;
+ }) : super(
+ zoom: zoom,
+ constraints: constraints,
+ verticalScrollOffset: verticalScrollOffset,
+ horizontalScrollOffset: horizontalScrollOffset,
+ chartStartInset: chartStartInset,
+ );
final List<VerticalLineSegment> verticalGuidelines;
final List<HorizontalLineSegment> horizontalGuidelines;
- final double chartStartInset;
-
@override
void paint(Canvas canvas, Size size) {
- final visible = Rect.fromLTWH(
- horizontalScrollOffset,
- verticalScrollOffset,
- constraints.maxWidth,
- constraints.maxHeight,
- );
-
// The guideline objects are calculated with a base zoom level of 1.0. We
// need to convert the zoomed left offset into the unzoomed left offset for
// proper calculation of the first vertical guideline index.
final unzoomedOffset = math.max(0.0, visible.left - chartStartInset) / zoom;
- final leftBoundWithZoom = chartStartInset + unzoomedOffset;
+ final leftBoundWithoutZoom = chartStartInset + unzoomedOffset;
final firstVerticalGuidelineIndex = lowerBound(
verticalGuidelines,
VerticalLineSegment(
- Offset(leftBoundWithZoom, visible.top),
- Offset(leftBoundWithZoom, visible.bottom),
+ Offset(leftBoundWithoutZoom, visible.top),
+ Offset(leftBoundWithoutZoom, visible.bottom),
),
);
final firstHorizontalGuidelineIndex = lowerBound(
@@ -567,17 +576,23 @@
bool shouldRepaint(AsyncGuidelinePainter oldDelegate) => true;
}
-class TimelineGridPainter extends CustomPainter {
+class TimelineGridPainter extends FlameChartPainter {
TimelineGridPainter({
- @required this.zoom,
- @required this.constraints,
- @required this.verticalScrollOffset,
- @required this.horizontalScrollOffset,
- @required this.chartStartInset,
+ @required double zoom,
+ @required BoxConstraints constraints,
+ @required double verticalScrollOffset,
+ @required double horizontalScrollOffset,
+ @required double chartStartInset,
@required this.chartEndInset,
@required this.flameChartWidth,
@required this.duration,
- });
+ }) : super(
+ zoom: zoom,
+ constraints: constraints,
+ verticalScrollOffset: verticalScrollOffset,
+ horizontalScrollOffset: horizontalScrollOffset,
+ chartStartInset: chartStartInset,
+ );
static const baseGridIntervalPx = 150.0;
static const timestampOffset = 6.0;
@@ -586,18 +601,6 @@
Color(0xFFFAFBFC),
);
- static const origin = 0.0;
-
- final double zoom;
-
- final BoxConstraints constraints;
-
- final double verticalScrollOffset;
-
- final double horizontalScrollOffset;
-
- final double chartStartInset;
-
final double chartEndInset;
final double flameChartWidth;
@@ -606,20 +609,12 @@
@override
void paint(Canvas canvas, Size size) {
- // The absolute coordinates of the flame chart's visible section.
- final visible = Rect.fromLTWH(
- horizontalScrollOffset,
- verticalScrollOffset,
- constraints.maxWidth,
- constraints.maxHeight,
- );
-
// Paint background for the section that will contain the timestamps. This
// section will appear sticky to the top of the viewport.
canvas.drawRect(
Rect.fromLTWH(
- origin,
- origin,
+ 0.0,
+ 0.0,
constraints.maxWidth,
math.min(constraints.maxHeight, rowHeight),
),
@@ -671,13 +666,13 @@
// of text widgets for the timestamps instead of painting them.
final xOffset = lineX - textPainter.width - timestampOffset;
if (xOffset > 0) {
- textPainter.paint(canvas, Offset(xOffset, origin + 5.0));
+ textPainter.paint(canvas, Offset(xOffset, 5.0));
}
}
void _paintGridLine(Canvas canvas, double lineX) {
canvas.drawLine(
- Offset(lineX, origin),
+ Offset(lineX, 0.0),
Offset(lineX, constraints.maxHeight),
Paint()..color = chartAccentColor,
);
@@ -734,6 +729,174 @@
);
}
+class SelectedFrameBracketPainter extends FlameChartPainter {
+ SelectedFrameBracketPainter(
+ this.selectedFrame, {
+ @required double zoom,
+ @required BoxConstraints constraints,
+ @required double verticalScrollOffset,
+ @required double horizontalScrollOffset,
+ @required double chartStartInset,
+ @required this.startTimeOffsetMicros,
+ @required this.startingPxPerMicro,
+ @required this.yForEvent,
+ }) : super(
+ zoom: zoom,
+ constraints: constraints,
+ verticalScrollOffset: verticalScrollOffset,
+ horizontalScrollOffset: horizontalScrollOffset,
+ chartStartInset: chartStartInset,
+ );
+
+ static const strokeWidth = 4.0;
+ static const bracketWidth = 24.0;
+ static const bracketCurveWidth = 8.0;
+ static const bracketVerticalPadding = 8.0;
+
+ final TimelineFrame selectedFrame;
+
+ final int startTimeOffsetMicros;
+
+ final double startingPxPerMicro;
+
+ final double Function(TimelineEvent) yForEvent;
+
+ @override
+ void paint(Canvas canvas, Size size) {
+ if (selectedFrame == null) return;
+
+ canvas.clipRect(Rect.fromLTWH(
+ 0.0,
+ rowHeight, // We do not want to paint inside the timestamp section.
+ constraints.maxWidth,
+ constraints.maxHeight - rowHeight,
+ ));
+
+ final paint = Paint()
+ ..color = timelineSelectionColor
+ ..style = PaintingStyle.stroke
+ ..strokeWidth = strokeWidth;
+
+ _paintBrackets(canvas, paint, event: selectedFrame.uiEventFlow);
+ _paintBrackets(canvas, paint, event: selectedFrame.rasterEventFlow);
+ }
+
+ void _paintBrackets(
+ Canvas canvas,
+ Paint paint, {
+ @required TimelineEvent event,
+ }) {
+ final startMicros = event.time.start.inMicroseconds - startTimeOffsetMicros;
+ final endMicros = event.time.end.inMicroseconds - startTimeOffsetMicros;
+ final startPx = startMicros * startingPxPerMicro * zoom + chartStartInset;
+ final endPx = endMicros * startingPxPerMicro * zoom + chartStartInset;
+
+ final startBracketX =
+ startPx - visible.left + (bracketWidth - bracketCurveWidth);
+ final endBracketX =
+ endPx - visible.left - (bracketWidth - bracketCurveWidth);
+ final bracketTopY = yForEvent(event) - visible.top - bracketVerticalPadding;
+ final bracketBottomY = bracketTopY +
+ event.depth * rowHeightWithPadding -
+ rowPadding +
+ bracketVerticalPadding * 2;
+
+ // Draw the start bracket.
+ canvas.drawPath(
+ Path()
+ ..moveTo(startBracketX, bracketTopY)
+ ..lineTo(startBracketX - bracketWidth + bracketCurveWidth, bracketTopY)
+ ..arcTo(
+ Rect.fromLTWH(
+ startBracketX - bracketWidth,
+ bracketTopY,
+ bracketCurveWidth * 2,
+ bracketCurveWidth * 2,
+ ),
+ degToRad(270),
+ degToRad(-90),
+ false,
+ )
+ ..lineTo(
+ startBracketX - bracketWidth,
+ bracketBottomY - bracketWidth,
+ )
+ ..arcTo(
+ Rect.fromLTWH(
+ startBracketX - bracketWidth,
+ bracketBottomY - bracketVerticalPadding * 2,
+ bracketCurveWidth * 2,
+ bracketCurveWidth * 2,
+ ),
+ degToRad(180),
+ degToRad(-90),
+ false,
+ )
+ ..lineTo(startBracketX, bracketBottomY),
+ paint,
+ );
+
+ // Draw the end bracket.
+ // TODO(kenz): reuse the path of the start bracket and transform it to draw
+ // the end bracket.
+ canvas.drawPath(
+ Path()
+ ..moveTo(endBracketX, bracketTopY)
+ ..lineTo(endBracketX + bracketWidth - bracketCurveWidth, bracketTopY)
+ ..arcTo(
+ Rect.fromLTWH(
+ endBracketX + bracketWidth - bracketCurveWidth * 2,
+ bracketTopY,
+ bracketCurveWidth * 2,
+ bracketCurveWidth * 2,
+ ),
+ degToRad(270),
+ degToRad(90),
+ false,
+ )
+ ..lineTo(
+ endBracketX + bracketWidth,
+ bracketBottomY - bracketWidth,
+ )
+ ..arcTo(
+ Rect.fromLTWH(
+ endBracketX + bracketWidth - bracketCurveWidth * 2,
+ bracketBottomY - bracketVerticalPadding * 2,
+ bracketCurveWidth * 2,
+ bracketCurveWidth * 2,
+ ),
+ degToRad(0),
+ degToRad(90),
+ false,
+ )
+ ..lineTo(endBracketX, bracketBottomY),
+ paint,
+ );
+ }
+
+ @override
+ bool shouldRepaint(SelectedFrameBracketPainter oldDelegate) =>
+ this != oldDelegate;
+
+ @override
+ bool operator ==(other) {
+ return selectedFrame == other.selectedFrame &&
+ zoom == other.zoom &&
+ constraints == other.constraints &&
+ verticalScrollOffset == other.verticalScrollOffset &&
+ horizontalScrollOffset == other.horizontalScrollOffset;
+ }
+
+ @override
+ int get hashCode => hashValues(
+ selectedFrame,
+ zoom,
+ constraints,
+ verticalScrollOffset,
+ horizontalScrollOffset,
+ );
+}
+
extension TimelineEventGroupDisplayExtension on TimelineEventGroup {
int get displaySize => rows.length + FlameChart.rowOffsetForSectionSpacer;
diff --git a/packages/devtools_app/lib/src/ui/colors.dart b/packages/devtools_app/lib/src/ui/colors.dart
index 889df0d..3096989 100644
--- a/packages/devtools_app/lib/src/ui/colors.dart
+++ b/packages/devtools_app/lib/src/ui/colors.dart
@@ -30,7 +30,7 @@
const selectedFrameBackgroundColor =
ThemedColor(Color(0xFFDBDDDD), Color(0xFF4E4F4F));
-const selectedFrameAccentColor = Color(0xFF36C6F4);
+const timelineSelectionColor = Color(0xFF36C6F4);
// Teal 200, 400 - see https://material.io/design/color/#tools-for-picking-colors.
const asyncColorPalette = [
diff --git a/packages/devtools_app/lib/src/utils.dart b/packages/devtools_app/lib/src/utils.dart
index 5b268bf..51d86e8 100644
--- a/packages/devtools_app/lib/src/utils.dart
+++ b/packages/devtools_app/lib/src/utils.dart
@@ -792,3 +792,6 @@
}).join();
}
}
+
+// Method to convert degrees to radians
+num degToRad(num deg) => deg * (pi / 180.0);
diff --git a/packages/devtools_app/test/flame_chart_test.dart b/packages/devtools_app/test/flame_chart_test.dart
index c92c203..81c1ff3 100644
--- a/packages/devtools_app/test/flame_chart_test.dart
+++ b/packages/devtools_app/test/flame_chart_test.dart
@@ -6,8 +6,8 @@
import 'package:devtools_app/src/flutter_widgets/linked_scroll_controller.dart';
import 'package:devtools_app/src/profiler/cpu_profile_model.dart';
import 'package:devtools_app/src/profiler/cpu_profile_flame_chart.dart';
-import 'package:devtools_app/src/theme.dart';
import 'package:devtools_app/src/timeline/timeline_model.dart';
+import 'package:devtools_app/src/ui/colors.dart';
import 'package:devtools_app/src/utils.dart';
import 'package:devtools_testing/support/cpu_profile_test_data.dart';
import 'package:devtools_testing/support/timeline_test_data.dart';
@@ -315,7 +315,7 @@
await pumpFlameChartNode(tester, selected: true, hovered: false);
expect(nodeFinder, findsOneWidget);
final Container nodeWidget = tester.widget(nodeFinder);
- expect(nodeWidget.color, equals(lightSelection));
+ expect(nodeWidget.color, equals(timelineSelectionColor));
expect(textFinder, findsOneWidget);
final Text textWidget = tester.widget(textFinder);
diff --git a/packages/devtools_app/test/flutter_frames_chart_test.dart b/packages/devtools_app/test/flutter_frames_chart_test.dart
index d62c078..e602bb3 100644
--- a/packages/devtools_app/test/flutter_frames_chart_test.dart
+++ b/packages/devtools_app/test/flutter_frames_chart_test.dart
@@ -29,7 +29,7 @@
expect(find.byType(FlutterFramesChart), findsOneWidget);
}
- group('TimelineScreen', () {
+ group('FlutterFramesChart', () {
setUp(() async {
setGlobal(
ServiceConnectionManager, FakeServiceManager(useFakeService: true));
@@ -60,4 +60,36 @@
expect(raster.color, equals(rasterJankColor));
});
});
+
+ group('FlutterFramesChartItem', () {
+ testWidgets('builds for selected frame', (WidgetTester tester) async {
+ await tester.pumpWidget(
+ // FlutterFramesChartItem needs to be wrapped in Material,
+ // Directionality, and Overlay in order to pump the widget and test.
+ Material(
+ child: Directionality(
+ textDirection: TextDirection.ltr,
+ child: Overlay(
+ initialEntries: [
+ OverlayEntry(
+ builder: (context) {
+ return FlutterFramesChartItem(
+ frame: testFrame0,
+ selected: true,
+ onSelected: () {},
+ msPerPx: 1,
+ availableChartHeight: 100.0,
+ displayRefreshRate: defaultRefreshRate,
+ );
+ },
+ ),
+ ],
+ ),
+ ),
+ ),
+ );
+ expect(find.byKey(FlutterFramesChartItem.selectedFrameIndicatorKey),
+ findsOneWidget);
+ });
+ });
}
diff --git a/packages/devtools_app/test/goldens/timeline_flame_chart_with_selected_frame.png b/packages/devtools_app/test/goldens/timeline_flame_chart_with_selected_frame.png
new file mode 100644
index 0000000..06022e1
--- /dev/null
+++ b/packages/devtools_app/test/goldens/timeline_flame_chart_with_selected_frame.png
Binary files differ
diff --git a/packages/devtools_app/test/timeline_flame_chart_test.dart b/packages/devtools_app/test/timeline_flame_chart_test.dart
index bf5fe32..c8bb185 100644
--- a/packages/devtools_app/test/timeline_flame_chart_test.dart
+++ b/packages/devtools_app/test/timeline_flame_chart_test.dart
@@ -71,5 +71,21 @@
findsOneWidget,
);
});
+
+ testWidgetsWithWindowSize(
+ 'builds flame chart with selected frame', windowSize,
+ (WidgetTester tester) async {
+ final controller = TimelineController();
+ await pumpTimelineBody(tester, controller);
+ expect(controller.data.frames.length, equals(1));
+ controller.selectFrame(controller.data.frames.first);
+ await tester.pumpAndSettle();
+
+ expect(find.byType(TimelineFlameChart), findsOneWidget);
+ expect(
+ find.byType(TimelineFlameChart),
+ matchesGoldenFile(
+ 'goldens/timeline_flame_chart_with_selected_frame.png'));
+ });
});
}
diff --git a/packages/devtools_testing/lib/support/timeline_test_data.dart b/packages/devtools_testing/lib/support/timeline_test_data.dart
index d4a96c9..a870baa 100644
--- a/packages/devtools_testing/lib/support/timeline_test_data.dart
+++ b/packages/devtools_testing/lib/support/timeline_test_data.dart
@@ -1028,7 +1028,9 @@
'tid': testRasterThreadId,
'args': {'name': '1.raster'},
},
+ frameStartEvent.json,
...goldenTraceEventsJson,
+ frameEndEvent.json,
],
'timeOriginMicros': 118039650802,
'timeExtentMicros': 118039679873 - 118039650802,
