-
Notifications
You must be signed in to change notification settings - Fork 355
home: Show starred-message count in main menu, subject to user setting #1999
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
a3d7fd8
45bfcb0
248cab2
e8f51b2
247a3d4
a22b49f
34f9a0b
1e74454
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,9 @@ mixin MessageStore on ChannelStore { | |
| /// All known messages, indexed by [Message.id]. | ||
| Map<int, Message> get messages; | ||
|
|
||
| /// All starred messages, as message IDs. | ||
| Set<int> get starredMessages; | ||
|
|
||
| /// [OutboxMessage]s sent by the user, indexed by [OutboxMessage.localMessageId]. | ||
| Map<int, OutboxMessage> get outboxMessages; | ||
|
|
||
|
|
@@ -208,6 +211,8 @@ mixin ProxyMessageStore on MessageStore { | |
| @override | ||
| Map<int, Message> get messages => messageStore.messages; | ||
| @override | ||
| Set<int> get starredMessages => messageStore.starredMessages; | ||
| @override | ||
| Map<int, OutboxMessage> get outboxMessages => messageStore.outboxMessages; | ||
| @override | ||
| void registerMessageList(MessageListView view) => | ||
|
|
@@ -261,14 +266,19 @@ class _EditMessageRequestStatus { | |
| } | ||
|
|
||
| class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessageStore { | ||
| MessageStoreImpl({required super.channels}) | ||
| : // There are no messages in InitialSnapshot, so we don't have | ||
| // a use case for initializing MessageStore with nonempty [messages]. | ||
| messages = {}; | ||
| MessageStoreImpl({ | ||
| required super.channels, | ||
| required List<int> initialStarredMessages, | ||
| }) : | ||
| messages = {}, | ||
| starredMessages = Set.of(initialStarredMessages); | ||
|
|
||
| @override | ||
| final Map<int, Message> messages; | ||
|
|
||
| @override | ||
| final Set<int> starredMessages; | ||
|
|
||
| @override | ||
| final Set<MessageListView> _messageListViews = {}; | ||
|
|
||
|
|
@@ -717,23 +727,29 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage | |
| } | ||
| } | ||
|
|
||
| void handleDeleteMessageEvent(DeleteMessageEvent event) { | ||
| /// Handle a [DeleteMessageEvent] | ||
| /// and return whether the [PerAccountStore] should notify listeners. | ||
| bool handleDeleteMessageEvent(DeleteMessageEvent event) { | ||
| bool perAccountStoreShouldNotify = false; | ||
| for (final messageId in event.messageIds) { | ||
| messages.remove(messageId); | ||
| perAccountStoreShouldNotify |= starredMessages.remove(messageId); | ||
| _maybeStaleChannelMessages.remove(messageId); | ||
| _editMessageRequests.remove(messageId); | ||
| } | ||
| for (final view in _messageListViews) { | ||
| view.handleDeleteMessageEvent(event); | ||
| } | ||
| return perAccountStoreShouldNotify; | ||
| } | ||
|
|
||
| void handleUpdateMessageFlagsEvent(UpdateMessageFlagsEvent event) { | ||
| /// Handle an [UpdateMessageFlagsEvent] | ||
| /// and return whether the [PerAccountStore] should notify listeners. | ||
| bool handleUpdateMessageFlagsEvent(UpdateMessageFlagsEvent event) { | ||
| final isAdd = switch (event) { | ||
| UpdateMessageFlagsAddEvent() => true, | ||
| UpdateMessageFlagsRemoveEvent() => false, | ||
| }; | ||
|
|
||
| if (isAdd && (event as UpdateMessageFlagsAddEvent).all) { | ||
|
Comment on lines
752
to
753
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: these stanzas don't become any more closely related with this change; in fact they become more separate, because this adds another stanza below which consumes |
||
| for (final message in messages.values) { | ||
| message.flags.add(event.flag); | ||
|
|
@@ -766,6 +782,15 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage | |
| _notifyMessageListViews(event.messages); | ||
| } | ||
| } | ||
|
|
||
| if (event.flag == MessageFlag.starred) { | ||
| isAdd | ||
| ? starredMessages.addAll(event.messages) | ||
| : starredMessages.removeAll(event.messages); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| void handleReactionEvent(ReactionEvent event) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -564,7 +564,8 @@ class PerAccountStore extends PerAccountStoreBase with | |
| presence: Presence(realm: realm, | ||
| initial: initialSnapshot.presences), | ||
| channels: channels, | ||
| messages: MessageStoreImpl(channels: channels), | ||
| messages: MessageStoreImpl(channels: channels, | ||
| initialStarredMessages: initialSnapshot.starredMessages), | ||
| unreads: Unreads(core: core, channelStore: channels, | ||
| initial: initialSnapshot.unreadMsgs), | ||
| recentDmConversationsView: RecentDmConversationsView(core: core, | ||
|
|
@@ -776,6 +777,8 @@ class PerAccountStore extends PerAccountStoreBase with | |
| switch (event.property!) { | ||
| case UserSettingName.twentyFourHourTime: | ||
| userSettings.twentyFourHourTime = event.value as TwentyFourHourTimeMode; | ||
| case UserSettingName.starredMessageCounts: | ||
| userSettings.starredMessageCounts = event.value as bool; | ||
| case UserSettingName.displayEmojiReactionUsers: | ||
| userSettings.displayEmojiReactionUsers = event.value as bool; | ||
| case UserSettingName.emojiset: | ||
|
|
@@ -877,12 +880,16 @@ class PerAccountStore extends PerAccountStoreBase with | |
| // specifically, their `senderId`s. By calling this after the | ||
| // aforementioned line, we'll lose reference to those messages. | ||
| recentSenders.handleDeleteMessageEvent(event, messages); | ||
| _messages.handleDeleteMessageEvent(event); | ||
| if (_messages.handleDeleteMessageEvent(event)) { | ||
| notifyListeners(); | ||
| } | ||
| unreads.handleDeleteMessageEvent(event); | ||
|
Comment on lines
+883
to
886
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. It makes me a bit nervous to be calling notifyListeners at a point where the event has been only partially applied — the store is in a somewhat inconsistent state. Instead let's delay the notifyListeners call to the end. |
||
|
|
||
| case UpdateMessageFlagsEvent(): | ||
| assert(debugLog("server event: update_message_flags/${event.op} ${event.flag.toJson()}")); | ||
| _messages.handleUpdateMessageFlagsEvent(event); | ||
| if (_messages.handleUpdateMessageFlagsEvent(event)) { | ||
| notifyListeners(); | ||
| } | ||
| unreads.handleUpdateMessageFlagsEvent(event); | ||
|
|
||
| case SubmessageEvent(): | ||
|
|
||
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.
Hmm, I see.
I'm trying to think if there are other cases where this should get updated. I guess maybe not:
I think it'd be good to make that reasoning explicit, though, in those methods. Probably including asserts to verify those expectations.