Skip to content

unread [nfc]: Add getUnreadCountForTopic #4805

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.

Sign up for GitHub

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

Merged
chrisbobbe merged 4 commits into from
Jun 18, 2021
Merged

unread [nfc]: Add getUnreadCountForTopic #4805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
119ee98
unread [nfc]: Add `getUnread` selector.
gnprice Jun 16, 2021
0b4c2a4
unread [nfc]: Add a getter, getUnreadCountForTopic.
gnprice Jun 16, 2021
f03bf0c
unread [nfc]: Use `getUnread` in `getUnreadCountForNarrow`.
gnprice Jun 16, 2021
6a1744c
unread [nfc]: Reuse `getUnreadCountForTopic` in an unread selector, too.
gnprice Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/topics/topicSelectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type {
TopicsState,
} from '../types';
import { getMute, getStreams, getTopics } from '../directSelectors';
import { getUnreadStreams } from '../unread/unreadModel';
import { getUnread, getUnreadCountForTopic } from '../unread/unreadModel';
import { getShownMessagesForNarrow } from '../chat/narrowsSelectors';
import { getStreamsById } from '../subscriptions/subscriptionSelectors';
import { NULL_ARRAY } from '../nullObjects';
Expand Down Expand Up @@ -44,15 +44,15 @@ export const getTopicsForStream: Selector<?(TopicExtended[]), number> = createSe
(state, streamId) => getTopics(state)[streamId],
state => getMute(state),
(state, streamId) => getStreamsById(state).get(streamId),
state => getUnreadStreams(state),
(topicList, mute, stream, unreadStreams) => {
state => getUnread(state),
(topicList, mute, stream, unread) => {
if (!topicList || !stream) {
return undefined;
}

return topicList.map(({ name, max_id }) => {
const isMuted = !!mute.find(x => x[0] === stream.name && x[1] === name);
const unreadCount = unreadStreams.get(stream.stream_id)?.get(name)?.size ?? 0;
const unreadCount = getUnreadCountForTopic(unread, stream.stream_id, name);
return { name, max_id, isMuted, unreadCount };
});
},
Expand Down
20 changes: 20 additions & 0 deletions src/unread/unreadModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ import {
//
// Selectors.
//
// These take the global state as their input.
//

/** The unread-messages state as a whole. */
export const getUnread = (state: GlobalState): UnreadState => state.unread;

export const getUnreadStreams = (state: GlobalState): UnreadStreamsState => state.unread.streams;

Expand All @@ -37,6 +42,21 @@ export const getUnreadHuddles = (state: GlobalState): UnreadHuddlesState => stat

export const getUnreadMentions = (state: GlobalState): UnreadMentionsState => state.unread.mentions;

//
//
// Getters.
//
// These operate directly on this particular model's state, as part of this
// model's own interface.
//

/** The total number of unreads in the given topic. */
export const getUnreadCountForTopic = (
unread: UnreadState,
Copy link
Copy Markdown
Collaborator

@chrisbobbe chrisbobbe Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine for a selector to take a substate (like UnreadState) as the input state, or should all selectors start with GlobalState?

Copy link
Copy Markdown
Member Author

@gnprice gnprice Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, perhaps calling this a "selector" is confusing and I should label it something else. Maybe "getter"?

The reason I want it taking just the unread state is so that we can conveniently use it in a context like #4635, in code where we have a BackgroundData style of object but don't have the whole Redux state.

More generally, a direction I'm interested in taking our data-model code is toward having each subsystem's model be more of a self-contained thing with its own identity and interface, vs. the status quo which kind of tries (but inconsistently so, because this doesn't really work if you try to push it hard) to treat them all as undifferentiated implementation details of the global state. For example, we'd have the whole "unreads" model as one such subsystem. So having a getter like this directly on that model's state is a small experiment in that direction.

Copy link
Copy Markdown
Collaborator

@chrisbobbe chrisbobbe Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name "selector" is probably fine, and the use case makes sense. Thanks for explaining, and I'm excited to see progress in getting our data-model code structured more sensibly. 🙂

We'd call both rootReducer and messagesReducer "reducers", and I think it's fine to call both this function and those that take GlobalState "selectors".

streamId: number,
topic: string,
): number => unread.streams.get(streamId)?.get(topic)?.size ?? 0;

//
//
// Reducer.
Expand Down
27 changes: 16 additions & 11 deletions src/unread/unreadSelectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ import { isTopicMuted } from '../utils/message';
import { caseNarrow } from '../utils/narrow';
import { NULL_SUBSCRIPTION } from '../nullObjects';
import { pmUnreadsKeyFromPmKeyIds } from '../utils/recipient';
import { getUnreadPms, getUnreadHuddles, getUnreadMentions, getUnreadStreams } from './unreadModel';
import {
getUnread,
getUnreadPms,
getUnreadHuddles,
getUnreadMentions,
getUnreadStreams,
getUnreadCountForTopic,
} from './unreadModel';

/** The number of unreads in each stream, excluding muted topics, by stream ID. */
export const getUnreadByStream: Selector<{| [number]: number |}> = createSelector(
Expand Down Expand Up @@ -217,11 +224,9 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
state => getStreams(state),
state => getOwnUserId(state),
state => getUnreadTotal(state),
state => getUnreadStreams(state),
state => getUnreadHuddles(state),
state => getUnreadPms(state),
state => getUnread(state),
state => getMute(state),
(narrow, streams, ownUserId, unreadTotal, unreadStreams, unreadHuddles, unreadPms, mute) =>
(narrow, streams, ownUserId, unreadTotal, unread, mute) =>
caseNarrow(narrow, {
home: () => unreadTotal,

Expand All @@ -232,7 +237,7 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
}
// prettier-ignore
return (
unreadStreams
unread.streams
.get(stream.stream_id)
?.entrySeq()
.filterNot(([topic, _]) => isTopicMuted(name, topic, mute))
Expand All @@ -247,18 +252,18 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
if (!stream) {
return 0;
}
return unreadStreams.get(stream.stream_id)?.get(topic)?.size ?? 0;
return getUnreadCountForTopic(unread, stream.stream_id, topic);
},

pm: ids => {
if (ids.length > 1) {
const unreadsKey = pmUnreadsKeyFromPmKeyIds(ids, ownUserId);
const unread = unreadHuddles.find(x => x.user_ids_string === unreadsKey);
return unread ? unread.unread_message_ids.length : 0;
const unreadItem = unread.huddles.find(x => x.user_ids_string === unreadsKey);
return unreadItem?.unread_message_ids.length ?? 0;
} else {
const senderId = ids[0];
const unread = unreadPms.find(x => x.sender_id === senderId);
return unread ? unread.unread_message_ids.length : 0;
const unreadItem = unread.pms.find(x => x.sender_id === senderId);
return unreadItem?.unread_message_ids.length ?? 0;
}
},

Expand Down