From e3f22a990755c357a4359a77d6d2de26b228fb18 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Fri, 14 Oct 2022 15:37:47 +0000 Subject: [PATCH] recent_topics: Add mention indicator in row for unread topics. Fixes #22984 Add an `@` icon in unread topics where user is mentioned. We track a new set of `stream_id:topic` pairs for the unread mentions so that recent topics instantly knows if a topic is unread and mentioned or not. --- frontend_tests/node_tests/example5.js | 1 + frontend_tests/node_tests/example7.js | 1 + frontend_tests/node_tests/recent_topics.js | 2 + .../node_tests/stream_topic_history.js | 2 + frontend_tests/node_tests/topic_list_data.js | 8 ++ frontend_tests/node_tests/unread.js | 9 +- static/js/message_events.js | 5 +- static/js/recent_topics_ui.js | 4 + static/js/unread.js | 88 ++++++++++++++++++- static/styles/recent_topics.css | 8 ++ static/templates/recent_topic_row.hbs | 1 + tools/test-js-with-node | 1 + 12 files changed, 125 insertions(+), 5 deletions(-) diff --git a/frontend_tests/node_tests/example5.js b/frontend_tests/node_tests/example5.js index 5f8117016c..0c2bb3ef28 100644 --- a/frontend_tests/node_tests/example5.js +++ b/frontend_tests/node_tests/example5.js @@ -84,6 +84,7 @@ run_test("insert_message", ({override}) => { sender_id: isaac.user_id, id: 1001, content: "example content", + topic: "Foo", }; assert.equal(message_store.get(new_message.id), undefined); diff --git a/frontend_tests/node_tests/example7.js b/frontend_tests/node_tests/example7.js index e51a755e26..342a615fbf 100644 --- a/frontend_tests/node_tests/example7.js +++ b/frontend_tests/node_tests/example7.js @@ -94,6 +94,7 @@ run_test("unread_ops", ({override}) => { // Make our test message appear to be unread, so that // we then need to subsequently process them as read. + message_store.update_message_cache(test_messages[0]); unread.process_loaded_messages(test_messages); // Make our message_viewport appear visible. diff --git a/frontend_tests/node_tests/recent_topics.js b/frontend_tests/node_tests/recent_topics.js index 77e9507559..c04a649998 100644 --- a/frontend_tests/node_tests/recent_topics.js +++ b/frontend_tests/node_tests/recent_topics.js @@ -150,6 +150,7 @@ mock_esm("../../static/js/unread", { } return 1; }, + topic_has_any_unread_mentions: () => false, }); const {all_messages_data} = zrequire("all_messages_data"); @@ -293,6 +294,7 @@ function generate_topic_data(topic_info_array) { conversation_key: get_topic_key(stream_id, topic), topic_url: "https://www.example.com", unread_count, + mention_in_unread: false, muted, topic_muted: muted, participated, diff --git a/frontend_tests/node_tests/stream_topic_history.js b/frontend_tests/node_tests/stream_topic_history.js index fd07b94d80..a8f7e3438a 100644 --- a/frontend_tests/node_tests/stream_topic_history.js +++ b/frontend_tests/node_tests/stream_topic_history.js @@ -10,6 +10,7 @@ const message_util = mock_esm("../../static/js/message_util"); const all_messages_data = zrequire("all_messages_data"); const unread = zrequire("unread"); +const message_store = zrequire("message_store"); const stream_data = zrequire("stream_data"); const stream_topic_history = zrequire("stream_topic_history"); const stream_topic_history_util = zrequire("stream_topic_history_util"); @@ -266,6 +267,7 @@ test("test_unread_logic", () => { msg.type = "stream"; msg.stream_id = stream_id; msg.unread = true; + message_store.update_message_cache(msg); } unread.process_loaded_messages(msgs); diff --git a/frontend_tests/node_tests/topic_list_data.js b/frontend_tests/node_tests/topic_list_data.js index 4968fba0e9..ffb47d7d48 100644 --- a/frontend_tests/node_tests/topic_list_data.js +++ b/frontend_tests/node_tests/topic_list_data.js @@ -7,6 +7,14 @@ const _ = require("lodash"); const {mock_esm, zrequire} = require("../zjsunit/namespace"); const {run_test} = require("../zjsunit/test"); +mock_esm("../../static/js/message_store", { + get() { + return { + stream_id: 556, + topic: "general", + }; + }, +}); const user_topics = mock_esm("../../static/js/user_topics", { is_topic_muted() { return false; diff --git a/frontend_tests/node_tests/unread.js b/frontend_tests/node_tests/unread.js index 6da0c6dae3..777e3838d1 100644 --- a/frontend_tests/node_tests/unread.js +++ b/frontend_tests/node_tests/unread.js @@ -4,12 +4,13 @@ const {strict: assert} = require("assert"); const _ = require("lodash"); -const {zrequire} = require("../zjsunit/namespace"); +const {zrequire, set_global} = require("../zjsunit/namespace"); const {run_test} = require("../zjsunit/test"); const {page_params, user_settings} = require("../zjsunit/zpage_params"); page_params.realm_push_notifications_enabled = false; +set_global("document", "document-stub"); const {FoldDict} = zrequire("fold_dict"); const message_store = zrequire("message_store"); const user_topics = zrequire("user_topics"); @@ -98,6 +99,8 @@ test("changing_topics", () => { topic: "lunCH", unread: true, }; + message_store.update_message_cache(message); + message_store.update_message_cache(other_message); assert.deepEqual(unread.get_read_message_ids([15, 16]), [15, 16]); assert.deepEqual(unread.get_unread_message_ids([15, 16]), []); @@ -266,6 +269,7 @@ test("num_unread_for_topic", () => { let i; for (i = num_msgs; i > 0; i -= 1) { message.id = i; + message_store.update_message_cache(message); unread.process_loaded_messages([message]); } @@ -363,7 +367,7 @@ test("phantom_messages", () => { stream_id: 555, topic: "phantom", }; - + message_store.update_message_cache(message); unread.mark_as_read(message.id); const counts = unread.get_counts(); assert.equal(counts.home_unread_messages, 0); @@ -557,6 +561,7 @@ test("mention updates", () => { id: 17, unread: false, type: "stream", + topic: "hello", }; function test_counted(counted) { diff --git a/static/js/message_events.js b/static/js/message_events.js index 7b2f7a4210..4387cf213e 100644 --- a/static/js/message_events.js +++ b/static/js/message_events.js @@ -183,8 +183,6 @@ export function update_messages(events) { message_store.update_booleans(msg, event.flags); - unread.update_message_for_mention(msg); - condense.un_cache_message_content_height(msg.id); if (event.rendered_content !== undefined) { @@ -228,6 +226,8 @@ export function update_messages(events) { msg.raw_content = event.content; } + unread.update_message_for_mention(msg, any_message_content_edited); + // new_topic will be undefined if the topic is unchanged. const new_topic = util.get_edit_event_topic(event); // new_stream_id will be undefined if the stream is unchanged. @@ -489,6 +489,7 @@ export function update_messages(events) { new_stream_id: post_edit_stream_id, new_topic: post_edit_topic, }); + unread.clear_and_populate_unread_mention_topics(); recent_topics_ui.process_topic_edit(...args); } diff --git a/static/js/recent_topics_ui.js b/static/js/recent_topics_ui.js index 705d237a62..f729040ac4 100644 --- a/static/js/recent_topics_ui.js +++ b/static/js/recent_topics_ui.js @@ -343,6 +343,10 @@ function format_conversation(conversation_data) { context.topic_muted = Boolean(user_topics.is_topic_muted(context.stream_id, context.topic)); const stream_muted = stream_data.is_muted(context.stream_id); context.muted = context.topic_muted || stream_muted; + context.mention_in_unread = unread.topic_has_any_unread_mentions( + context.stream_id, + context.topic, + ); // Display in most recent sender first order all_senders = recent_senders.get_topic_recent_senders(context.stream_id, context.topic); diff --git a/static/js/unread.js b/static/js/unread.js index 28f8651497..c5ad3480ec 100644 --- a/static/js/unread.js +++ b/static/js/unread.js @@ -2,6 +2,8 @@ import {FoldDict} from "./fold_dict"; import * as message_store from "./message_store"; import {page_params} from "./page_params"; import * as people from "./people"; +import * as recent_topics_ui from "./recent_topics_ui"; +import * as recent_topics_util from "./recent_topics_util"; import * as settings_config from "./settings_config"; import * as stream_data from "./stream_data"; import * as sub_store from "./sub_store"; @@ -30,6 +32,38 @@ export function set_messages_read_in_narrow(value) { export const unread_mentions_counter = new Set(); const unread_messages = new Set(); +// Map with keys of the form "{stream_id}:{topic.toLowerCase()}" and +// values being Sets of message IDs for unread messages mentioning the +// user within that topic. Use `recent_topics_util.get_topic_key` to +// calculate keys. +// +// Functionally a cache; see clear_and_populate_unread_mention_topics +// for how we can refresh it efficiently. +export const unread_mention_topics = new Map(); + +function add_message_to_unread_mention_topics(message_id) { + const message = message_store.get(message_id); + if (message.type !== "stream") { + return; + } + const topic_key = recent_topics_util.get_topic_key(message.stream_id, message.topic); + if (unread_mention_topics.has(topic_key)) { + unread_mention_topics.get(topic_key).add(message_id); + } + unread_mention_topics.set(topic_key, new Set([message_id])); +} + +function remove_message_from_unread_mention_topics(message_id) { + const message = message_store.get(message_id); + if (message.type !== "stream") { + return; + } + const topic_key = recent_topics_util.get_topic_key(message.stream_id, message.topic); + if (unread_mention_topics.has(topic_key)) { + unread_mention_topics.get(topic_key).delete(message_id); + } +} + class Bucketer { // Maps item_id => bucket_key for items present in a bucket. reverse_lookup = new Map(); @@ -433,6 +467,36 @@ class UnreadTopicCounter { } const unread_topic_counter = new UnreadTopicCounter(); +export function clear_and_populate_unread_mention_topics() { + // The unread_mention_topics is an important data structure for + // efficiently querying whether a given stream/topic pair contains + // unread mentions. + // + // It is effectively a cache, since it can be reconstructed from + // unread_mentions_counter (IDs for all unread mentions) and + // unread_topic_counter (Streams/topics for all unread messages). + // + // Since this function runs in O(unread mentions) time, we can use + // it in topic editing code paths where it might be onerous to + // write custom live-update code; but we should avoid calling it + // in loops. + unread_mention_topics.clear(); + + for (const message_id of unread_mentions_counter) { + const stream_id = unread_topic_counter.bucketer.reverse_lookup.get(message_id); + if (!stream_id) { + continue; + } + const per_stream_bucketer = unread_topic_counter.bucketer.get_bucket(stream_id); + const topic = per_stream_bucketer.reverse_lookup.get(message_id); + const topic_key = recent_topics_util.get_topic_key(stream_id, topic); + if (unread_mention_topics.has(topic_key)) { + unread_mention_topics.get(topic_key).add(message_id); + } + unread_mention_topics.set(topic_key, new Set([message_id])); + } +} + export function message_unread(message) { if (message === undefined) { return false; @@ -518,9 +582,10 @@ export function process_unread_message(message) { update_message_for_mention(message); } -export function update_message_for_mention(message) { +export function update_message_for_mention(message, content_edited = false) { if (!message.unread) { unread_mentions_counter.delete(message.id); + remove_message_from_unread_mention_topics(message.id); return; } @@ -531,8 +596,17 @@ export function update_message_for_mention(message) { if (is_unmuted_mention || message.mentioned_me_directly) { unread_mentions_counter.add(message.id); + add_message_to_unread_mention_topics(message.id); } else { unread_mentions_counter.delete(message.id); + remove_message_from_unread_mention_topics(message.id); + } + + if (content_edited && message.type === "stream") { + // We only need to update recent topics here if this was a content change in an unread + // mention, since in other cases recent topics gets rerendered by other functions. + const topic_key = recent_topics_util.get_topic_key(message.stream_id, message.topic); + recent_topics_ui.inplace_rerender(topic_key); } } @@ -544,6 +618,7 @@ export function mark_as_read(message_id) { unread_topic_counter.delete(message_id); unread_mentions_counter.delete(message_id); unread_messages.delete(message_id); + remove_message_from_unread_mention_topics(message_id); const message = message_store.get(message_id); if (message) { @@ -556,6 +631,7 @@ export function declare_bankruptcy() { unread_topic_counter.clear(); unread_mentions_counter.clear(); unread_messages.clear(); + unread_mention_topics.clear(); } export function get_counts() { @@ -619,10 +695,19 @@ export function num_unread_for_topic(stream_id, topic_name) { } export function stream_has_any_unread_mentions(stream_id) { + // This function is somewhat inefficient and thus should not be + // called in loops, since runs in O(total unread mentions) time. const streams_with_mentions = unread_topic_counter.get_streams_with_unread_mentions(); return streams_with_mentions.has(stream_id); } +export function topic_has_any_unread_mentions(stream_id, topic) { + // Because this function is called in a loop for every displayed + // Recent Topics row, it's important for it to run in O(1) time. + const topic_key = stream_id + ":" + topic.toLowerCase(); + return unread_mention_topics.get(topic_key) && unread_mention_topics.get(topic_key).size > 0; +} + export function topic_has_any_unread(stream_id, topic) { return unread_topic_counter.topic_has_any_unread(stream_id, topic); } @@ -685,6 +770,7 @@ export function initialize() { for (const message_id of unread_msgs.mentions) { unread_mentions_counter.add(message_id); } + clear_and_populate_unread_mention_topics(); for (const obj of unread_msgs.huddles) { for (const message_id of obj.unread_message_ids) { diff --git a/static/styles/recent_topics.css b/static/styles/recent_topics.css index 4ba33f9bdf..e267268859 100644 --- a/static/styles/recent_topics.css +++ b/static/styles/recent_topics.css @@ -170,6 +170,10 @@ opacity: 0.7; } + .unread_mention_info:not(:empty) { + margin-right: -5px; + } + .unread_hidden { visibility: hidden; } @@ -209,6 +213,10 @@ flex-flow: row nowrap; } + .mention_in_unread { + opacity: 0.7; + } + .recent_topic_actions .recent_topics_focusable { display: flex; align-items: center; diff --git a/static/templates/recent_topic_row.hbs b/static/templates/recent_topic_row.hbs index 7425023740..ca85258358 100644 --- a/static/templates/recent_topic_row.hbs +++ b/static/templates/recent_topic_row.hbs @@ -39,6 +39,7 @@ {{#if is_private}} {{unread_count}} {{else}} + @
{{unread_count}} diff --git a/tools/test-js-with-node b/tools/test-js-with-node index b862d384ae..66a1c6ebb4 100755 --- a/tools/test-js-with-node +++ b/tools/test-js-with-node @@ -197,6 +197,7 @@ EXEMPT_FILES = make_set( "static/js/ui_init.js", "static/js/ui_report.ts", "static/js/ui_util.ts", + "static/js/unread.js", "static/js/unread_ops.js", "static/js/unread_ui.js", "static/js/upload_widget.ts",