From 240f7b53b2c007d12522a6252cc953590a443358 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Sat, 27 Apr 2024 06:26:18 +0000 Subject: [PATCH] message_lists: Remove `save_pre_narrow_offset_for_reload`. This is no longer required since narrow state is restored by `reload` library and narrow history restores the narrow pointer and offset when going back to a view. Only regression here is we will no longer restore the pointer when user navigates to the combined feed view without using browser back button and combined feed view is the default view. This is fixed in the next commit. --- web/src/message_lists.ts | 27 --------------------------- web/src/narrow.js | 28 +++++----------------------- web/src/unread.ts | 6 ------ web/src/unread_ops.js | 13 ------------- web/src/views_util.ts | 2 -- web/tests/narrow_activate.test.js | 2 -- 6 files changed, 5 insertions(+), 73 deletions(-) diff --git a/web/src/message_lists.ts b/web/src/message_lists.ts index 3e86ace384..95830a0747 100644 --- a/web/src/message_lists.ts +++ b/web/src/message_lists.ts @@ -1,6 +1,5 @@ import $ from "jquery"; -import * as blueslip from "./blueslip"; import * as inbox_util from "./inbox_util"; import type {MessageListData} from "./message_list_data"; import type {Message} from "./message_store"; @@ -39,7 +38,6 @@ export type MessageList = { selected_idx: () => number; all_messages: () => Message[]; get: (id: number) => Message | undefined; - pre_narrow_offset?: number; can_mark_messages_read_without_setting: () => boolean; rerender_view: () => void; resume_reading: () => void; @@ -109,31 +107,6 @@ export function update_recipient_bar_background_color(): void { inbox_util.update_stream_colors(); } -export function save_pre_narrow_offset_for_reload(): void { - // Only save the pre_narrow_offset if the message list will be cached if user - // switches to a different narrow, otherwise the pre_narrow_offset would just be lost when - // user switches to a different narrow. In case of a reload, offset for the current - // message is captured and restored by `reload` library. - if (!current?.preserve_rendered_state) { - return; - } - - if (current.selected_id() !== -1) { - if (current.selected_row().length === 0) { - const current_message = current.get(current.selected_id()); - blueslip.debug("narrow.activate missing selected row", { - selected_id: current.selected_id(), - selected_idx: current.selected_idx(), - selected_idx_exact: - current_message && current.all_messages().indexOf(current_message), - render_start: current.view._render_win_start, - render_end: current.view._render_win_end, - }); - } - current.pre_narrow_offset = current.selected_row().get_offset_to_window().top; - } -} - export function calculate_timestamp_widths(): void { const $temp_time_div = $("
"); $temp_time_div.attr("id", "calculated-timestamp-widths"); diff --git a/web/src/narrow.js b/web/src/narrow.js index a8d52a9b9e..eb884d2261 100644 --- a/web/src/narrow.js +++ b/web/src/narrow.js @@ -51,7 +51,6 @@ import * as stream_data from "./stream_data"; import * as stream_list from "./stream_list"; import * as topic_generator from "./topic_generator"; import * as typing_events from "./typing_events"; -import * as unread from "./unread"; import * as unread_ops from "./unread_ops"; import * as unread_ui from "./unread_ui"; import {user_settings} from "./user_settings"; @@ -373,11 +372,6 @@ export function activate(raw_terms, opts) { recent_view_ui.hide(); } else if (coming_from_inbox) { inbox_ui.hide(); - } else { - // We must instead be switching from another message view. - // Save the scroll position in that message list, so that - // we can restore it if/when we later navigate back to that view. - message_lists.save_pre_narrow_offset_for_reload(); } // Open tooltips are only interesting for current narrow, @@ -423,10 +417,6 @@ export function activate(raw_terms, opts) { } } - if (!was_narrowed_already) { - unread.set_messages_read_in_narrow(false); - } - const excludes_muted_topics = filter.excludes_muted_topics(); // Check if we already have a rendered message list for the `filter`. @@ -494,20 +484,12 @@ export function activate(raw_terms, opts) { force_rerender: false, }; - if (unread.messages_read_in_narrow) { - // We read some unread messages in a narrow. Instead of going back to - // where we were before the narrow, go to our first unread message (or - // the bottom of the feed, if there are no unread messages). - id_info.final_select_id = message_lists.current.first_unread_message_id(); - } else { - // We narrowed, but only backwards in time (ie no unread were read). Try - // to go back to exactly where we were before narrowing. - // We scroll the user back to exactly the offset from the selected - // message that they were at the time that they narrowed. - // TODO: Make this correctly handle the case of resizing while narrowed. - then_select_offset = message_lists.current.pre_narrow_offset; - id_info.final_select_id = message_lists.current.selected_id(); + if (opts.then_select_id !== -1) { + // Restore user's last position in narrow if user is navigation via browser back / forward button. + id_info.final_select_id = opts.then_select_id; + then_select_offset = opts.then_select_offset; } + // We are navigating to the combined feed from another // narrow, so we reset the reading state to allow user to // read messages again in the combined feed if user has diff --git a/web/src/unread.ts b/web/src/unread.ts index e2e7f5e167..eec7cb39df 100644 --- a/web/src/unread.ts +++ b/web/src/unread.ts @@ -25,12 +25,6 @@ import * as util from "./util"; // See https://zulip.readthedocs.io/en/latest/subsystems/pointer.html // for more details on how this system is designed. -export let messages_read_in_narrow = false; - -export function set_messages_read_in_narrow(value: boolean): void { - messages_read_in_narrow = value; -} - export let old_unreads_missing = false; export function clear_old_unreads_missing(): void { diff --git a/web/src/unread_ops.js b/web/src/unread_ops.js index bfbdeda2e8..0960296bfc 100644 --- a/web/src/unread_ops.js +++ b/web/src/unread_ops.js @@ -311,11 +311,6 @@ export function process_read_messages_event(message_ids) { if (message_ids.length === 0) { return; } - if (message_lists.current?.narrowed) { - // I'm not sure this entirely makes sense for all server - // notifications. - unread.set_messages_read_in_narrow(true); - } for (const message_id of message_ids) { unread.mark_as_read(message_id); @@ -340,10 +335,6 @@ export function process_unread_messages_event({message_ids, message_details}) { return; } - if (message_lists.current?.narrowed) { - unread.set_messages_read_in_narrow(false); - } - for (const message_id of message_ids) { const message = message_store.get(message_id); const message_info = message_details[message_id]; @@ -420,10 +411,6 @@ export function notify_server_messages_read(messages, options = {}) { message_flags.send_read(messages); - if (message_lists.current?.narrowed) { - unread.set_messages_read_in_narrow(true); - } - for (const message of messages) { unread.mark_as_read(message.id); process_newly_read_message(message, options); diff --git a/web/src/views_util.ts b/web/src/views_util.ts index 3ac5460e90..ff8f505c19 100644 --- a/web/src/views_util.ts +++ b/web/src/views_util.ts @@ -72,8 +72,6 @@ export function show(opts: { complete_rerender: () => void; is_recent_view?: boolean; }): void { - message_lists.save_pre_narrow_offset_for_reload(); - if (opts.is_visible()) { // If we're already visible, E.g. because the user hit Esc // while already in the view, do nothing. diff --git a/web/tests/narrow_activate.test.js b/web/tests/narrow_activate.test.js index 39050efafa..7889cc03eb 100644 --- a/web/tests/narrow_activate.test.js +++ b/web/tests/narrow_activate.test.js @@ -103,7 +103,6 @@ function test_helper({override}) { stub(narrow_history, "save_narrow_state_and_flush"); stub(message_feed_loading, "hide_indicators"); stub(message_feed_top_notices, "hide_top_of_narrow_notices"); - stub(message_lists, "save_pre_narrow_offset_for_reload"); stub(narrow_title, "update_narrow_title"); stub(stream_list, "handle_narrow_activated"); stub(message_view_header, "render_title_area"); @@ -225,7 +224,6 @@ run_test("basics", ({override}) => { [message_feed_top_notices, "hide_top_of_narrow_notices"], [message_feed_loading, "hide_indicators"], [compose_banner, "clear_message_sent_banners"], - [message_lists, "save_pre_narrow_offset_for_reload"], [compose_actions, "on_narrow"], [unread_ops, "process_visible"], [narrow_history, "save_narrow_state_and_flush"],