From 9aa047109228d086d9d2d3bc9c72737263afa47d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 8 Mar 2024 21:21:49 -0800 Subject: [PATCH 1/3] app: Add ZulipApp.scaffoldMessenger Also generalize ValueNotifierChecks to ValueListenableChecks. We can't write checks-extensions for ZulipApp.scaffoldMessenger or ZulipApp.ready, though, because Dart doesn't have extensions for static members. Apparently they're currently working on them: https://github.com/dart-lang/language/issues/723 Co-authored-by: Zixuan James Li --- lib/widgets/app.dart | 16 ++++++++++++++++ test/flutter_checks.dart | 3 ++- test/widgets/app_test.dart | 13 +++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 38abaad4a7..418bd36e5e 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -63,6 +63,22 @@ class ZulipApp extends StatefulWidget { /// to be mounted. static final navigatorKey = GlobalKey(); + /// The [ScaffoldMessengerState] for the app. + /// + /// This is null during the app's early startup, while [ready] is still false. + /// + /// For code that exists entirely outside the widget tree and has no natural + /// [BuildContext] of its own, this enables controlling snack bars. + /// Where a relevant [BuildContext] does exist, prefer using that instead, + /// with [ScaffoldMessenger.of]. + static ScaffoldMessengerState? get scaffoldMessenger { + final context = navigatorKey.currentContext; + if (context == null) return null; + // Not maybeOf; we use MaterialApp, which provides ScaffoldMessenger, + // so it's a bug if navigatorKey is mounted somewhere lacking that. + return ScaffoldMessenger.of(context); + } + /// Reset the state of [ZulipApp] statics, for testing. /// /// TODO refactor this better, perhaps unify with ZulipBinding diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index 641a2d8de1..3a2b2f2239 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -2,6 +2,7 @@ library; import 'package:checks/checks.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; @@ -51,7 +52,7 @@ extension RouteSettingsChecks on Subject { Subject get arguments => has((s) => s.arguments, 'arguments'); } -extension ValueNotifierChecks on Subject> { +extension ValueListenableChecks on Subject> { Subject get value => has((c) => c.value, 'value'); } diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index 62d6e47abf..a888fd9658 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -157,4 +157,17 @@ void main() { ..bottom.isLessThan(2 / 3 * screenHeight); }); }); + + group('scaffoldMessenger', () { + testWidgets('scaffoldMessenger becomes non-null after startup', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(const ZulipApp()); + + check(ZulipApp.scaffoldMessenger).isNull(); + check(ZulipApp.ready).value.isFalse(); + await tester.pump(); + check(ZulipApp.scaffoldMessenger).isNotNull(); + check(ZulipApp.ready).value.isTrue(); + }); + }); } From 2d5624c2010a353601d3cc8cc499654caf984b45 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 8 Mar 2024 21:33:33 -0800 Subject: [PATCH 2/3] log: Support reportErrorToUserBriefly The motivation of having this indirection, rather than using `showErrorDialog` and `showSnackBar` directly, is to keep dependencies of `lib/log.dart` from relying on widget code. Co-authored-by: Zixuan James Li Signed-off-by: Zixuan James Li --- assets/l10n/app_en.arb | 4 ++ lib/log.dart | 28 +++++++++ lib/widgets/app.dart | 39 ++++++++++++ test/widgets/app_test.dart | 118 +++++++++++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index 7cb8907827..f89e86aa2d 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -293,6 +293,10 @@ "@errorDialogTitle": { "description": "Generic title for error dialog." }, + "snackBarDetails": "Details", + "@snackBarDetails": { + "description": "Button label for snack bar button that opens a dialog with more details." + }, "lightboxCopyLinkTooltip": "Copy link", "@lightboxCopyLinkTooltip": { "description": "Tooltip in lightbox for the copy link action." diff --git a/lib/log.dart b/lib/log.dart index 2f8de60963..3fcbeddb2b 100644 --- a/lib/log.dart +++ b/lib/log.dart @@ -30,3 +30,31 @@ bool debugLog(String message) { }()); return true; } + +typedef ReportErrorCallback = void Function(String? message, {String? details}); + +/// Display an error message in a [SnackBar]. +/// +/// This shows a [SnackBar] containing the message if [ZulipApp] is ready, +/// otherwise logs it to the console. +/// +/// If `message` is null, this will clear the existing [SnackBar]s if there +/// are any. Useful for promptly dismissing errors. +/// +/// If `details` is non-null, the [SnackBar] will contain a button that would +/// open a dialog containing the error details. +// This gets set in [ZulipApp]. We need this indirection to keep `lib/log.dart` +// from importing widget code, because the file is a dependency for the rest of +// the app. +ReportErrorCallback reportErrorToUserBriefly = defaultReportErrorToUserBriefly; + +void defaultReportErrorToUserBriefly(String? message, {String? details}) { + // Error dismissing is a no-op to the default handler. + if (message == null) return; + // If this callback is still in place, then the app's widget tree + // hasn't mounted yet even as far as the [Navigator]. + // So there's not much we can do to tell the user; + // just log, in case the user is actually a developer watching the console. + assert(debugLog(message)); + if (details != null) assert(debugLog(details)); +} diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 418bd36e5e..557243f183 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -5,10 +5,12 @@ import 'package:flutter/material.dart'; import 'package:flutter/scheduler.dart'; import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; +import '../log.dart'; import '../model/localizations.dart'; import '../model/narrow.dart'; import 'about_zulip.dart'; import 'app_bar.dart'; +import 'dialog.dart'; import 'inbox.dart'; import 'login.dart'; import 'message_list.dart'; @@ -84,6 +86,8 @@ class ZulipApp extends StatefulWidget { /// TODO refactor this better, perhaps unify with ZulipBinding @visibleForTesting static void debugReset() { + _snackBarCount = 0; + reportErrorToUserBriefly = defaultReportErrorToUserBriefly; _ready.dispose(); _ready = ValueNotifier(false); } @@ -92,9 +96,44 @@ class ZulipApp extends StatefulWidget { /// Useful in tests. final List? navigatorObservers; + static int _snackBarCount = 0; + + static void _reportErrorToUserBriefly(String? message, {String? details}) { + assert(_ready.value); + + if (message == null) { + if (_snackBarCount == 0) return; + assert(_snackBarCount > 0); + // The snack bar API only exposes ways to hide ether the current snack + // bar or all of them. + // + // To reduce the possibility of hiding snack bars not created by this + // helper, only clear when there are known active snack bars. + scaffoldMessenger!.clearSnackBars(); + return; + } + + final localizations = ZulipLocalizations.of(navigatorKey.currentContext!); + final newSnackBar = scaffoldMessenger!.showSnackBar( + snackBarAnimationStyle: AnimationStyle( + duration: const Duration(milliseconds: 200), + reverseDuration: const Duration(milliseconds: 50)), + SnackBar( + content: Text(message), + action: (details == null) ? null : SnackBarAction( + label: localizations.snackBarDetails, + onPressed: () => showErrorDialog(context: navigatorKey.currentContext!, + title: localizations.errorDialogTitle, + message: details)))); + + _snackBarCount++; + newSnackBar.closed.whenComplete(() => _snackBarCount--); + } + void _declareReady() { assert(navigatorKey.currentContext != null); _ready.value = true; + reportErrorToUserBriefly = _reportErrorToUserBriefly; } @override diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index a888fd9658..683298f017 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -1,6 +1,7 @@ import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/log.dart'; import 'package:zulip/model/database.dart'; import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/inbox.dart'; @@ -10,6 +11,7 @@ import '../example_data.dart' as eg; import '../flutter_checks.dart'; import '../model/binding.dart'; import '../test_navigation.dart'; +import 'dialog_checks.dart'; import 'page_checks.dart'; import 'test_app.dart'; @@ -169,5 +171,121 @@ void main() { check(ZulipApp.scaffoldMessenger).isNotNull(); check(ZulipApp.ready).value.isTrue(); }); + + Finder findSnackBarByText(String text) => find.descendant( + of: find.byType(SnackBar), + matching: find.text(text)); + + testWidgets('reportErrorToUserBriefly', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(const ZulipApp()); + const message = 'test error message'; + + // Prior to app startup, reportErrorToUserBriefly only logs. + reportErrorToUserBriefly(message); + check(ZulipApp.ready).value.isFalse(); + await tester.pump(); + check(findSnackBarByText(message).evaluate()).isEmpty(); + + check(ZulipApp.ready).value.isTrue(); + // After app startup, reportErrorToUserBriefly displays a SnackBar. + reportErrorToUserBriefly(message); + await tester.pump(); + check(findSnackBarByText(message).evaluate()).single; + check(find.text('Details').evaluate()).isEmpty(); + }); + + testWidgets('reportErrorToUserBriefly with details', (tester) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(const ZulipApp()); + const message = 'test error message'; + const details = 'error details'; + + // Prior to app startup, reportErrorToUserBriefly only logs. + reportErrorToUserBriefly(message, details: details); + check(ZulipApp.ready).value.isFalse(); + await tester.pump(); + check(findSnackBarByText(message).evaluate()).isEmpty(); + check(find.byType(AlertDialog).evaluate()).isEmpty(); + + check(ZulipApp.ready).value.isTrue(); + // After app startup, reportErrorToUserBriefly displays a SnackBar. + reportErrorToUserBriefly(message, details: details); + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).single; + check(find.byType(AlertDialog).evaluate()).isEmpty(); + + // Open the error details dialog. + await tester.tap(find.text('Details')); + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).isEmpty(); + checkErrorDialog(tester, expectedTitle: 'Error', expectedMessage: details); + }); + + Future prepareSnackBarWithDetails(WidgetTester tester, String message, String details) async { + addTearDown(testBinding.reset); + await tester.pumpWidget(const ZulipApp()); + await tester.pump(); + check(ZulipApp.ready).value.isTrue(); + + reportErrorToUserBriefly(message, details: details); + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).single; + } + + testWidgets('reportErrorToUser dismissing SnackBar', (tester) async { + const message = 'test error message'; + const details = 'error details'; + await prepareSnackBarWithDetails(tester, message, details); + + // Dismissing the SnackBar. + reportErrorToUserBriefly(null); + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).isEmpty(); + + // Verify that the SnackBar would otherwise stay when not dismissed. + reportErrorToUserBriefly(message, details: details); + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).single; + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).single; + }); + + testWidgets('reportErrorToUserBriefly(null) does not dismiss dialog', (tester) async { + const message = 'test error message'; + const details = 'error details'; + await prepareSnackBarWithDetails(tester, message, details); + + // Open the error details dialog. + await tester.tap(find.text('Details')); + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).isEmpty(); + checkErrorDialog(tester, expectedTitle: 'Error', expectedMessage: details); + + // The dialog should not get dismissed. + reportErrorToUserBriefly(null); + await tester.pumpAndSettle(); + checkErrorDialog(tester, expectedTitle: 'Error', expectedMessage: details); + }); + + testWidgets('reportErrorToUserBriefly(null) does not dismiss unrelated SnackBar', (tester) async { + const message = 'test error message'; + const details = 'error details'; + await prepareSnackBarWithDetails(tester, message, details); + + // Dismissing the SnackBar. + reportErrorToUserBriefly(null); + await tester.pumpAndSettle(); + check(findSnackBarByText(message).evaluate()).isEmpty(); + + // Unrelated SnackBars should not be dismissed. + ZulipApp.scaffoldMessenger!.showSnackBar( + const SnackBar(content: Text ('unrelated'))); + await tester.pumpAndSettle(); + check(findSnackBarByText('unrelated').evaluate()).single; + reportErrorToUserBriefly(null); + await tester.pumpAndSettle(); + check(findSnackBarByText('unrelated').evaluate()).single; + }); }); } From e36f98495e6ef155c645d64d75f006a57321af9a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 23 Sep 2024 20:27:03 -0700 Subject: [PATCH 3/3] log [nfc]: Revise reportErrorToUserBriefly doc; tweak related comments --- lib/log.dart | 8 +++++--- lib/widgets/app.dart | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/log.dart b/lib/log.dart index 3fcbeddb2b..e3261f8cba 100644 --- a/lib/log.dart +++ b/lib/log.dart @@ -33,10 +33,12 @@ bool debugLog(String message) { typedef ReportErrorCallback = void Function(String? message, {String? details}); -/// Display an error message in a [SnackBar]. +/// Show the user an error message, without requiring them to interact with it. /// -/// This shows a [SnackBar] containing the message if [ZulipApp] is ready, -/// otherwise logs it to the console. +/// Typically this shows a [SnackBar] containing the message. +/// If called before the app's widget tree is ready (see [ZulipApp.ready]), +/// then we give up on showing the message to the user, +/// and just log the message to the console. /// /// If `message` is null, this will clear the existing [SnackBar]s if there /// are any. Useful for promptly dismissing errors. diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 557243f183..3912f37a29 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -98,13 +98,14 @@ class ZulipApp extends StatefulWidget { static int _snackBarCount = 0; + /// The callback we normally use as [reportErrorToUserBriefly]. static void _reportErrorToUserBriefly(String? message, {String? details}) { assert(_ready.value); if (message == null) { if (_snackBarCount == 0) return; assert(_snackBarCount > 0); - // The snack bar API only exposes ways to hide ether the current snack + // The [SnackBar] API only exposes ways to hide ether the current snack // bar or all of them. // // To reduce the possibility of hiding snack bars not created by this