Skip to content
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

unread: Indicate which streams and topics have unread @-mentions. #22583

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
13 changes: 12 additions & 1 deletion frontend_tests/node_tests/stream_list.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ page_params.realm_users = [];

// We use this with override.
let num_unread_for_stream;

let stream_has_any_unread_mentions;
const noop = () => {};

mock_esm("../../static/js/narrow_state", {
Expand All @@ -30,6 +30,7 @@ const scroll_util = mock_esm("../../static/js/scroll_util", {
mock_esm("../../static/js/ui", {get_scroll_element: ($element) => $element});
mock_esm("../../static/js/unread", {
num_unread_for_stream: () => num_unread_for_stream,
stream_has_any_unread_mentions: () => stream_has_any_unread_mentions,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I'm going to merge this, but I think it'd be better to change this test to use the real unread.js module; it's a data module, so it should be easy to setup with a bit of data for the relevant streams rather than mocking it.

});

const {Filter} = zrequire("../js/filter");
Expand Down Expand Up @@ -60,39 +61,47 @@ let inactive_subheader_flag = false;
function create_devel_sidebar_row({mock_template}) {
const $devel_count = $.create("devel-count");
const $subscription_block = $.create("devel-block");
const $devel_unread_mention_info = $.create("devel-unread-mention-info");

const $sidebar_row = $("<devel-sidebar-row-stub>");

$sidebar_row.set_find_results(".subscription_block", $subscription_block);
$subscription_block.set_find_results(".unread_count", $devel_count);
$subscription_block.set_find_results(".unread_mention_info", $devel_unread_mention_info);

mock_template("stream_sidebar_row.hbs", false, (data) => {
assert.equal(data.uri, "#narrow/stream/100-devel");
return "<devel-sidebar-row-stub>";
});

num_unread_for_stream = 42;
stream_has_any_unread_mentions = false;
stream_list.create_sidebar_row(devel);
assert.equal($devel_count.text(), "42");
assert.equal($devel_unread_mention_info.text(), "");
}

function create_social_sidebar_row({mock_template}) {
const $social_count = $.create("social-count");
const $subscription_block = $.create("social-block");
const $social_unread_mention_info = $.create("social-unread-mention-info");

const $sidebar_row = $("<social-sidebar-row-stub>");

$sidebar_row.set_find_results(".subscription_block", $subscription_block);
$subscription_block.set_find_results(".unread_count", $social_count);
$subscription_block.set_find_results(".unread_mention_info", $social_unread_mention_info);

mock_template("stream_sidebar_row.hbs", false, (data) => {
assert.equal(data.uri, "#narrow/stream/200-social");
return "<social-sidebar-row-stub>";
});

num_unread_for_stream = 99;
stream_has_any_unread_mentions = true;
stream_list.create_sidebar_row(social);
assert.equal($social_count.text(), "99");
assert.equal($social_unread_mention_info.text(), "@");
}

function create_stream_subheader({mock_template}) {
Expand Down Expand Up @@ -658,8 +667,10 @@ test_ui("rename_stream", ({mock_template}) => {

const $subscription_block = $.create("development-block");
const $unread_count = $.create("development-count");
const $unread_mention_info = $.create("development-unread-mention-info");
$li_stub.set_find_results(".subscription_block", $subscription_block);
$subscription_block.set_find_results(".unread_count", $unread_count);
$subscription_block.set_find_results(".unread_mention_info", $unread_mention_info);

stream_list.rename_stream(sub);
assert.equal($unread_count.text(), "99");
Expand Down
45 changes: 45 additions & 0 deletions frontend_tests/node_tests/unread.js
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,51 @@ test("mention updates", () => {
test_counted(true);
});

test("stream_has_any_unread_mentions", () => {
const muted_stream_id = 401;
user_topics.add_muted_topic(401, "lunch");

const mention_me_message = {
id: 15,
type: "stream",
stream_id: 400,
topic: "lunch",
mentioned: true,
mentioned_me_directly: true,
unread: true,
};

const mention_all_message = {
id: 16,
type: "stream",
stream_id: 400,
topic: "lunch",
mentioned: true,
mentioned_me_directly: false,
unread: true,
};

// This message's stream_id should not be present in `streams_with_mentions`.
const muted_mention_all_message = {
id: 17,
type: "stream",
stream_id: muted_stream_id,
topic: "lunch",
mentioned: true,
mentioned_me_directly: false,
unread: true,
};

unread.process_loaded_messages([
mention_me_message,
mention_all_message,
muted_mention_all_message,
]);

assert.equal(unread.stream_has_any_unread_mentions(400), true);
assert.equal(unread.stream_has_any_unread_mentions(muted_stream_id), false);
});

test("starring", () => {
// We don't need any setup here, because we just hard code
// this to [] in the code.
Expand Down
19 changes: 14 additions & 5 deletions static/js/stream_list.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,17 @@ export let stream_cursor;

let has_scrolled = false;

export function update_count_in_dom($stream_li, count) {
export function update_count_in_dom($stream_li, count, stream_has_any_unread_mention_messages) {
// The subscription_block properly excludes the topic list,
// and it also has sensitive margins related to whether the
// count is there or not.
const $subscription_block = $stream_li.find(".subscription_block");

ui_util.update_unread_count_in_dom($subscription_block, count);
ui_util.update_unread_mention_info_in_dom(
$subscription_block,
stream_has_any_unread_mention_messages,
);

if (count === 0) {
$subscription_block.removeClass("stream-with-count");
Expand Down Expand Up @@ -335,7 +339,10 @@ class StreamSidebarRow {

update_unread_count() {
const count = unread.num_unread_for_stream(this.sub.stream_id);
update_count_in_dom(this.$list_item, count);
const stream_has_any_unread_mention_messages = unread.stream_has_any_unread_mentions(
this.sub.stream_id,
);
update_count_in_dom(this.$list_item, count, stream_has_any_unread_mention_messages);
timabbott marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -374,15 +381,15 @@ export function redraw_stream_privacy(sub) {
$div.html(html);
}

function set_stream_unread_count(stream_id, count) {
function set_stream_unread_count(stream_id, count, stream_has_any_unread_mention_messages) {
const $stream_li = get_stream_li(stream_id);
if (!$stream_li) {
// This can happen for legitimate reasons, but we warn
// just in case.
blueslip.warn("stream id no longer in sidebar: " + stream_id);
return;
}
update_count_in_dom($stream_li, count);
update_count_in_dom($stream_li, count, stream_has_any_unread_mention_messages);
}

export function update_streams_sidebar(force_rerender) {
Expand All @@ -402,7 +409,9 @@ export function update_streams_sidebar(force_rerender) {
export function update_dom_with_unread_counts(counts) {
// counts.stream_count maps streams to counts
for (const [stream_id, count] of counts.stream_count) {
set_stream_unread_count(stream_id, count);
const stream_has_any_unread_mention_messages =
counts.streams_with_mentions.includes(stream_id);
set_stream_unread_count(stream_id, count, stream_has_any_unread_mention_messages);
}
}

Expand Down
15 changes: 15 additions & 0 deletions static/js/ui_util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,21 @@ export function update_unread_count_in_dom($unread_count_elem: JQuery, count: nu
$unread_count_span.text(count);
}

export function update_unread_mention_info_in_dom(
$unread_mention_info_elem: JQuery,
stream_has_any_unread_mention_messages: Boolean,
): void {
const $unread_mention_info_span = $unread_mention_info_elem.find(".unread_mention_info");
if (!stream_has_any_unread_mention_messages) {
$unread_mention_info_span.hide();
$unread_mention_info_span.text("");
return;
}

$unread_mention_info_span.show();
$unread_mention_info_span.text("@");
}

/**
* Parse HTML and return a DocumentFragment.
*
Expand Down
22 changes: 22 additions & 0 deletions static/js/unread.js
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,21 @@ class UnreadTopicCounter {
return util.sorted_ids(ids);
}

get_streams_with_unread_mentions() {
const streams_with_mentions = new Set();
// Collect the set of streams containing at least one mention.
// We can do this efficiently, since unread_mentions_counter
// contains all unread message IDs, and we use stream_ids as
// bucket keys in our outer bucketer.

for (const message_id of unread_mentions_counter) {
const stream_id = this.bucketer.reverse_lookup.get(message_id);
streams_with_mentions.add(stream_id);
}

return streams_with_mentions;
}

timabbott marked this conversation as resolved.
Show resolved Hide resolved
topic_has_any_unread(stream_id, topic) {
const per_stream_bucketer = this.bucketer.get_bucket(stream_id);

Expand Down Expand Up @@ -528,8 +543,10 @@ export function get_counts() {

// This sets stream_count, topic_count, and home_unread_messages
const topic_res = unread_topic_counter.get_counts();
const streams_with_mentions = unread_topic_counter.get_streams_with_unread_mentions();
res.home_unread_messages = topic_res.stream_unread_messages;
res.stream_count = topic_res.stream_count;
res.streams_with_mentions = Array.from(streams_with_mentions);

const pm_res = unread_pm_counter.get_counts();
res.pm_count = pm_res.pm_dict;
Expand Down Expand Up @@ -575,6 +592,11 @@ export function num_unread_for_topic(stream_id, topic_name) {
return unread_topic_counter.get(stream_id, topic_name);
}

export function stream_has_any_unread_mentions(stream_id) {
const streams_with_mentions = unread_topic_counter.get_streams_with_unread_mentions();
return streams_with_mentions.has(stream_id);
}

timabbott marked this conversation as resolved.
Show resolved Hide resolved
export function topic_has_any_unread(stream_id, topic) {
return unread_topic_counter.topic_has_any_unread(stream_id, topic);
}
Expand Down
6 changes: 6 additions & 0 deletions static/styles/app_components.css
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,12 @@ div.overlay {
color: hsl(0, 0%, 100%);
}

.unread_mention_info:not(:empty) {
jai2201 marked this conversation as resolved.
Show resolved Hide resolved
margin-right: 5px;
margin-left: 2px;
opacity: 0.7;
}
jai2201 marked this conversation as resolved.
Show resolved Hide resolved

/* Implement the web app's default-hidden convention for alert
elements. Portico pages lack this CSS and thus show them by
default. */
Expand Down
1 change: 1 addition & 0 deletions static/templates/stream_sidebar_row.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

<a href="{{uri}}" title="{{name}}" class="stream-name">{{name}}</a>

<span class="unread_mention_info"></span>
<span class="unread_count"></span>
</div>
<span class="stream-sidebar-menu-icon hidden-for-spectators"><i class="zulip-icon zulip-icon-ellipsis-v-solid" aria-hidden="true"></i></span>
Expand Down