From d11ba47aa0afdd25ddd3996e4023256fe4575eac Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 29 May 2020 15:49:29 -0700 Subject: [PATCH 1/2] Timeline offline mode fix and cleanup. --- .../flutter/import_export/import_export.dart | 11 +++++------ packages/devtools_app/lib/src/flutter/theme.dart | 4 +++- .../lib/src/memory/flutter/memory_chart.dart | 4 ++-- .../src/timeline/flutter/timeline_flame_chart.dart | 2 +- .../lib/src/timeline/flutter/timeline_screen.dart | 4 ++-- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart b/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart index 1199fa0289e..72f54ad0e11 100644 --- a/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart +++ b/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart @@ -35,17 +35,18 @@ class ImportController { final NotificationService _notifications; - bool importing = false; + int previousImportMs = 0; // TODO(kenz): improve error handling here or in snapshot_screen.dart. void importData(Map json) { - if (importing) return; - importing = true; + // Do not allow two different imports within 500 ms of each other. This is a + // workaround for the fact that we get two drop events for the same file. + final now = DateTime.now().millisecondsSinceEpoch; + if ((now - previousImportMs).abs() < 500) return; final isDevToolsSnapshot = json[devToolsSnapshotKey]; if (isDevToolsSnapshot == null || !isDevToolsSnapshot) { _notifications.push(nonDevToolsFileMessage); - importing = false; return; } @@ -54,8 +55,6 @@ class ImportController { offlineDataJson = json; _notifications.push(attemptingToImportMessage(activeScreenId)); _pushSnapshotScreenForImport(activeScreenId); - - importing = false; } } diff --git a/packages/devtools_app/lib/src/flutter/theme.dart b/packages/devtools_app/lib/src/flutter/theme.dart index 86c2c161e0e..221a9186058 100644 --- a/packages/devtools_app/lib/src/flutter/theme.dart +++ b/packages/devtools_app/lib/src/flutter/theme.dart @@ -156,7 +156,9 @@ Color titleSolidBackgroundColor(ThemeData theme) { return theme.isDarkTheme ? devtoolsGrey[900] : devtoolsGrey[50]; } -final chartBackgroundColor = ThemedColor(Colors.grey[50], Colors.grey[850]); +// TODO(kenz): can we access this color from the Theme somehow? This mirrors +// the background color of the app. +final defaultBackgroundColor = ThemedColor(Colors.grey[50], Colors.grey[850]); const chartAccentColor = ThemedColor(Color(0xFFCCCCCC), Color(0xFF585858)); const chartTextColor = ThemedColor(Colors.black, Colors.white); diff --git a/packages/devtools_app/lib/src/memory/flutter/memory_chart.dart b/packages/devtools_app/lib/src/memory/flutter/memory_chart.dart index 53d82745bdf..92a71a04393 100644 --- a/packages/devtools_app/lib/src/memory/flutter/memory_chart.dart +++ b/packages/devtools_app/lib/src/memory/flutter/memory_chart.dart @@ -278,7 +278,7 @@ class MemoryChartState extends State with AutoDisposeMixin { ..textSize = 2.0; */ }, - backgroundColor: chartBackgroundColor, + backgroundColor: defaultBackgroundColor, doubleTapToZoomEnabled: false, // TODO(terry): For now disable zoom with double-click. pinchZoomEnabled: false, @@ -342,7 +342,7 @@ class MemoryChartState extends State with AutoDisposeMixin { ..textSize = 2.0; */ }, - backgroundColor: chartBackgroundColor, + backgroundColor: defaultBackgroundColor, // TOD(terry): Disable zoom via double-click. Consider +/- button // for a controlled zoom in/zoom out. scaleXEnabled: false, diff --git a/packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart b/packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart index 11f0dfc6801..40cd9ae3bea 100644 --- a/packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart @@ -626,7 +626,7 @@ class TimelineGridPainter extends CustomPainter { constraints.maxWidth, math.min(constraints.maxHeight, rowHeight), ), - Paint()..color = chartBackgroundColor, + Paint()..color = defaultBackgroundColor, ); // Paint the timeline grid lines and corresponding timestamps in the flame 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 5dab18b420b..35a45d4105b 100644 --- a/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart @@ -111,7 +111,7 @@ class TimelineScreenBodyState extends State // Refresh data on page load if data is null. On subsequent tab changes, // this should not be called. - if (controller.data == null) { + if (controller.data == null && !offlineMode) { controller.refreshData(); } @@ -181,7 +181,7 @@ class TimelineScreenBodyState extends State timelineScreen, if (loadingOfflineData) Container( - color: Colors.grey[50], + color: defaultBackgroundColor, child: const Center( child: CircularProgressIndicator(), ), From 4d5bfb0e0b5bc0d7d23ad78afcf54c9a3d7a39a3 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Mon, 1 Jun 2020 07:29:10 -0700 Subject: [PATCH 2/2] Review comments --- .../flutter/import_export/import_export.dart | 14 +++++++++++--- packages/devtools_app/lib/src/flutter/theme.dart | 4 ++-- .../lib/src/timeline/flutter/timeline_screen.dart | 2 +- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart b/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart index 72f54ad0e11..1b916d7aa39 100644 --- a/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart +++ b/packages/devtools_app/lib/src/config_specific/flutter/import_export/import_export.dart @@ -31,18 +31,26 @@ class ImportController { this._pushSnapshotScreenForImport, ); + static const repeatImportTimeBufferMs = 500; + final void Function(String screenId) _pushSnapshotScreenForImport; final NotificationService _notifications; - int previousImportMs = 0; + DateTime previousImportTime; // TODO(kenz): improve error handling here or in snapshot_screen.dart. void importData(Map json) { // Do not allow two different imports within 500 ms of each other. This is a // workaround for the fact that we get two drop events for the same file. - final now = DateTime.now().millisecondsSinceEpoch; - if ((now - previousImportMs).abs() < 500) return; + final now = DateTime.now(); + if (previousImportTime != null && + (now.millisecondsSinceEpoch - previousImportTime.millisecondsSinceEpoch) + .abs() < + repeatImportTimeBufferMs) { + return; + } + previousImportTime = now; final isDevToolsSnapshot = json[devToolsSnapshotKey]; if (isDevToolsSnapshot == null || !isDevToolsSnapshot) { diff --git a/packages/devtools_app/lib/src/flutter/theme.dart b/packages/devtools_app/lib/src/flutter/theme.dart index 221a9186058..9de5e836f57 100644 --- a/packages/devtools_app/lib/src/flutter/theme.dart +++ b/packages/devtools_app/lib/src/flutter/theme.dart @@ -156,8 +156,8 @@ Color titleSolidBackgroundColor(ThemeData theme) { return theme.isDarkTheme ? devtoolsGrey[900] : devtoolsGrey[50]; } -// TODO(kenz): can we access this color from the Theme somehow? This mirrors -// the background color of the app. +// This is the same as Theme.of(context).scaffoldBackgroundColor, but we use +// this in places where we do not have access to the context. final defaultBackgroundColor = ThemedColor(Colors.grey[50], Colors.grey[850]); const chartAccentColor = ThemedColor(Color(0xFFCCCCCC), Color(0xFF585858)); const chartTextColor = ThemedColor(Colors.black, Colors.white); 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 35a45d4105b..a44254b7b4a 100644 --- a/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart +++ b/packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart @@ -181,7 +181,7 @@ class TimelineScreenBodyState extends State timelineScreen, if (loadingOfflineData) Container( - color: defaultBackgroundColor, + color: Theme.of(context).scaffoldBackgroundColor, child: const Center( child: CircularProgressIndicator(), ),