From 6ec40cf9a0ac32407013e2f9f6a337082e68e249 Mon Sep 17 00:00:00 2001 From: Mohit Gupta Date: Wed, 10 Jul 2019 05:33:41 +0530 Subject: [PATCH] search: Don't mark messages as read in search narrow. Don't mark unread messages as read while searching. This behavior will be extended to other narrows later. Fixes: #12556. --- docs/subsystems/pointer.md | 4 ++++ frontend_tests/node_tests/compose_actions.js | 3 +++ frontend_tests/node_tests/filter.js | 2 ++ frontend_tests/node_tests/general.js | 10 ++++++++++ frontend_tests/node_tests/message_flags.js | 5 ++++- frontend_tests/node_tests/message_list_data.js | 2 +- static/js/compose_actions.js | 4 +++- static/js/filter.js | 4 ++++ static/js/message_flags.js | 4 ++++ static/js/message_list.js | 4 ++++ static/js/message_list_data.js | 4 +++- static/js/narrow.js | 10 ++++++++++ static/js/navigate.js | 12 +++++++++--- static/js/pointer.js | 4 +++- static/js/unread_ops.js | 4 ++-- 15 files changed, 66 insertions(+), 10 deletions(-) diff --git a/docs/subsystems/pointer.md b/docs/subsystems/pointer.md index 25a64c594b..a53392c125 100644 --- a/docs/subsystems/pointer.md +++ b/docs/subsystems/pointer.md @@ -99,6 +99,10 @@ These two simple rules, combined with the pointer logic above, end up matching user expectations well for whether the product should treat them as having read a set of messages (or not). +One key detail to highlight is that we only mark messages as read +through these processes in views that contain all messages in a +thread; search views will never mark messages as read. + ## Testing and development In a Zulip development environment, you can use `manage.py diff --git a/frontend_tests/node_tests/compose_actions.js b/frontend_tests/node_tests/compose_actions.js index 9da81dfc79..2e7819768c 100644 --- a/frontend_tests/node_tests/compose_actions.js +++ b/frontend_tests/node_tests/compose_actions.js @@ -82,6 +82,9 @@ function stub_selected_message(msg) { selected_message: function () { return msg; }, + can_mark_messages_read: function () { + return true; + }, }); } diff --git a/frontend_tests/node_tests/filter.js b/frontend_tests/node_tests/filter.js index 3f2350c336..e3076e3ca5 100644 --- a/frontend_tests/node_tests/filter.js +++ b/frontend_tests/node_tests/filter.js @@ -68,6 +68,7 @@ run_test('basics', () => { assert(!filter.has_operand('stream', 'nada')); assert(!filter.is_search()); + assert(filter.can_mark_messages_read()); assert(filter.can_apply_locally()); operators = [ @@ -78,6 +79,7 @@ run_test('basics', () => { filter = new Filter(operators); assert(filter.is_search()); + assert(!filter.can_mark_messages_read()); assert(!filter.can_apply_locally()); assert(!filter.is_exactly('stream')); diff --git a/frontend_tests/node_tests/general.js b/frontend_tests/node_tests/general.js index 81e6494ac0..21ccbcefed 100644 --- a/frontend_tests/node_tests/general.js +++ b/frontend_tests/node_tests/general.js @@ -527,8 +527,11 @@ run_test('unread_ops', () => { // Make us not be in a narrow (somewhat hackily). message_list.narrowed = undefined; + // Set current_message_list containing messages that + // can be marked read set_global('current_msg_list', { all_messages: () => test_messages, + can_mark_messages_read: () => true, }); // Ignore these interactions for now: @@ -544,6 +547,12 @@ run_test('unread_ops', () => { channel_post_opts = opts; }; + // First, test for a message list that cannot read messages + current_msg_list.can_mark_messages_read = () => false; + unread_ops.process_visible(); + assert.deepEqual(channel_post_opts, undefined); + + current_msg_list.can_mark_messages_read = () => true; // Do the main thing we're testing! unread_ops.process_visible(); @@ -556,6 +565,7 @@ run_test('unread_ops', () => { data: { messages: '[50]', op: 'add', flag: 'read' }, success: channel_post_opts.success, }); + }); /* diff --git a/frontend_tests/node_tests/message_flags.js b/frontend_tests/node_tests/message_flags.js index 9a251727b0..e7992f57c5 100644 --- a/frontend_tests/node_tests/message_flags.js +++ b/frontend_tests/node_tests/message_flags.js @@ -13,7 +13,10 @@ run_test('starred', () => { const message = { id: 50, }; - + set_global('current_msg_list', { + all_messages: () => [message], + is_search: () => false, + }); var ui_updated; ui.update_starred_view = () => { diff --git a/frontend_tests/node_tests/message_list_data.js b/frontend_tests/node_tests/message_list_data.js index 23282bed0e..e2bb5c8b87 100644 --- a/frontend_tests/node_tests/message_list_data.js +++ b/frontend_tests/node_tests/message_list_data.js @@ -37,7 +37,7 @@ run_test('basics', () => { }); assert.equal(mld.is_search(), false); - + assert(mld.can_mark_messages_read()); mld.add_anywhere(make_msgs([35, 25, 15, 45])); assert_contents(mld, [15, 25, 35, 45]); diff --git a/static/js/compose_actions.js b/static/js/compose_actions.js index bb8a2edaef..7cc605ddc7 100644 --- a/static/js/compose_actions.js +++ b/static/js/compose_actions.js @@ -301,7 +301,9 @@ exports.respond_to_message = function (opts) { return; } - unread_ops.notify_server_message_read(message); + if (current_msg_list.can_mark_messages_read()) { + unread_ops.notify_server_message_read(message); + } var stream = ''; var topic = ''; diff --git a/static/js/filter.js b/static/js/filter.js index c9b7d2a4e2..016236df07 100644 --- a/static/js/filter.js +++ b/static/js/filter.js @@ -375,6 +375,10 @@ Filter.prototype = { return this.has_operator('search'); }, + can_mark_messages_read: function () { + return !this.has_operator('search'); + }, + can_apply_locally: function () { if (this.is_search()) { // The semantics for matching keywords are implemented diff --git a/static/js/message_flags.js b/static/js/message_flags.js index 3cbf9c428b..8ba01d6776 100644 --- a/static/js/message_flags.js +++ b/static/js/message_flags.js @@ -101,6 +101,10 @@ exports.toggle_starred_and_update_server = function (message) { message.starred = !message.starred; + // Unlike most calls to mark messages as read, we don't check + // msg_list.can_mark_messages_read, because starring a message is an + // explicit interaction and we'd like to preserve the user + // expectation invariant that all starred messages are read. unread_ops.notify_server_message_read(message); ui.update_starred_view(message.id, message.starred); diff --git a/static/js/message_list.js b/static/js/message_list.js index 539a91da81..8804b80477 100644 --- a/static/js/message_list.js +++ b/static/js/message_list.js @@ -122,6 +122,10 @@ exports.MessageList.prototype = { return this.data.is_search(); }, + can_mark_messages_read: function () { + return this.data.can_mark_messages_read(); + }, + clear: function MessageList_clear(opts) { opts = _.extend({clear_selected_id: true}, opts); diff --git a/static/js/message_list_data.js b/static/js/message_list_data.js index e7a73022f6..9c07d1ee60 100644 --- a/static/js/message_list_data.js +++ b/static/js/message_list_data.js @@ -142,7 +142,9 @@ MessageListData.prototype = { is_search: function () { return this.filter.is_search(); }, - + can_mark_messages_read: function () { + return this.filter.can_mark_messages_read(); + }, _get_predicate: function () { // We cache this. if (!this.predicate) { diff --git a/static/js/narrow.js b/static/js/narrow.js index 694cd423ea..e067cc85ba 100644 --- a/static/js/narrow.js +++ b/static/js/narrow.js @@ -592,7 +592,12 @@ exports.by_topic = function (target_id, opts) { exports.by_recipient(target_id, opts); return; } + + // We don't check msg_list.can_mark_messages_read here only because + // the target msg_list isn't initialized yet; in any case, the + // message is about to be marked read in the new view. unread_ops.notify_server_message_read(original); + var search_terms = [ {operator: 'stream', operand: original.stream}, {operator: 'topic', operand: util.get_message_topic(original)}, @@ -606,7 +611,12 @@ exports.by_recipient = function (target_id, opts) { opts = _.defaults({}, opts, {then_select_id: target_id}); // don't use current_msg_list as it won't work for muted messages or for out-of-narrow links var message = message_store.get(target_id); + + // We don't check msg_list.can_mark_messages_read here only because + // the target msg_list isn't initialized yet; in any case, the + // message is about to be marked read in the new view. unread_ops.notify_server_message_read(message); + switch (message.type) { case 'private': exports.by('pm-with', message.reply_to, opts); diff --git a/static/js/navigate.js b/static/js/navigate.js index 6b3cecb1d0..42425f7850 100644 --- a/static/js/navigate.js +++ b/static/js/navigate.js @@ -28,7 +28,9 @@ exports.down = function (with_centering) { var current_msg_table = rows.get_table(current_msg_list.table_name); message_viewport.scrollTop(current_msg_table.safeOuterHeight(true) - message_viewport.height() * 0.1); - unread_ops.mark_current_list_as_read(); + if (current_msg_list.can_mark_messages_read()) { + unread_ops.mark_current_list_as_read(); + } } return; @@ -54,7 +56,9 @@ exports.to_end = function () { message_viewport.set_last_movement_direction(1); current_msg_list.select_id(next_id, {then_scroll: true, from_scroll: true}); - unread_ops.mark_current_list_as_read(); + if (current_msg_list.can_mark_messages_read()) { + unread_ops.mark_current_list_as_read(); + } }; function amount_to_paginate() { @@ -114,7 +118,9 @@ exports.page_up = function () { exports.page_down = function () { if (message_viewport.at_bottom() && !current_msg_list.empty()) { current_msg_list.select_id(current_msg_list.last().id, {then_scroll: false}); - unread_ops.mark_current_list_as_read(); + if (current_msg_list.can_mark_messages_read()) { + unread_ops.mark_current_list_as_read(); + } } else { exports.page_down_the_right_amount(); } diff --git a/static/js/pointer.js b/static/js/pointer.js index 688a80674f..26e21ff709 100644 --- a/static/js/pointer.js +++ b/static/js/pointer.js @@ -124,7 +124,9 @@ exports.initialize = function initialize() { } else { messages = event.msg_list.message_range(event.previously_selected, event.id); } - unread_ops.notify_server_messages_read(messages, {from: 'pointer'}); + if (event.msg_list.can_mark_messages_read()) { + unread_ops.notify_server_messages_read(messages, {from: 'pointer'}); + } } }); }; diff --git a/static/js/unread_ops.js b/static/js/unread_ops.js index 8402414aaa..510beb47ff 100644 --- a/static/js/unread_ops.js +++ b/static/js/unread_ops.js @@ -60,7 +60,6 @@ exports.process_read_messages_event = function (message_ids) { // Skips any messages that are already marked as read. exports.notify_server_messages_read = function (messages, options) { options = options || {}; - messages = unread.get_unread_messages(messages); if (messages.length === 0) { return; @@ -91,7 +90,8 @@ exports.process_visible = function () { return; } - if (message_viewport.bottom_message_visible()) { + if (message_viewport.bottom_message_visible() && + current_msg_list.can_mark_messages_read()) { exports.mark_current_list_as_read(); } };