Skip to content

Support displaying user visible error messages #935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
30 changes: 30 additions & 0 deletions lib/log.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,33 @@ bool debugLog(String message) {
}());
return true;
}

typedef ReportErrorCallback = void Function(String? message, {String? details});

/// Show the user an error message, without requiring them to interact with it.
///
/// 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.
///
/// 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));
}
56 changes: 56 additions & 0 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -63,11 +65,29 @@ class ZulipApp extends StatefulWidget {
/// to be mounted.
static final navigatorKey = GlobalKey<NavigatorState>();

/// 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
@visibleForTesting
static void debugReset() {
_snackBarCount = 0;
reportErrorToUserBriefly = defaultReportErrorToUserBriefly;
_ready.dispose();
_ready = ValueNotifier(false);
}
Expand All @@ -76,9 +96,45 @@ class ZulipApp extends StatefulWidget {
/// Useful in tests.
final List<NavigatorObserver>? navigatorObservers;

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 [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
// 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
Expand Down
3 changes: 2 additions & 1 deletion test/flutter_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -51,7 +52,7 @@ extension RouteSettingsChecks<T> on Subject<RouteSettings> {
Subject<Object?> get arguments => has((s) => s.arguments, 'arguments');
}

extension ValueNotifierChecks<T> on Subject<ValueNotifier<T>> {
extension ValueListenableChecks<T> on Subject<ValueListenable<T>> {
Subject<T> get value => has((c) => c.value, 'value');
}

Expand Down
131 changes: 131 additions & 0 deletions test/widgets/app_test.dart
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';

Expand Down Expand Up @@ -157,4 +159,133 @@ 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();
});

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<void> 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;
});
});
}