Skip to content

Commit f2dec7f

Browse files
Khader-1Khader-1
committed
actions [nfc]: Port generic error handling to markNarrowAsRead
Error handling is coupled to `markNarrowAsRead` it self and it is tested as such. Additionally, when factoring out a more generic helper having error handling in that one would help avoiding to copy paste error handling logic and it's tests.
1 parent 1964388 commit f2dec7f

File tree

3 files changed

+99
-99
lines changed

3 files changed

+99
-99
lines changed

‎lib/widgets/actions.dart

Lines changed: 95 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -16,101 +16,112 @@ import '../model/narrow.dart';
1616
import 'dialog.dart';
1717
import 'store.dart';
1818

19-
Future<void> markNarrowAsRead(
19+
/// Returns true if mark as read process is completed successfully.
20+
Future<bool> markNarrowAsRead(
2021
BuildContext context,
2122
Narrow narrow,
2223
bool useLegacy, // TODO(server-6)
2324
) async {
24-
final store = PerAccountStoreWidget.of(context);
25-
final connection = store.connection;
26-
if (useLegacy) {
27-
return await _legacyMarkNarrowAsRead(context, narrow);
28-
}
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+
}
2932

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

42-
// Include `is:unread` in the narrow. That has a database index, so
43-
// this can be an important optimization in narrows with a lot of history.
44-
// The server applies the same optimization within the (deprecated)
45-
// specialized endpoints for marking messages as read; see
46-
// `do_mark_stream_messages_as_read` in `zulip:zerver/actions/message_flags.py`.
47-
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());
4851

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

74-
if (result.foundNewest) {
75-
if (responseCount > 1) {
76-
// We previously showed an in-progress [SnackBar], so say we're done.
77-
// There may be a backlog of [SnackBar]s accumulated in the queue
78-
// so be sure to clear them out here.
79-
scaffoldMessenger
80-
..clearSnackBars()
81-
..showSnackBar(SnackBar(behavior: SnackBarBehavior.floating,
82-
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;
8388
}
84-
return;
85-
}
8689

87-
if (result.lastProcessedId == null) {
88-
// No messages were in the range of the request.
89-
// This should be impossible given that `foundNewest` was false
90-
// (and that our `numAfter` was positive.)
91-
await showErrorDialog(context: context,
92-
title: zulipLocalizations.errorMarkAsReadFailedTitle,
93-
message: zulipLocalizations.errorInvalidResponse);
94-
return;
95-
}
96-
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!);
97100

98-
// The task is taking a while, so tell the user we're working on it.
99-
// No need to say how many messages, as the [MarkAsUnread] widget
100-
// should follow along.
101-
// TODO: Ideally we'd have a progress widget here that showed up based
102-
// on actual time elapsed -- so it could appear before the first
103-
// batch returns, if that takes a while -- and that then stuck
104-
// around continuously until the task ends. For now we use a
105-
// series of [SnackBar]s, which may feel a bit janky.
106-
// There is complexity in tracking the status of each [SnackBar],
107-
// due to having no way to determine which is currently active,
108-
// or if there is an active one at all. Resetting the [SnackBar] here
109-
// results in the same message popping in and out and the user experience
110-
// is better for now if we allow them to run their timer through
111-
// and clear the backlog later.
112-
scaffoldMessenger.showSnackBar(SnackBar(behavior: SnackBarBehavior.floating,
113-
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;
114125
}
115126
}
116127

‎lib/widgets/message_list.dart

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import 'action_sheet.dart';
1515
import 'actions.dart';
1616
import 'compose_box.dart';
1717
import 'content.dart';
18-
import 'dialog.dart';
1918
import 'emoji_reaction.dart';
2019
import 'icons.dart';
2120
import 'page.dart';
@@ -695,18 +694,9 @@ class _MarkAsReadWidgetState extends State<MarkAsReadWidget> {
695694
final useLegacy = connection.zulipFeatureLevel! < 155;
696695
setState(() => _loading = true);
697696

698-
try {
699-
await markNarrowAsRead(context, widget.narrow, useLegacy);
700-
} catch (e) {
701-
if (!context.mounted) return;
702-
final zulipLocalizations = ZulipLocalizations.of(context);
703-
showErrorDialog(context: context,
704-
title: zulipLocalizations.errorMarkAsReadFailedTitle,
705-
message: e.toString()); // TODO(#741): extract user-facing message better
706-
return;
707-
} finally {
708-
setState(() => _loading = false);
709-
}
697+
final didPass = await markNarrowAsRead(context, widget.narrow, useLegacy);
698+
setState(() => _loading = false);
699+
if (!didPass) return;
710700
if (!context.mounted) return;
711701
if (widget.narrow is CombinedFeedNarrow && !useLegacy) {
712702
PerAccountStoreWidget.of(context).unreads.handleAllMessagesReadSuccess();

‎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)