-
Notifications
You must be signed in to change notification settings - Fork 356
home: Show unread counts in main menu #1997
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import 'store.dart'; | |
| import 'subscription_list.dart'; | ||
| import 'text.dart'; | ||
| import 'theme.dart'; | ||
| import 'unread_count_badge.dart'; | ||
| import 'user.dart'; | ||
|
|
||
| enum _HomePageTab { | ||
|
|
@@ -351,6 +352,8 @@ abstract class _MenuButton extends StatelessWidget { | |
| color: selected ? designVariables.iconSelected : designVariables.icon); | ||
| } | ||
|
|
||
| Widget? buildTrailing(BuildContext context) => null; | ||
|
|
||
| void onPressed(BuildContext context); | ||
|
|
||
| void _handlePress(BuildContext context) { | ||
|
|
@@ -391,6 +394,8 @@ abstract class _MenuButton extends StatelessWidget { | |
| ~WidgetState.pressed: selected ? borderSideSelected : null, | ||
| })); | ||
|
|
||
| final trailing = buildTrailing(context); | ||
|
|
||
| return AnimatedScaleOnTap( | ||
| duration: const Duration(milliseconds: 100), | ||
| scaleEnd: 0.95, | ||
|
|
@@ -407,6 +412,7 @@ abstract class _MenuButton extends StatelessWidget { | |
| overflow: TextOverflow.ellipsis, | ||
| style: const TextStyle(fontSize: 19, height: 26 / 19) | ||
| .merge(weightVariableTextStyle(context, wght: selected ? 600 : 400)))), | ||
| ?trailing, | ||
| ])))); | ||
| } | ||
| } | ||
|
|
@@ -457,6 +463,18 @@ class _InboxButton extends _NavigationBarMenuButton { | |
| return zulipLocalizations.inboxPageTitle; | ||
| } | ||
|
|
||
| @override | ||
| Widget? buildTrailing(BuildContext context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| final unreadCount = store.unreads.countInCombinedFeedNarrow(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I worry a bit about adding this call site given this TODO for #370: Do you have any timings on how long this takes with lots of unreads?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if (unreadCount == 0) return null; | ||
| return UnreadCountBadge( | ||
| style: UnreadCountBadgeStyle.mainMenu, | ||
| count: unreadCount, | ||
| channelIdForBackground: null, | ||
| ); | ||
| } | ||
|
|
||
| @override | ||
| _HomePageTab get navigationTarget => _HomePageTab.inbox; | ||
| } | ||
|
|
@@ -472,6 +490,18 @@ class _MentionsButton extends _MenuButton { | |
| return zulipLocalizations.mentionsPageTitle; | ||
| } | ||
|
|
||
| @override | ||
| Widget? buildTrailing(BuildContext context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| final unreadCount = store.unreads.countInMentionsNarrow(); | ||
| if (unreadCount == 0) return null; | ||
| return UnreadCountBadge( | ||
| style: UnreadCountBadgeStyle.mainMenu, | ||
| count: unreadCount, | ||
| channelIdForBackground: null, | ||
| ); | ||
| } | ||
|
|
||
| @override | ||
| void onPressed(BuildContext context) { | ||
| Navigator.of(context).push(MessageListPage.buildRoute( | ||
|
|
@@ -541,6 +571,18 @@ class _DirectMessagesButton extends _NavigationBarMenuButton { | |
| return zulipLocalizations.recentDmConversationsPageTitle; | ||
| } | ||
|
|
||
| @override | ||
| Widget? buildTrailing(BuildContext context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| final unreadCount = store.unreads.countInAllDms(); | ||
| if (unreadCount == 0) return null; | ||
| return UnreadCountBadge( | ||
| style: UnreadCountBadgeStyle.mainMenu, | ||
| count: unreadCount, | ||
| channelIdForBackground: null, | ||
| ); | ||
| } | ||
|
|
||
| @override | ||
| _HomePageTab get navigationTarget => _HomePageTab.directMessages; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,16 +191,18 @@ void main() { | |
| await store.addSubscription(eg.subscription(stream2)); | ||
| await store.addSubscription(eg.subscription(stream3, isMuted: true)); | ||
| await store.setUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); | ||
| await store.setMutedUsers([eg.thirdUser.userId]); | ||
| fillWithMessages([ | ||
| eg.streamMessage(stream: stream1, topic: 'a', flags: []), | ||
| eg.streamMessage(stream: stream1, topic: 'b', flags: []), | ||
| eg.streamMessage(stream: stream1, topic: 'b', flags: []), | ||
| eg.streamMessage(stream: stream2, topic: 'c', flags: []), | ||
| eg.streamMessage(stream: stream3, topic: 'd', flags: []), | ||
| eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []), | ||
| // Exclude because user is muted | ||
| eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser], flags: []), | ||
|
Comment on lines
+202
to
203
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the muted channel and topic just above don't have comments, so seems clearest not to add one only here |
||
| ]); | ||
| check(model.countInCombinedFeedNarrow()).equals(5); | ||
| check(model.countInCombinedFeedNarrow()).equals(4); | ||
| }); | ||
|
|
||
| test('countInChannel/Narrow', () async { | ||
|
|
@@ -271,6 +273,20 @@ void main() { | |
| ]); | ||
| check(model.countInStarredMessagesNarrow()).equals(0); | ||
| }); | ||
|
|
||
| test('countInAllDms', () async { | ||
| prepare(); | ||
| await store.setMutedUsers([eg.thirdUser.userId]); | ||
| fillWithMessages([ | ||
| // No one is muted: don't exclude | ||
| eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []), | ||
| // Everyone is muted: exclude | ||
| eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser], flags: []), | ||
| // 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the name of the test case, this should be calling |
||
| }); | ||
| }); | ||
|
|
||
| group('isUnread', () { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.