zulip · syed-rafat · Jun 2, 2023 · Jun 2, 2023
diff --git a/web/src/filter.js b/web/src/filter.js
@@ -500,10 +500,9 @@ export class Filter {
         return true;
     }
 
-    calc_can_mark_messages_read() {
-        // Arguably this should match supports_collapsing_recipients.
-        // We may want to standardize on that in the future.  (At
-        // present, this function does not allow combining valid filters).
+    // these filters work for both
+    // calc_can_mark_messages_read and is_common_narrow
+    core_filters() {
         const term_types = this.sorted_term_types();
 
         if (_.isEqual(term_types, ["stream", "topic"])) {
@@ -514,11 +513,6 @@ export class Filter {
             return true;
         }
 
-        // TODO: Some users really hate it when Zulip marks messages as read
-        // in interleaved views, so we will eventually have a setting
-        // that early-exits before the subsequent checks.
-        // (in which case, is_common_narrow would also need to be modified)
-
         if (_.isEqual(term_types, ["stream"])) {
             return true;
         }
@@ -547,6 +541,48 @@ export class Filter {
         return false;
     }
 
+    calc_can_mark_messages_read() {
+        // Arguably this should match supports_collapsing_recipients.
+        // We may want to standardize on that in the future.  (At
+        // present, this function does not allow combining valid filters).
+
+        if (this.core_filters()) {
+            return true;
+        }
+
+        const term_types = this.sorted_term_types();
+
+        // TODO: Some users really hate it when Zulip marks messages as read
+        // in interleaved views, so we will eventually have a setting
+        // that early-exits before the subsequent checks.
+        // (in which case, is_common_narrow would also need to be modified)
+
+        // -is:dm excludes direct messages from all messages
+        if (_.isEqual(term_types, ["not-is-dm"])) {
+            return true;
+        }
+
+        // Excluding direct messages from stream and topic does not
+        // accomplish anything, still adding code to be consistent with our design.
+
+        if (_.isEqual(term_types, ["stream", "not-is-dm"])) {
+            return true;
+        }
+
+        if (_.isEqual(term_types, ["topic", "not-is-dm"])) {
+            return true;
+        }
+
+        // Excluding direct messages from stream and topic does not
+        // accomplish anything, but we are still letting user mark
+        // mark messages as read to be consistent with our design.
+        if (_.isEqual(term_types, ["stream", "topic", "not-is-dm"])) {
+            return true;
+        }
+
+        return false;
+    }
+
     can_mark_messages_read() {
         if (this._can_mark_messages_read === undefined) {
             this._can_mark_messages_read = this.calc_can_mark_messages_read();
@@ -560,16 +596,12 @@ export class Filter {
     // https://paper.dropbox.com/doc/Navbar-behavior-table--AvnMKN4ogj3k2YF5jTbOiVv_AQ-cNOGtu7kSdtnKBizKXJge
     // common narrows show a narrow description and allow the user to
     // close search bar UI and show the narrow description UI.
-    //
-    // TODO: We likely will want to rewrite this to not piggy-back on
-    // can_mark_messages_read, since that might gain some more complex behavior
-    // with near: narrows.
     is_common_narrow() {
         // can_mark_messages_read tests the following filters:
         // stream, stream + topic,
         // is:dm, dm,
         // is:mentioned, is:resolved
-        if (this.can_mark_messages_read()) {
+        if (this.core_filters()) {
             return true;
         }
         // that leaves us with checking:

diff --git a/web/tests/filter.test.js b/web/tests/filter.test.js
@@ -194,6 +194,52 @@ test("basics", () => {
     assert.ok(!filter.is_personal_filter());
     assert.ok(!filter.is_conversation_view());
 
+    operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.has_operator("stream"));
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_negated_operand("stream", "not-is-dm"));
+    assert.ok(filter.can_apply_locally());
+    assert.ok(!filter.is_personal_filter());
+
+    operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.has_operator("stream"));
+    assert.ok(filter.has_operator("topic"));
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_operator("search"));
+    assert.ok(!filter.has_negated_operand("stream", "foo"));
+    assert.ok(!filter.has_negated_operand("topic", "bar"));
+    assert.ok(filter.can_apply_locally());
+    assert.ok(!filter.is_personal_filter());
+    assert.ok(filter.can_bucket_by("stream"));
+    assert.ok(filter.can_bucket_by("stream", "topic"));
+
+    operators = [
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.has_operator("topic"));
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_operator("search"));
+    assert.ok(!filter.has_negated_operand("topic", "bar"));
+    assert.ok(filter.can_apply_locally());
+    assert.ok(!filter.is_personal_filter());
+
     operators = [{operator: "is", operand: "dm"}];
     filter = new Filter(operators);
     assert.ok(filter.contains_only_private_messages());
@@ -204,6 +250,15 @@ test("basics", () => {
     assert.ok(!filter.is_personal_filter());
     assert.ok(!filter.is_conversation_view());
 
+    operators = [{operator: "is", operand: "dm", negated: true}];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_operator("search"));
+    assert.ok(!filter.is_personal_filter());
+    assert.ok(!filter.is_conversation_view());
+
     // "is:private" was renamed to "is:dm"
     operators = [{operator: "is", operand: "private"}];
     filter = new Filter(operators);
@@ -444,6 +499,37 @@ test("can_mark_messages_read", () => {
     filter = new Filter(stream_negated_topic_operators);
     assert.ok(!filter.can_mark_messages_read());
 
+    const stream_negated_dm_operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(stream_negated_dm_operators);
+    assert.ok(filter.can_mark_messages_read());
+    assert_not_mark_read_with_has_operands(stream_negated_dm_operators);
+    assert_not_mark_read_with_is_operands(stream_negated_dm_operators);
+    assert_not_mark_read_when_searching(stream_negated_dm_operators);
+
+    const stream_topic_negated_dm_operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(stream_topic_negated_dm_operators);
+    assert.ok(filter.can_mark_messages_read());
+    assert_not_mark_read_with_has_operands(stream_topic_negated_dm_operators);
+    assert_not_mark_read_with_is_operands(stream_topic_negated_dm_operators);
+    assert_not_mark_read_when_searching(stream_topic_negated_dm_operators);
+
+    const topic_negated_dm_operators = [
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(topic_negated_dm_operators);
+    assert.ok(filter.can_mark_messages_read());
+    assert_not_mark_read_with_is_operands(topic_negated_dm_operators);
+    assert_not_mark_read_when_searching(topic_negated_dm_operators);
+    assert_not_mark_read_when_searching(topic_negated_dm_operators);
+
     const dm = [{operator: "dm", operand: "joe@example.com,"}];
 
     const dm_negated = [{operator: "dm", operand: "joe@example.com,", negated: true}];
@@ -469,6 +555,10 @@ test("can_mark_messages_read", () => {
     assert_not_mark_read_with_has_operands(is_dm);
     assert_not_mark_read_when_searching(is_dm);
 
+    const not_is_dm = [{operator: "is", operand: "dm", negated: true}];
+    filter = new Filter(not_is_dm);
+    assert.ok(filter.can_mark_messages_read());
+
     const in_all = [{operator: "in", operand: "all"}];
     filter = new Filter(in_all);
     assert.ok(filter.can_mark_messages_read());