home: Show unread counts in main menu #1997
Conversation
|
cc @alya |
chrisbobbe
added
the
product review
chrisbobbe
force-pushed
the
pr-main-menu-unread-counts
branch
from
109e8dc to
b33421b
Compare
rajveermalviya
left a comment
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! LGTM, moving over to Greg's review.
rajveermalviya
added
integration review
test/model/unreads_test.dart
Outdated
| // One is muted, one isn't: don't exclude | ||
| eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser, eg.otherUser], flags: []), | ||
| ]); | ||
| check(model.countInCombinedFeedNarrow()).equals(2); |
There was a problem hiding this comment.
Given the name of the test case, this should be calling countInAllDms, right?
lib/model/unreads.dart
Outdated
| int countInAllDms() { | ||
| int c = 0; | ||
| for (final MapEntry(key: narrow, value: messageIds) in dms.entries) { | ||
| if (channelStore.shouldMuteDmConversation(narrow)) continue; | ||
| c += messageIds.length; |
There was a problem hiding this comment.
The name feels misleading, given that this isn't actually all DMs. 🙂 A bit like how we moved away from "All messages" to "Combined feed".
How about just countInDms? And maybe dartdoc mentioning it excludes muted DM conversations.
test/model/unreads_test.dart
Outdated
| // Exclude because user is muted | ||
| eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser], flags: []), |
There was a problem hiding this comment.
nit: the muted channel and topic just above don't have comments, so seems clearest not to add one only here
| @override | ||
| Widget? buildTrailing(BuildContext context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| final unreadCount = store.unreads.countInCombinedFeedNarrow(); |
There was a problem hiding this comment.
Hmm, I worry a bit about adding this call site given this TODO for #370:
// TODO(#370): maintain this count incrementally, rather than recomputing from scratch
int countInCombinedFeedNarrow() {
Do you have any timings on how long this takes with lots of unreads?
There was a problem hiding this comment.
Ah I see, this is only in the main-menu bottom sheet, not the bottom tabs. In that case I guess it doesn't make the impact any worse than it is now with the mark-as-read button in the message list.
chrisbobbe
force-pushed
the
pr-main-menu-unread-counts
branch
from
b33421b to
72f172d
Compare
|
Thanks for the review! Revision pushed. |
|
Pushed one new commit: 381b1f6 unreads: In @-mentions count, exclude messages in muted DM conversations |
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks! The initial 3 commits all look good now — small comments below just on the new 4th commit.
Would you also file a quick issue for the bug the new commit is fixing? Doesn't need more than a few words — just helps us track what behavior is changing from one release to the next.
lib/model/unreads.dart
Outdated
| for (final messageId in mentions) { | ||
| final narrow = locatorMap[messageId]; | ||
| if (narrow == null) continue; |
There was a problem hiding this comment.
| for (final messageId in mentions) { | |
| final narrow = locatorMap[messageId]; | |
| if (narrow == null) continue; | |
| for (final messageId in mentions) { | |
| final narrow = locatorMap[messageId]; | |
| if (narrow == null) continue; // TODO(log) |
This case should be impossible, right?
test/model/unreads_test.dart
Outdated
| eg.streamMessage(stream: stream, topic: 'a', flags: [MessageFlag.mentioned]), | ||
| eg.streamMessage(stream: stream, flags: [MessageFlag.wildcardMentioned]), | ||
| eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: [MessageFlag.mentioned]), | ||
| eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser], flags: [MessageFlag.mentioned]), | ||
| ]); | ||
| check(model.countInMentionsNarrow()).equals(2); | ||
| check(model.countInMentionsNarrow()).equals(4); |
There was a problem hiding this comment.
This is probably clearest as a new test case (or two: one showing topic mutes don't matter, one showing user mutes do).
Otherwise, it rapidly gets tough to see at a glance why this result is the right one (vs 3 or 5 or 6). I think in the existing test case, it's clear enough why 2 is right: there are two messages that are mentions, and one that isn't.
chrisbobbe
force-pushed
the
pr-main-menu-unread-counts
branch
from
381b1f6 to
18810a1
Compare
|
Thanks for the review! Revision pushed. |
3 participants
Fixes-partly: #1088
A follow-up to #1959, clearing one of the TODOs on the
UnreadCountBadgewidget.Screenshots: