Skip to content

Commit 2a734c7

Browse files
Khader-1gnprice
Khader-1
authored and
gnprice
committed
actions [nfc]: Port generic error handling to markNarrowAsRead 
1 parent 7392ccb commit 2a734c7

File tree

3 files changed

+97
-100
lines changed

3 files changed

+97
-100
lines changed

‎lib/widgets/actions.dart

Lines changed: 93 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -22,97 +22,106 @@ Future<bool> markNarrowAsRead(
2222
Narrow narrow,
2323
bool useLegacy, // TODO(server-6)
2424
) async {
25-
final store = PerAccountStoreWidget.of(context);
26-
final connection = store.connection;
27-
if (useLegacy) {
28-
await _legacyMarkNarrowAsRead(context, narrow);
29-
return true;
30-
}
25+
try {
26+
final store = PerAccountStoreWidget.of(context);
27+
final connection = store.connection;
28+
if (useLegacy) {
29+
await _legacyMarkNarrowAsRead(context, narrow);
30+
return true;
31+
}
3132

32-
// Compare web's `mark_all_as_read` in web/src/unread_ops.js
33-
// and zulip-mobile's `markAsUnreadFromMessage` in src/action-sheets/index.js .
34-
final zulipLocalizations = ZulipLocalizations.of(context);
35-
final scaffoldMessenger = ScaffoldMessenger.of(context);
36-
// Use [AnchorCode.oldest], because [AnchorCode.firstUnread]
37-
// will be the oldest non-muted unread message, which would
38-
// result in muted unreads older than the first unread not
39-
// being processed.
40-
Anchor anchor = AnchorCode.oldest;
41-
int responseCount = 0;
42-
int updatedCount = 0;
33+
// Compare web's `mark_all_as_read` in web/src/unread_ops.js
34+
// and zulip-mobile's `markAsUnreadFromMessage` in src/action-sheets/index.js .
35+
final zulipLocalizations = ZulipLocalizations.of(context);
36+
final scaffoldMessenger = ScaffoldMessenger.of(context);
37+
// Use [AnchorCode.oldest], because [AnchorCode.firstUnread]
38+
// will be the oldest non-muted unread message, which would
39+
// result in muted unreads older than the first unread not
40+
// being processed.
41+
Anchor anchor = AnchorCode.oldest;
42+
int responseCount = 0;
43+
int updatedCount = 0;
4344

44-
// Include `is:unread` in the narrow. That has a database index, so
45-
// this can be an important optimization in narrows with a lot of history.
46-
// The server applies the same optimization within the (deprecated)
47-
// specialized endpoints for marking messages as read; see
48-
// `do_mark_stream_messages_as_read` in `zulip:zerver/actions/message_flags.py`.
49-
final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread());
45+
// Include `is:unread` in the narrow. That has a database index, so
46+
// this can be an important optimization in narrows with a lot of history.
47+
// The server applies the same optimization within the (deprecated)
48+
// specialized endpoints for marking messages as read; see
49+
// `do_mark_stream_messages_as_read` in `zulip:zerver/actions/message_flags.py`.
50+
final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread());
5051

51-
while (true) {
52-
final result = await updateMessageFlagsForNarrow(connection,
53-
anchor: anchor,
54-
// [AnchorCode.oldest] is an anchor ID lower than any valid
55-
// message ID; and follow-up requests will have already
56-
// processed the anchor ID, so we just want this to be
57-
// unconditionally false.
58-
includeAnchor: false,
59-
// There is an upper limit of 5000 messages per batch
60-
// (numBefore + numAfter <= 5000) enforced on the server.
61-
// See `update_message_flags_in_narrow` in zerver/views/message_flags.py .
62-
// zulip-mobile uses `numAfter` of 5000, but web uses 1000
63-
// for more responsive feedback. See zulip@f0d87fcf6.
64-
numBefore: 0,
65-
numAfter: 1000,
66-
narrow: apiNarrow,
67-
op: UpdateMessageFlagsOp.add,
68-
flag: MessageFlag.read);
69-
if (!context.mounted) {
70-
scaffoldMessenger.clearSnackBars();
71-
return false;
72-
}
73-
responseCount++;
74-
updatedCount += result.updatedCount;
52+
while (true) {
53+
final result = await updateMessageFlagsForNarrow(connection,
54+
anchor: anchor,
55+
// [AnchorCode.oldest] is an anchor ID lower than any valid
56+
// message ID; and follow-up requests will have already
57+
// processed the anchor ID, so we just want this to be
58+
// unconditionally false.
59+
includeAnchor: false,
60+
// There is an upper limit of 5000 messages per batch
61+
// (numBefore + numAfter <= 5000) enforced on the server.
62+
// See `update_message_flags_in_narrow` in zerver/views/message_flags.py .
63+
// zulip-mobile uses `numAfter` of 5000, but web uses 1000
64+
// for more responsive feedback. See zulip@f0d87fcf6.
65+
numBefore: 0,
66+
numAfter: 1000,
67+
narrow: apiNarrow,
68+
op: UpdateMessageFlagsOp.add,
69+
flag: MessageFlag.read);
70+
if (!context.mounted) {
71+
scaffoldMessenger.clearSnackBars();
72+
return false;
73+
}
74+
responseCount++;
75+
updatedCount += result.updatedCount;
7576

76-
if (result.foundNewest) {
77-
if (responseCount > 1) {
78-
// We previously showed an in-progress [SnackBar], so say we're done.
79-
// There may be a backlog of [SnackBar]s accumulated in the queue
80-
// so be sure to clear them out here.
81-
scaffoldMessenger
82-
..clearSnackBars()
83-
..showSnackBar(SnackBar(behavior: SnackBarBehavior.floating,
84-
content: Text(zulipLocalizations.markAsReadComplete(updatedCount))));
77+
if (result.foundNewest) {
78+
if (responseCount > 1) {
79+
// We previously showed an in-progress [SnackBar], so say we're done.
80+
// There may be a backlog of [SnackBar]s accumulated in the queue
81+
// so be sure to clear them out here.
82+
scaffoldMessenger
83+
..clearSnackBars()
84+
..showSnackBar(SnackBar(behavior: SnackBarBehavior.floating,
85+
content: Text(zulipLocalizations.markAsReadComplete(updatedCount))));
86+
}
87+
return true;
8588
}
86-
return true;
87-
}
8889

89-
if (result.lastProcessedId == null) {
90-
// No messages were in the range of the request.
91-
// This should be impossible given that `foundNewest` was false
92-
// (and that our `numAfter` was positive.)
93-
await showErrorDialog(context: context,
94-
title: zulipLocalizations.errorMarkAsReadFailedTitle,
95-
message: zulipLocalizations.errorInvalidResponse);
96-
return false;
97-
}
98-
anchor = NumericAnchor(result.lastProcessedId!);
90+
if (result.lastProcessedId == null) {
91+
// No messages were in the range of the request.
92+
// This should be impossible given that `foundNewest` was false
93+
// (and that our `numAfter` was positive.)
94+
showErrorDialog(context: context,
95+
title: zulipLocalizations.errorMarkAsReadFailedTitle,
96+
message: zulipLocalizations.errorInvalidResponse);
97+
return false;
98+
}
99+
anchor = NumericAnchor(result.lastProcessedId!);
99100

100-
// The task is taking a while, so tell the user we're working on it.
101-
// No need to say how many messages, as the [MarkAsUnread] widget
102-
// should follow along.
103-
// TODO: Ideally we'd have a progress widget here that showed up based
104-
// on actual time elapsed -- so it could appear before the first
105-
// batch returns, if that takes a while -- and that then stuck
106-
// around continuously until the task ends. For now we use a
107-
// series of [SnackBar]s, which may feel a bit janky.
108-
// There is complexity in tracking the status of each [SnackBar],
109-
// due to having no way to determine which is currently active,
110-
// or if there is an active one at all. Resetting the [SnackBar] here
111-
// results in the same message popping in and out and the user experience
112-
// is better for now if we allow them to run their timer through
113-
// and clear the backlog later.
114-
scaffoldMessenger.showSnackBar(SnackBar(behavior: SnackBarBehavior.floating,
115-
content: Text(zulipLocalizations.markAsReadInProgress)));
101+
// The task is taking a while, so tell the user we're working on it.
102+
// No need to say how many messages, as the [MarkAsUnread] widget
103+
// should follow along.
104+
// TODO: Ideally we'd have a progress widget here that showed up based
105+
// on actual time elapsed -- so it could appear before the first
106+
// batch returns, if that takes a while -- and that then stuck
107+
// around continuously until the task ends. For now we use a
108+
// series of [SnackBar]s, which may feel a bit janky.
109+
// There is complexity in tracking the status of each [SnackBar],
110+
// due to having no way to determine which is currently active,
111+
// or if there is an active one at all. Resetting the [SnackBar] here
112+
// results in the same message popping in and out and the user experience
113+
// is better for now if we allow them to run their timer through
114+
// and clear the backlog later.
115+
scaffoldMessenger.showSnackBar(SnackBar(behavior: SnackBarBehavior.floating,
116+
content: Text(zulipLocalizations.markAsReadInProgress)));
117+
}
118+
} catch (e) {
119+
if (!context.mounted) return false;
120+
final zulipLocalizations = ZulipLocalizations.of(context);
121+
showErrorDialog(context: context,
122+
title: zulipLocalizations.errorMarkAsReadFailedTitle,
123+
message: e.toString()); // TODO(#741): extract user-facing message better
124+
return false;
116125
}
117126
}
118127

‎lib/widgets/message_list.dart

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import 'actions.dart';
1616
import 'app_bar.dart';
1717
import 'compose_box.dart';
1818
import 'content.dart';
19-
import 'dialog.dart';
2019
import 'emoji_reaction.dart';
2120
import 'icons.dart';
2221
import 'page.dart';
@@ -714,19 +713,9 @@ class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
714713
final connection = store.connection;
715714
final useLegacy = connection.zulipFeatureLevel! < 155;
716715
setState(() => _loading = true);
717-
bool didPass = false;
718-
try {
719-
didPass = await markNarrowAsRead(context, widget.narrow, useLegacy);
720-
} catch (e) {
721-
if (!context.mounted) return;
722-
final zulipLocalizations = ZulipLocalizations.of(context);
723-
showErrorDialog(context: context,
724-
title: zulipLocalizations.errorMarkAsReadFailedTitle,
725-
message: e.toString()); // TODO(#741): extract user-facing message better
726-
return;
727-
} finally {
728-
setState(() => _loading = false);
729-
}
716+
717+
final didPass = await markNarrowAsRead(context, widget.narrow, useLegacy);
718+
setState(() => _loading = false);
730719
if (!didPass) return;
731720
if (!context.mounted) return;
732721
if (widget.narrow is CombinedFeedNarrow && !useLegacy) {

‎test/widgets/actions_test.dart

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,6 @@ void main() {
290290
checkErrorDialog(tester,
291291
expectedTitle: zulipLocalizations.errorMarkAsReadFailedTitle,
292292
expectedMessage: 'NetworkException: Oops (ClientException: Oops)');
293-
}, skip: true, // TODO move this functionality inside markNarrowAsRead
294-
);
293+
});
295294
});
296295
}

0 commit comments

Comments
 (0)