Skip to content

Commit

Permalink
narrow: Implement rendering of with narrow operators.
Browse files Browse the repository at this point in the history
Adds server and web app support for processing the new `with`
search operator.

Fixes part of zulip#21505.

Co-authored-by: roanster007 <[email protected]>
  • Loading branch information
amanagr and roanster007 committed Jul 6, 2024
1 parent 0ea162c commit 1be4d9c
Show file tree
Hide file tree
Showing 14 changed files with 587 additions and 21 deletions.
10 changes: 10 additions & 0 deletions api_docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@ format used by the Zulip server that they are interacting with.

## Changes in Zulip 9.0

**Feature level 268**

* [`GET /messages`](/api/get-messages),
[`GET /messages/matches_narrow`](/api/check-messages-match-narrow),
[`POST /messages/flags/narrow`](/api/update-message-flags-for-narrow),
[`POST /register`](/api/register-queue):
Added support for a new [search/narrow filter](/api/construct-narrow),
`with`, which returns messages in the same channel and topic as that
of the operand of filter, with the first unread message as anchor.

**Feature level 267**

* [`GET /invites`](/api/get-invites),[`POST /invites`](/api/send-invites): Added
Expand Down
24 changes: 18 additions & 6 deletions api_docs/construct-narrow.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ important optimization when fetching messages in certain cases (e.g.,
when [adding the `read` flag to a user's personal
messages](/api/update-message-flags-for-narrow)).

**Changes**: In Zulip 9.0 (feature level 265), support was added for a
**Changes**: In Zulip 9.0 (feature level 268), narrows gained support
for a new `with` operator for linking to topics.

In Zulip 9.0 (feature level 265), support was added for a
new `is:followed` filter, matching messages in topics that the current
user is [following](/help/follow-a-topic).

Expand Down Expand Up @@ -88,15 +91,24 @@ filters did.

### Message IDs

The `near` and `id` operators, documented in the help center, use message
IDs for their operands.
The `with` operator is designed to be used in links that should follow
a topic across being moved. Its operand is a message ID.

The `near` and `id` operators, documented in the help center,
also use message IDs for their operands.

* `with:12345`: Search for the conversation (stream/topic pair) which
contains the message with ID `12345`. If such a message exists, and
can be accessed by the user, then the search will be treated as having
the `channel`/`topic`/`dm` operators corresponding to the
conversation containing that message, replacing any such operators
in the original request.
* `near:12345`: Search messages around the message with ID `12345`.
* `id:12345`: Search for only message with ID `12345`.

The message ID operand for the `id` operator may be encoded as either a
number or a string. The message ID operand for the `near` operator must
be encoded as a string.
The message ID operand for the `with` and `id` operators may be encoded
as either a number or a string. The message ID operand for the `near`
operator must be encoded as a string.

**Changes**: Prior to Zulip 8.0 (feature level 194), the message ID
operand for the `id` operator needed to be encoded as a string.
Expand Down
2 changes: 1 addition & 1 deletion version.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
# Changes should be accompanied by documentation explaining what the
# new level means in api_docs/changelog.md, as well as "**Changes**"
# entries in the endpoint's documentation in `zulip.yaml`.
API_FEATURE_LEVEL = 267
API_FEATURE_LEVEL = 268

# Bump the minor PROVISION_VERSION to indicate that folks should provision
# only when going from an old version of the code to a newer version. Bump
Expand Down
88 changes: 84 additions & 4 deletions web/src/filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,12 +295,12 @@ export class Filter {
_sorted_term_types?: string[] = undefined;
_predicate?: (message: Message) => boolean;
_can_mark_messages_read?: boolean;
requires_adjustment_for_moved_with_target?: boolean;

constructor(terms: NarrowTerm[]) {
this._terms = this.fix_terms(terms);
if (this.has_operator("channel")) {
this._sub = stream_data.get_sub_by_name(this.operands("channel")[0]!);
}
this._terms = terms;
this.setup_filter(terms);
this.requires_adjustment_for_moved_with_target = this.has_operator("with");
}

static canonicalize_operator(operator: string): string {
Expand Down Expand Up @@ -389,6 +389,35 @@ export class Filter {
};
}

static canonicalize_narrow(narrow: NarrowTerm[]): NarrowTerm[] {
// In presence of `with` term without channel or topic terms in the narrow, the
// narrow is populated with the channel and toipic terms through this operation,
// so that `with` can be used as a standalone operator to target conversation.
const with_term = narrow.find((term: NarrowTerm) => term.operator === "with");

if (with_term) {
let channel_term = narrow.find(
(term: NarrowTerm) => Filter.canonicalize_operator(term.operator) === "channel",
);
let topic_term = narrow.find(
(term: NarrowTerm) => Filter.canonicalize_operator(term.operator) === "topic",
);

if (!topic_term) {
topic_term = {operator: "topic", operand: ""};
const with_index = narrow.indexOf(with_term);
narrow.splice(with_index, 0, topic_term);
}

if (!channel_term) {
channel_term = {operator: "channel", operand: ""};
const topic_index = narrow.indexOf(topic_term);
narrow.splice(topic_index, 0, channel_term);
}
}
return narrow;
}

/* We use a variant of URI encoding which looks reasonably
nice and still handles unambiguously cases such as
spaces in operands.
Expand Down Expand Up @@ -598,6 +627,7 @@ export class Filter {
"channels-public",
"channel",
"topic",
"with",
"dm",
"dm-including",
"sender",
Expand Down Expand Up @@ -806,6 +836,8 @@ export class Filter {
const adjusted_terms = [];
let terms_changed = false;

raw_terms = Filter.canonicalize_narrow(raw_terms);

for (const term of raw_terms) {
const adjusted_term = {...term};
if (
Expand Down Expand Up @@ -834,6 +866,13 @@ export class Filter {
return adjusted_terms;
}

setup_filter(terms: NarrowTerm[]): void {
this._terms = this.fix_terms(terms);
if (this.has_operator("channel")) {
this._sub = stream_data.get_sub_by_name(this.operands("channel")[0]!);
}
}

equals(filter: Filter, excluded_operators?: string[]): boolean {
return _.isEqual(
filter.sorted_terms(excluded_operators),
Expand Down Expand Up @@ -951,6 +990,7 @@ export class Filter {
"channels-web-public",
"not-channels-web-public",
"near",
"with",
]);

for (const term of term_types) {
Expand Down Expand Up @@ -985,6 +1025,10 @@ export class Filter {
// it is limited by the user's message history. Therefore, we check "channel"
// and "topic" together to ensure that the current filter will return all the
// messages of a conversation.
if (_.isEqual(term_types, ["channel", "topic", "with"])) {
return true;
}

if (_.isEqual(term_types, ["channel", "topic"])) {
return true;
}
Expand Down Expand Up @@ -1202,6 +1246,7 @@ export class Filter {
const term_types = this.sorted_term_types();
if (
(term_types.length === 3 && _.isEqual(term_types, ["channel", "topic", "near"])) ||
(term_types.length === 3 && _.isEqual(term_types, ["channel", "topic", "with"])) ||
(term_types.length === 2 && _.isEqual(term_types, ["channel", "topic"])) ||
(term_types.length === 1 && _.isEqual(term_types, ["channel"]))
) {
Expand Down Expand Up @@ -1384,17 +1429,29 @@ export class Filter {
fix_terms(terms: NarrowTerm[]): NarrowTerm[] {
terms = this._canonicalize_terms(terms);
terms = this._fix_redundant_is_private(terms);
terms = this._fix_redundant_with_dm(terms);
return terms;
}

_fix_redundant_is_private(terms: NarrowTerm[]): NarrowTerm[] {
// Every DM is a DM, so drop `is:dm` if on a DM conversation.
if (!terms.some((term) => Filter.term_type(term) === "dm")) {
return terms;
}

return terms.filter((term) => Filter.term_type(term) !== "is-dm");
}

_fix_redundant_with_dm(terms: NarrowTerm[]): NarrowTerm[] {
// Because DMs can't move, the `with` operator is a noop on a
// DM conversation.
if (terms.some((term) => Filter.term_type(term) === "dm")) {
return terms.filter((term) => Filter.term_type(term) !== "with");
}

return terms;
}

_canonicalize_terms(terms_mixed_case: NarrowTerm[]): NarrowTerm[] {
return terms_mixed_case.map((term: NarrowTerm) => Filter.canonicalize_term(term));
}
Expand Down Expand Up @@ -1534,4 +1591,27 @@ export class Filter {
!this.has_operand("is", "starred")
);
}

try_adjusting_for_moved_with_target(message?: Message): void {
// If we have the message named in a `with` operator
// available, either via parameter or message_store,
if (!this.requires_adjustment_for_moved_with_target) {
return;
}

if (!message) {
const message_id = Number.parseInt(this.operands("with")[0]!, 10);
message = message_store.get(message_id);
}

if (!message) {
return;
}

const adjusted_terms = Filter.adjusted_terms_if_moved(this._terms, message);
if (adjusted_terms) {
this.setup_filter(adjusted_terms);
}
this.requires_adjustment_for_moved_with_target = false;
}
}
1 change: 1 addition & 0 deletions web/src/hash_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ export const allowed_web_public_narrows = [
"search",
"near",
"id",
"with",
];

export function is_spectator_compatible(hash: string): boolean {
Expand Down
4 changes: 4 additions & 0 deletions web/src/message_fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ function process_result(data, opts) {

if (messages.length !== 0) {
if (opts.msg_list) {
if (opts.validate_filter_topic_post_fetch) {
opts.msg_list.data.filter.try_adjusting_for_moved_with_target(messages[0]);
}
// Since this adds messages to the MessageList and renders MessageListView,
// we don't need to call it if msg_list was not defined by the caller.
message_util.add_old_messages(messages, opts.msg_list);
Expand Down Expand Up @@ -390,6 +393,7 @@ export function load_messages_for_narrow(opts) {
num_after: consts.narrow_after,
msg_list: opts.msg_list,
cont: opts.cont,
validate_filter_topic_post_fetch: opts.validate_filter_topic_post_fetch,
});
}

Expand Down
34 changes: 29 additions & 5 deletions web/src/message_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,17 @@ export function changehash(newhash, trigger) {
return;
}
message_viewport.stop_auto_scrolling();
browser_history.set_hash(newhash);

// It is important to use `replaceState` rather than `replace` in case
// of "retarget topic location" because we retarget to the last unread.
// Using `replce` would update the hash and load it once more.
// This would impact the `narrow_pointer` for users with
// `mark_read_on_scroll_state_policy` set.
if (trigger === "retarget topic location") {
window.history.replaceState(null, "", newhash);
} else {
browser_history.set_hash(newhash);
}
}

export function update_hash_to_match_filter(filter, trigger) {
Expand Down Expand Up @@ -134,10 +144,12 @@ function create_and_update_message_list(filter, id_info, opts) {
// Populate the message list if we can apply our filter locally (i.e.
// with no backend help) and we have the message we want to select.
// Also update id_info accordingly.
maybe_add_local_messages({
id_info,
msg_data,
});
if (!filter.requires_adjustment_for_moved_with_target) {
maybe_add_local_messages({
id_info,
msg_data,
});
}

if (!id_info.local_select_id) {
// If we're not actually ready to select an ID, we need to
Expand Down Expand Up @@ -306,6 +318,7 @@ export function show(raw_terms, opts) {
raw_terms = [{operator: "is", operand: "home"}];
}
const filter = new Filter(raw_terms);
filter.try_adjusting_for_moved_with_target();

if (try_rendering_locally_for_same_narrow(filter, opts)) {
return;
Expand Down Expand Up @@ -642,7 +655,18 @@ export function show(raw_terms, opts) {
}
message_fetch.load_messages_for_narrow({
anchor,
validate_filter_topic_post_fetch:
filter.requires_adjustment_for_moved_with_target,
cont() {
if (
!filter.requires_adjustment_for_moved_with_target &&
filter.has_operator("with")
) {
// In this situation, we've adjusted our filter via
// filter.try_adjusting_for_moved_with_target, and should
// update the URL hash accordingly.
update_hash_to_match_filter(filter, "retarget topic location");
}
if (!select_immediately) {
render_message_list_with_selected_message({
id_info,
Expand Down
14 changes: 14 additions & 0 deletions web/src/narrow_state.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import assert from "minimalistic-assert";

import * as blueslip from "./blueslip";
import {Filter} from "./filter";
import * as message_lists from "./message_lists";
import * as message_store from "./message_store";
import {page_params} from "./page_params";
import * as people from "./people";
import type {NarrowTerm} from "./state_data";
Expand Down Expand Up @@ -265,6 +268,17 @@ export function _possible_unread_message_ids(
let topic_name;
let current_filter_pm_string;

if (current_filter.can_bucket_by("channel", "topic", "with")) {
const with_operand = current_filter.operands("with")[0];
assert(with_operand !== undefined);
const msg_id = Number.parseInt(with_operand, 10);
const msg = message_store.get(msg_id);
if (msg !== undefined && msg.type === "stream") {
return unread.get_msg_ids_for_topic(msg.stream_id, msg.topic);
}
return [];
}

if (current_filter.can_bucket_by("channel", "topic")) {
sub = stream_sub(current_filter);
topic_name = topic(current_filter);
Expand Down
Loading

0 comments on commit 1be4d9c

Please sign in to comment.