Skip to content

lightbox: Replace onDismiss with await showErrorDialog #937

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.

Sign up for GitHub

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 1 commit into from
Sep 25, 2024
Merged

lightbox: Replace onDismiss with await showErrorDialog #937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 4 additions & 7 deletions lib/widgets/dialog.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,23 @@ Widget _dialogActionText(String text) {
);
}

/// Displays an [AlertDialog] with a dismiss button.
///
/// Returns a [Future] that resolves when the dialog is closed.
Future<void> showErrorDialog({
Comment on lines +20 to 21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior (which showDialog has and we faithfully wrap) is kind of a bad pattern for an API, as we've discussed in the context of #731 / the unawaited_futures lint. If a function is named "show thing" and its return type is some Future<T>, the future should complete when the thing has been shown, not when some possibly-much-later event happens.

I think a good model here is ScaffoldMessengerState.showSnackBar: there's a Future it wants to provide for callers to potentially await on, but it's for something that happens after the function's own work is done, so instead of returning a value that is a Future it returns a value that has a Future. And the property name, closed, makes it clear what event is being awaited.

Anyway, that's a problem for outside this PR.

required BuildContext context,
required String title,
String? message,
VoidCallback? onDismiss,
}) {
final zulipLocalizations = ZulipLocalizations.of(context);
return showDialog(
context: context,
// `showDialog` doesn't take an `onDismiss`, so dismissing via the barrier
// always causes the default dismiss behavior of popping just this route.
// When we want a non-default `onDismiss`, disable that.
// TODO(upstream): add onDismiss to showDialog, passing through to [ModalBarrier.onDismiss]
barrierDismissible: onDismiss == null,
builder: (BuildContext context) => AlertDialog(
title: Text(title),
content: message != null ? SingleChildScrollView(child: Text(message)) : null,
actions: [
TextButton(
onPressed: onDismiss ?? () => Navigator.pop(context),
onPressed: () => Navigator.pop(context),
child: _dialogActionText(zulipLocalizations.errorDialogContinue)),
]));
}
Expand Down
20 changes: 9 additions & 11 deletions lib/widgets/lightbox.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,17 +474,15 @@ class _VideoLightboxPageState extends State<VideoLightboxPage> with PerAccountSt
await _controller!.play();
} catch (error) { // TODO(log)
assert(debugLog("VideoPlayerController.initialize failed: $error"));
if (mounted) {
final zulipLocalizations = ZulipLocalizations.of(context);
await showErrorDialog(
context: context,
title: zulipLocalizations.errorDialogTitle,
message: zulipLocalizations.errorVideoPlayerFailed,
onDismiss: () {
Navigator.pop(context); // Pops the dialog
Navigator.pop(context); // Pops the lightbox
});
}
if (!mounted) return;
final zulipLocalizations = ZulipLocalizations.of(context);
// Wait until the dialog is closed
await showErrorDialog(
context: context,
title: zulipLocalizations.errorDialogTitle,
message: zulipLocalizations.errorVideoPlayerFailed);
if (!mounted) return;
Navigator.pop(context); // Pops the lightbox
}
}

Expand Down