From 86bbc8ad006b9036ab3f07c6aceb3dec3c779d1e Mon Sep 17 00:00:00 2001 From: Daniel Chevalier Date: Wed, 30 Aug 2023 16:51:54 -0400 Subject: [PATCH] Clean up Logging page top bar (#6281) ![](https://media.giphy.com/media/3ov9k3Pq54ky2rNyp2/giphy-downsized.gif) Fixes https://github.com/flutter/devtools/issues/4921 https://github.com/flutter/devtools/assets/1386322/ebe4f959-72b3-4d02-a755-a1bbef7fb577 # Open question The structured errors are saved in the service, not in the local preferences. Should we have a way to somehow let the user know that this setting is saved in their app? --- .../src/screens/logging/logging_screen.dart | 50 ++++++++++++++++++- .../src/screens/logging/shared/constants.dart | 5 ++ .../service/service_extension_widgets.dart | 7 +-- .../lib/src/shared/analytics/constants.dart | 1 + .../release_notes/NEXT_RELEASE_NOTES.md | 2 + .../test/logging/logging_screen_test.dart | 12 ++++- .../lib/src/ui/common.dart | 6 +-- .../lib/src/ui/theme/theme.dart | 17 +++---- 8 files changed, 80 insertions(+), 20 deletions(-) create mode 100644 packages/devtools_app/lib/src/screens/logging/shared/constants.dart diff --git a/packages/devtools_app/lib/src/screens/logging/logging_screen.dart b/packages/devtools_app/lib/src/screens/logging/logging_screen.dart index 990ad6ba838..eb1b47e1201 100644 --- a/packages/devtools_app/lib/src/screens/logging/logging_screen.dart +++ b/packages/devtools_app/lib/src/screens/logging/logging_screen.dart @@ -21,6 +21,7 @@ import '../../shared/utils.dart'; import '_log_details.dart'; import '_logs_table.dart'; import 'logging_controller.dart'; +import 'shared/constants.dart'; /// Presents logs from the connected app. class LoggingScreen extends Screen { @@ -123,13 +124,15 @@ class _LoggingScreenState extends State onPressed: controller.clear, gaScreen: gac.logging, gaSelection: gac.clear, + minScreenWidthForTextBeforeScaling: loggingMinVerboseWidth, ), const Spacer(), - const StructuredErrorsToggle(), const SizedBox(width: denseSpacing), // TODO(kenz): fix focus issue when state is refreshed SearchField( - searchFieldWidth: wideSearchFieldWidth, + searchFieldWidth: isScreenWiderThan(context, loggingMinVerboseWidth) + ? wideSearchFieldWidth + : defaultSearchFieldWidth, searchController: controller, searchFieldEnabled: hasData, ), @@ -145,6 +148,20 @@ class _LoggingScreenState extends State .joinWithTrailing('\n'), tooltip: 'Copy filtered logs', ), + const SizedBox(width: denseSpacing), + SettingsOutlinedButton( + gaScreen: gac.logging, + gaSelection: gac.loggingSettings, + tooltip: 'Logging Settings', + onPressed: () { + unawaited( + showDialog( + context: context, + builder: (context) => const LoggingSettingsDialog(), + ), + ); + }, + ), ], ); } @@ -189,3 +206,32 @@ class _LoggingScreenState extends State ); } } + +class LoggingSettingsDialog extends StatelessWidget { + const LoggingSettingsDialog({super.key}); + + @override + Widget build(BuildContext context) { + final theme = Theme.of(context); + return DevToolsDialog( + title: const DialogTitleText('Logging Settings'), + content: SizedBox( + width: defaultDialogWidth, + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + ...dialogSubHeader( + theme, + 'General', + ), + const StructuredErrorsToggle(), + ], + ), + ), + actions: const [ + DialogCloseButton(), + ], + ); + } +} diff --git a/packages/devtools_app/lib/src/screens/logging/shared/constants.dart b/packages/devtools_app/lib/src/screens/logging/shared/constants.dart new file mode 100644 index 00000000000..20c02803cd7 --- /dev/null +++ b/packages/devtools_app/lib/src/screens/logging/shared/constants.dart @@ -0,0 +1,5 @@ +// Copyright 2023 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. + +const loggingMinVerboseWidth = 650.0; diff --git a/packages/devtools_app/lib/src/service/service_extension_widgets.dart b/packages/devtools_app/lib/src/service/service_extension_widgets.dart index 57596987c87..fd4606097b5 100644 --- a/packages/devtools_app/lib/src/service/service_extension_widgets.dart +++ b/packages/devtools_app/lib/src/service/service_extension_widgets.dart @@ -184,9 +184,10 @@ class ServiceExtensionButton extends StatelessWidget { child: Container( height: defaultButtonHeight, padding: EdgeInsets.symmetric( - horizontal: includeText(context, minScreenWidthForTextBeforeScaling) - ? defaultSpacing - : 0.0, + horizontal: + isScreenWiderThan(context, minScreenWidthForTextBeforeScaling) + ? defaultSpacing + : 0.0, ), child: ImageIconLabel( ServiceExtensionIcon(extensionState: extensionState), diff --git a/packages/devtools_app/lib/src/shared/analytics/constants.dart b/packages/devtools_app/lib/src/shared/analytics/constants.dart index 0678ab9ef26..99037ea4856 100644 --- a/packages/devtools_app/lib/src/shared/analytics/constants.dart +++ b/packages/devtools_app/lib/src/shared/analytics/constants.dart @@ -68,6 +68,7 @@ const enableOnDeviceInspector = 'enableOnDeviceInspector'; const showOnDeviceInspector = 'showInspector'; const treeNodeSelection = 'treeNodeSelection'; const inspectorSettings = 'inspectorSettings'; +const loggingSettings = 'loggingSettings'; const refreshPubRoots = 'refreshPubRoots'; enum HomeScreenEvents { diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index ae6e53aa4e1..222fb61e019 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md @@ -27,6 +27,8 @@ TODO: Remove this section if there are not any general updates. TODO: Remove this section if there are not any general updates. ## Logging updates +* Improved responsiveness of the top bar in the Logging page - [#6281](https://github.com/flutter/devtools/pull/6281) + * Add the ability to copy filtered logs - [#6260](https://github.com/flutter/devtools/pull/6260) ![Logger tab copy](images/logger_copy.png "Logger tab copy") ## App size tool updates diff --git a/packages/devtools_app/test/logging/logging_screen_test.dart b/packages/devtools_app/test/logging/logging_screen_test.dart index 3b2109b4dfe..0df3f908669 100644 --- a/packages/devtools_app/test/logging/logging_screen_test.dart +++ b/packages/devtools_app/test/logging/logging_screen_test.dart @@ -108,7 +108,7 @@ void main() { expect(find.byType(ClearButton), findsOneWidget); expect(find.byType(TextField), findsOneWidget); expect(find.byType(DevToolsFilterButton), findsOneWidget); - expect(find.byType(StructuredErrorsToggle), findsOneWidget); + expect(find.byType(SettingsOutlinedButton), findsOneWidget); }, ); @@ -151,7 +151,15 @@ void main() { serviceConnection, ); await pumpLoggingScreen(tester); - Switch toggle = tester.widget(find.byType(Switch)); + + await tester.tap(find.byType(SettingsOutlinedButton)); + await tester.pump(); + Switch toggle = tester.widget( + find.descendant( + of: find.byType(StructuredErrorsToggle), + matching: find.byType(Switch), + ), + ); expect(toggle.value, false); serviceConnection.serviceManager.serviceExtensionManager diff --git a/packages/devtools_app_shared/lib/src/ui/common.dart b/packages/devtools_app_shared/lib/src/ui/common.dart index bedb43ec6f9..83e45f21670 100644 --- a/packages/devtools_app_shared/lib/src/ui/common.dart +++ b/packages/devtools_app_shared/lib/src/ui/common.dart @@ -404,7 +404,7 @@ class DevToolsButton extends StatelessWidget { tooltipPadding: tooltipPadding, child: SizedBox( height: defaultButtonHeight, - width: !includeText(context, minScreenWidthForTextBeforeScaling) + width: !isScreenWiderThan(context, minScreenWidthForTextBeforeScaling) ? buttonMinWidth : null, child: outlined @@ -613,7 +613,7 @@ final class ImageIconLabel extends StatelessWidget { children: [ icon, // TODO(jacobr): animate showing and hiding the text. - if (includeText(context, unscaledMinIncludeTextWidth)) + if (isScreenWiderThan(context, unscaledMinIncludeTextWidth)) Padding( padding: const EdgeInsets.only(left: 8.0), child: Text(text), @@ -655,7 +655,7 @@ final class MaterialIconLabel extends StatelessWidget { ), // TODO(jacobr): animate showing and hiding the text. if (label != null && - includeText(context, minScreenWidthForTextBeforeScaling)) + isScreenWiderThan(context, minScreenWidthForTextBeforeScaling)) Padding( padding: EdgeInsets.only( left: iconData != null ? denseSpacing : 0.0, diff --git a/packages/devtools_app_shared/lib/src/ui/theme/theme.dart b/packages/devtools_app_shared/lib/src/ui/theme/theme.dart index aa1ce6355c9..5840f6acff0 100644 --- a/packages/devtools_app_shared/lib/src/ui/theme/theme.dart +++ b/packages/devtools_app_shared/lib/src/ui/theme/theme.dart @@ -487,18 +487,15 @@ const defaultCurve = Curves.easeInOutCubic; CurvedAnimation defaultCurvedAnimation(AnimationController parent) => CurvedAnimation(curve: defaultCurve, parent: parent); -/// Measures the screen size to determine whether text should be included for -/// the value of [minScreenWidthForTextBeforeScaling]. -/// -/// Returns false if the screen width is smaller than minimum size required to -/// include text [minScreenWidthForTextBeforeScaling]. -bool includeText( +/// Measures the screen size to determine whether it is strictly larger +/// than [width], scaled to the current font factor. +bool isScreenWiderThan( BuildContext context, - double? minScreenWidthForTextBeforeScaling, + double? width, ) { - return minScreenWidthForTextBeforeScaling == null || + return width == null || MediaQuery.of(context).size.width > - scaleByFontFactor(minScreenWidthForTextBeforeScaling); + scaleByFontFactor(width); } ButtonStyle denseAwareOutlinedButtonStyle( @@ -532,7 +529,7 @@ ButtonStyle _generateButtonStyle({ required ButtonStyle buttonStyle, double? minScreenWidthForTextBeforeScaling, }) { - if (!includeText(context, minScreenWidthForTextBeforeScaling)) { + if (!isScreenWiderThan(context, minScreenWidthForTextBeforeScaling)) { buttonStyle = buttonStyle.copyWith( padding: MaterialStateProperty.resolveWith((_) { return EdgeInsets.zero;