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

@@ -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

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;
+        },
     });
 }
 

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'));
 

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,
     });
+
 });
 
 /*

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 = () => {

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]);

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 = '';

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

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);
 

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);
 

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) {

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);

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();
     }

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'});
+            }
         }
     });
 };

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();
     }
 };