Skip to content

Commit 60e8bd8

Browse files
PIG208oriohac
authored andcommitted
dialog [nfc]: Wrap showErrorDialog's return value
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 zulip#937 (comment). Signed-off-by: Zixuan James Li <[email protected]>
1 parent 18794eb commit 60e8bd8

File tree

2 files changed

+82
-83
lines changed

2 files changed

+82
-83
lines changed

lib/widgets/app.dart

Lines changed: 62 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -313,75 +313,74 @@ class HomePage extends StatelessWidget {
313313
return Scaffold(
314314
appBar: ZulipAppBar(title: const Text("Home")),
315315
body: Center(
316-
child: ElevatedButtonTheme(
316+
child: SingleChildScrollView(
317+
child: ElevatedButtonTheme(
317318
data: ElevatedButtonThemeData(style: ButtonStyle(
318319
backgroundColor: WidgetStatePropertyAll(colorScheme.secondaryContainer),
319320
foregroundColor: WidgetStatePropertyAll(colorScheme.onSecondaryContainer))),
320-
child: SingleChildScrollView(
321-
child: Center(
322-
child: Column(mainAxisAlignment: MainAxisAlignment.center, children: [
323-
DefaultTextStyle.merge(
324-
textAlign: TextAlign.center,
325-
style: const TextStyle(fontSize: 18),
326-
child: Column(children: [
327-
const Text('🚧 Under construction 🚧'),
328-
const SizedBox(height: 12),
329-
Text.rich(TextSpan(
330-
text: 'Connected to: ',
331-
children: [bold(store.realmUrl.toString())])),
332-
Text.rich(TextSpan(
333-
text: 'Zulip server version: ',
334-
children: [bold(store.zulipVersion)])),
335-
Text(zulipLocalizations.subscribedToNChannels(store.subscriptions.length)),
336-
])),
337-
const SizedBox(height: 16),
338-
ElevatedButton(
339-
onPressed: () => Navigator.push(context,
340-
MessageListPage.buildRoute(context: context,
341-
narrow: const CombinedFeedNarrow())),
342-
child: Text(zulipLocalizations.combinedFeedPageTitle)),
343-
const SizedBox(height: 16),
344-
ElevatedButton(
345-
onPressed: () => Navigator.push(context,
346-
MessageListPage.buildRoute(context: context,
347-
narrow: const MentionsNarrow())),
348-
child: Text(zulipLocalizations.mentionsPageTitle)),
349-
const SizedBox(height: 16),
350-
ElevatedButton(
351-
onPressed: () => Navigator.push(context,
352-
MessageListPage.buildRoute(context: context,
353-
narrow: const MentionsNarrow())),
354-
child: Text(zulipLocalizations.mentionsPageTitle)),
321+
child: Center(
322+
child: Column(mainAxisAlignment: MainAxisAlignment.center, children: [
323+
DefaultTextStyle.merge(
324+
textAlign: TextAlign.center,
325+
style: const TextStyle(fontSize: 18),
326+
child: Column(children: [
327+
const Text('🚧 Under construction 🚧'),
328+
const SizedBox(height: 12),
329+
Text.rich(TextSpan(
330+
text: 'Connected to: ',
331+
children: [bold(store.realmUrl.toString())])),
332+
Text.rich(TextSpan(
333+
text: 'Zulip server version: ',
334+
children: [bold(store.zulipVersion)])),
335+
Text(zulipLocalizations.subscribedToNChannels(store.subscriptions.length)),
336+
])),
337+
const SizedBox(height: 16),
338+
ElevatedButton(
339+
onPressed: () => Navigator.push(context,
340+
MessageListPage.buildRoute(context: context,
341+
narrow: const CombinedFeedNarrow())),
342+
child: Text(zulipLocalizations.combinedFeedPageTitle)),
343+
const SizedBox(height: 16),
344+
ElevatedButton(
345+
onPressed: () => Navigator.push(context,
346+
MessageListPage.buildRoute(context: context,
347+
narrow: const MentionsNarrow())),
348+
child: Text(zulipLocalizations.mentionsPageTitle)),
349+
const SizedBox(height: 16),
350+
ElevatedButton(
351+
onPressed: () => Navigator.push(context,
352+
MessageListPage.buildRoute(context: context,
353+
narrow: const MentionsNarrow())),
354+
child: Text(zulipLocalizations.mentionsPageTitle)),
355+
const SizedBox(height: 16),
356+
ElevatedButton(
357+
onPressed: () => Navigator.push(context,
358+
MessageListPage.buildRoute(context: context,
359+
narrow: const StarredMessagesNarrow())),
360+
child: Text(zulipLocalizations.starredMessagesPageTitle)),
361+
const SizedBox(height: 16),
362+
ElevatedButton(
363+
onPressed: () => Navigator.push(context,
364+
InboxPage.buildRoute(context: context)),
365+
child: const Text("Inbox")), // TODO(i18n)
366+
const SizedBox(height: 16),
367+
ElevatedButton(
368+
onPressed: () => Navigator.push(context,
369+
SubscriptionListPage.buildRoute(context: context)),
370+
child: const Text("Subscribed channels")),
371+
const SizedBox(height: 16),
372+
ElevatedButton(
373+
onPressed: () => Navigator.push(context,
374+
RecentDmConversationsPage.buildRoute(context: context)),
375+
child: Text(zulipLocalizations.recentDmConversationsPageTitle)),
376+
if (testStreamId != null) ...[
355377
const SizedBox(height: 16),
356378
ElevatedButton(
357379
onPressed: () => Navigator.push(context,
358380
MessageListPage.buildRoute(context: context,
359-
narrow: const StarredMessagesNarrow())),
360-
child: Text(zulipLocalizations.starredMessagesPageTitle)),
361-
const SizedBox(height: 16),
362-
ElevatedButton(
363-
onPressed: () => Navigator.push(context,
364-
InboxPage.buildRoute(context: context)),
365-
child: const Text("Inbox")), // TODO(i18n)
366-
const SizedBox(height: 16),
367-
ElevatedButton(
368-
onPressed: () => Navigator.push(context,
369-
SubscriptionListPage.buildRoute(context: context)),
370-
child: const Text("Subscribed channels")),
371-
const SizedBox(height: 16),
372-
ElevatedButton(
373-
onPressed: () => Navigator.push(context,
374-
RecentDmConversationsPage.buildRoute(context: context)),
375-
child: Text(zulipLocalizations.recentDmConversationsPageTitle)),
376-
if (testStreamId != null) ...[
377-
const SizedBox(height: 16),
378-
ElevatedButton(
379-
onPressed: () => Navigator.push(context,
380-
MessageListPage.buildRoute(context: context,
381-
narrow: ChannelNarrow(testStreamId!))),
382-
child: const Text("#test here")), // scaffolding hack, see above
383-
],
384-
])),
385-
))));
381+
narrow: ChannelNarrow(testStreamId!))),
382+
child: const Text("#test here")), // scaffolding hack, see above
383+
],
384+
]))))));
386385
}
387386
}

lib/widgets/dialog.dart

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,43 +15,44 @@ Widget _dialogActionText(String text) {
1515
);
1616
}
1717

18-
/// Tracks the status of a dialog, in being still open or already closed.
18+
/// A wrapper providing access to the status of an [AlertDialog].
1919
///
2020
/// See also:
2121
/// * [showDialog], whose return value this class is intended to wrap.
22-
class DialogStatus {
23-
const DialogStatus(this.closed);
22+
class DialogStatusController {
23+
const DialogStatusController(this._closed);
2424

2525
/// Resolves when the dialog is closed.
26-
final Future<void> closed;
26+
Future<void> get closed => _closed;
27+
final Future<void> _closed;
2728
}
2829

2930
/// Displays an [AlertDialog] with a dismiss button.
3031
///
31-
/// The [DialogStatus.closed] field of the return value can be used
32-
/// for waiting for the dialog to be closed.
32+
/// Returns a [DialogStatusController]. Useful for checking if the dialog has
33+
/// been closed.
3334
// This API is inspired by [ScaffoldManager.showSnackBar]. We wrap
34-
// [showDialog]'s return value, a [Future], inside [DialogStatus]
35+
// [showDialog]'s return value, a [Future], inside [DialogStatusController]
3536
// whose documentation can be accessed. This helps avoid confusion when
3637
// intepreting the meaning of the [Future].
37-
DialogStatus showErrorDialog({
38+
DialogStatusController showErrorDialog({
3839
required BuildContext context,
3940
required String title,
4041
String? message,
4142
}) {
4243
final zulipLocalizations = ZulipLocalizations.of(context);
4344
final future = showDialog<void>(
4445
context: context,
45-
builder: (BuildContext context) => AlertDialog(
46-
title: Text(title),
47-
content: message != null ? Text(message) : null,
48-
scrollable: true,
49-
actions: [
50-
TextButton(
51-
onPressed: () => Navigator.pop(context),
52-
child: _dialogActionText(zulipLocalizations.errorDialogContinue)),
53-
]));
54-
return DialogStatus(future);
46+
builder: (BuildContext context) => SingleChildScrollView(
47+
child: AlertDialog(
48+
title: Text(title),
49+
content: message != null ? Text(message) : null,
50+
actions: [
51+
TextButton(
52+
onPressed: () => Navigator.pop(context),
53+
child: _dialogActionText(zulipLocalizations.errorDialogContinue)),
54+
])));
55+
return DialogStatusController(future);
5556
}
5657

5758
void showSuggestedActionDialog({
@@ -66,8 +67,7 @@ void showSuggestedActionDialog({
6667
context: context,
6768
builder: (BuildContext context) => AlertDialog(
6869
title: Text(title),
69-
content: Text(message),
70-
scrollable: true,
70+
content: SingleChildScrollView(child: Text(message)),
7171
actions: [
7272
TextButton(
7373
onPressed: () => Navigator.pop(context),

0 commit comments

Comments
 (0)