Skip to content

Commit

Permalink
message view: Don't mark selected message as read when narrowing.
Browse files Browse the repository at this point in the history
For narrows where we select the first unread message,
on activation we have to select it. And on our initial
selection was also `force_rerender` the message list
which causes the message to get reselected.

When `MessageList.select_id` is called for the first time,
`previously_selected_id` is -1. However, because the rerender
function call is before the event trigger, `select_id` is
called again this time with `previously_selected_id` have the
same id as the to be selected message and this selection event
gets triggered before our first time's selection.

This above case only occurs only for narrow activation or
when a message is selected again by clicking on it (message
is already read for this case).

For the All Messages view we have similar logic where, since
opts does not contain `force_rerender` we pass `mark_read`
as false (which was previously unused) which skips the
"message_selected.zulip" event in `message_scroll.js`.

Fixes zulip#10886.
  • Loading branch information
ryanreh99 committed Sep 13, 2020
1 parent 29e6595 commit 4b4e683
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 2 deletions.
39 changes: 38 additions & 1 deletion frontend_tests/node_tests/message_list.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,46 @@ run_test("basics", () => {

assert.deepEqual(list.all_messages(), messages);

global.$.Event = function (ev) {
let message_select_events = [];
global.$.Event = function (ev, opts) {
assert.equal(ev, "message_selected.zulip");
message_select_events.push(opts);
};

assert.equal(list.selected_id(), -1);
global.with_stub((stub) => {
list.view.clear_table = stub.f;
// When narrow initially renders in narrow.update_selection.
list.select_id(60, {
use_closest: true,
force_rerender: true,
});
});

const expected_message_select_events = [
{
id: 60,
previously_selected_id: 60,
},
{
id: 60,
previously_selected_id: -1,
},
];
const actual_message_select_events = message_select_events.map((e) => ({
id: e.id,
previously_selected_id: e.previously_selected_id,
}));

// The render function is called which inturn calls selected_id,
// before the original message selection event takes place.
// Thus the event which contains the updated data is actually
// called first, instead of the original selected_id function call.
assert.equal(list.selected_id(), 60);
assert.equal(message_select_events.length, 2);
assert.deepEqual(actual_message_select_events, expected_message_select_events);
message_select_events = [];

list.select_id(50);

assert.equal(list.selected_id(), 50);
Expand Down
6 changes: 5 additions & 1 deletion static/js/message_scroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,12 @@ exports.initialize = function () {
if (event.id === -1) {
return;
}
if (!event.mark_read || event.id === event.previously_selected_id) {
// Do not mark selected message read on initial narrow load.
return;
}

if (event.mark_read && event.previously_selected_id !== -1) {
if (event.previously_selected_id !== -1) {
// Mark messages between old pointer and new pointer as read
let messages;
if (event.id < event.previously_selected_id) {
Expand Down
1 change: 1 addition & 0 deletions static/js/narrow.js
Original file line number Diff line number Diff line change
Expand Up @@ -821,6 +821,7 @@ exports.deactivate = function () {
then_scroll: true,
use_closest: true,
empty_ok: true,
mark_read: false,
};

// We fall back to the closest selected id, if the user has removed a
Expand Down

0 comments on commit 4b4e683

Please sign in to comment.