Skip to content

Commit

Permalink
dialog [nfc]: Wrap showErrorDialog's return value
Browse files Browse the repository at this point in the history
As a result, showErrorDialog no longer returns a Future that can be
mistakenly awaited.  The wrapped return value offers clarity on how the
Future is supposed to be used.

There is no user visible change because existing callers (other than
lightbox's) that wait for its return value are all asynchronous
handlers for UI elements like buttons, and waits unnecessarily.

See also discussion
  #937 (comment).

Signed-off-by: Zixuan James Li <[email protected]>
  • Loading branch information
PIG208 committed Oct 5, 2024
1 parent 6979b50 commit 1e20eba
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 16 deletions.
8 changes: 4 additions & 4 deletions lib/widgets/action_sheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class AddThumbsUpButton extends MessageActionSheetMenuItemButton {
default:
}

await showErrorDialog(context: context,
showErrorDialog(context: context,
title: 'Adding reaction failed', message: errorMessage);
}
}
Expand Down Expand Up @@ -165,7 +165,7 @@ class StarButton extends MessageActionSheetMenuItemButton {
default:
}

await showErrorDialog(context: messageListContext,
showErrorDialog(context: messageListContext,
title: switch(op) {
UpdateMessageFlagsOp.remove => zulipLocalizations.errorUnstarMessageFailedTitle,
UpdateMessageFlagsOp.add => zulipLocalizations.errorStarMessageFailedTitle,
Expand Down Expand Up @@ -215,7 +215,7 @@ Future<String?> fetchRawContentWithFeedback({
// TODO(?) give no feedback on error conditions we expect to
// flag centrally in event polling, like invalid auth,
// user/realm deactivated. (Support with reusable code.)
await showErrorDialog(context: context,
showErrorDialog(context: context,
title: errorDialogTitle, message: errorMessage);
}

Expand Down Expand Up @@ -423,7 +423,7 @@ class ShareButton extends MessageActionSheetMenuItemButton {
// Until we learn otherwise, assume something wrong happened.
case ShareResultStatus.unavailable:
if (!messageListContext.mounted) return;
await showErrorDialog(context: messageListContext,
showErrorDialog(context: messageListContext,
title: zulipLocalizations.errorSharingFailed);
case ShareResultStatus.success:
case ShareResultStatus.dismissed:
Expand Down
2 changes: 1 addition & 1 deletion lib/widgets/actions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ Future<void> markNarrowAsRead(BuildContext context, Narrow narrow) async {
return;
} catch (e) {
if (!context.mounted) return;
await showErrorDialog(context: context,
showErrorDialog(context: context,
title: zulipLocalizations.errorMarkAsReadFailedTitle,
message: e.toString()); // TODO(#741): extract user-facing message better
return;
Expand Down
6 changes: 3 additions & 3 deletions lib/widgets/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1162,7 +1162,7 @@ class GlobalTime extends StatelessWidget {
}

void _launchUrl(BuildContext context, String urlString) async {
Future<void> showError(BuildContext context, String? message) {
DialogStatusController showError(BuildContext context, String? message) {
return showErrorDialog(context: context,
title: 'Unable to open link',
message: [
Expand All @@ -1174,7 +1174,7 @@ void _launchUrl(BuildContext context, String urlString) async {
final store = PerAccountStoreWidget.of(context);
final url = store.tryResolveUrl(urlString);
if (url == null) { // TODO(log)
await showError(context, null);
showError(context, null);
return;
}

Expand Down Expand Up @@ -1203,7 +1203,7 @@ void _launchUrl(BuildContext context, String urlString) async {
}
if (!launched) { // TODO(log)
if (!context.mounted) return;
await showError(context, errorMessage);
showError(context, errorMessage);
}
}

Expand Down
26 changes: 22 additions & 4 deletions lib/widgets/dialog.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,33 @@ Widget _dialogActionText(String text) {
);
}

/// Displays an [AlertDialog] with a dismiss button.
/// A wrapper providing access to the status of an [AlertDialog].
///
/// Returns a [Future] that resolves when the dialog is closed.
Future<void> showErrorDialog({
/// See also:
/// * [showDialog], whose return value this class is intended to wrap.
class DialogStatusController {
const DialogStatusController(this._closed);

/// Resolves when the dialog is closed.
Future<void> get closed => _closed;
final Future<void> _closed;
}

/// Displays an [AlertDialog] with a dismiss button.
///
/// Returns a [DialogStatusController]. Useful for checking if the dialog has
/// been closed.
// This API is inspired by [ScaffoldManager.showSnackBar]. We wrap
// [showDialog]'s return value, a [Future], inside [DialogStatusController]
// whose documentation can be accessed. This helps avoid confusion when
// intepreting the meaning of the [Future].
DialogStatusController showErrorDialog({
required BuildContext context,
required String title,
String? message,
}) {
final zulipLocalizations = ZulipLocalizations.of(context);
return showDialog(
final future = showDialog<void>(
context: context,
builder: (BuildContext context) => AlertDialog(
title: Text(title),
Expand All @@ -34,6 +51,7 @@ Future<void> showErrorDialog({
onPressed: () => Navigator.pop(context),
child: _dialogActionText(zulipLocalizations.errorDialogContinue)),
]));
return DialogStatusController(future);
}

void showSuggestedActionDialog({
Expand Down
4 changes: 2 additions & 2 deletions lib/widgets/lightbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -476,11 +476,11 @@ class _VideoLightboxPageState extends State<VideoLightboxPage> with PerAccountSt
assert(debugLog("VideoPlayerController.initialize failed: $error"));
if (!mounted) return;
final zulipLocalizations = ZulipLocalizations.of(context);
// Wait until the dialog is closed
await showErrorDialog(
final dialog = showErrorDialog(
context: context,
title: zulipLocalizations.errorDialogTitle,
message: zulipLocalizations.errorVideoPlayerFailed);
await dialog.closed;
if (!mounted) return;
Navigator.pop(context); // Pops the lightbox
}
Expand Down
4 changes: 2 additions & 2 deletions lib/widgets/login.dart
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ class _LoginPageState extends State<LoginPage> {
if (e is PlatformException && e.message != null) {
message = e.message!;
}
await showErrorDialog(context: context,
showErrorDialog(context: context,
title: zulipLocalizations.errorWebAuthOperationalErrorTitle,
message: message);
} finally {
Expand Down Expand Up @@ -351,7 +351,7 @@ class _LoginPageState extends State<LoginPage> {
if (e is PlatformException && e.message != null) {
message = e.message!;
}
await showErrorDialog(context: context,
showErrorDialog(context: context,
title: zulipLocalizations.errorWebAuthOperationalErrorTitle,
message: message);
}
Expand Down

0 comments on commit 1e20eba

Please sign in to comment.