Skip to content

Commit

Permalink
narrow: Clarify some confusing details.
Browse files Browse the repository at this point in the history
The update_selection function name was rather misleading, since that
function call is in fact what renders the message list object for the
view.

Also add comments about a few subtle/confusing details that I noticed
while debugging this code path today.
  • Loading branch information
timabbott committed Feb 1, 2024
1 parent 7d4ec1f commit 652fea9
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 5 deletions.
9 changes: 9 additions & 0 deletions web/src/message_list.js
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,15 @@ export class MessageList {
this.data.set_selected_id(id);

if (opts.force_rerender) {
// TODO: Because rerender() itself will call
// reselect_selected_id after doing the rendering, we
// actually end up with this function being called
// recursively in this case. The ordering will end up
// being that the message_selected.zulip event for that
// rerender is processed before execution returns here.
//
// The recursive call is unnecessary, so we should figure
// out how to avoid that, both here and in the next block.
this.rerender();
} else if (!opts.from_rendering) {
this.view.maybe_rerender();
Expand Down
14 changes: 9 additions & 5 deletions web/src/narrow.js
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ export function activate(raw_terms, opts) {
anchor,
cont() {
if (!select_immediately) {
update_selection({
render_message_list_with_selected_message({
id_info,
select_offset: then_select_offset,
msg_list: message_lists.current,
Expand All @@ -509,14 +509,14 @@ export function activate(raw_terms, opts) {
}

// Important: We need to consider opening the compose box
// before calling update_selection, so that the logic in
// before calling render_message_list_with_selected_message, so that the logic in
// recenter_view for positioning the currently selected
// message can take into account the space consumed by the
// open compose box.
compose_actions.on_narrow(opts);

if (select_immediately) {
update_selection({
render_message_list_with_selected_message({
id_info,
select_offset: then_select_offset,
msg_list: message_lists.current,
Expand All @@ -532,7 +532,6 @@ export function activate(raw_terms, opts) {

handle_post_view_change(msg_list);


unread_ui.update_unread_banner();

// It is important to call this after other important updates
Expand Down Expand Up @@ -740,7 +739,7 @@ export function maybe_add_local_messages(opts) {
return;
}

export function update_selection(opts) {
export function render_message_list_with_selected_message(opts) {
if (message_lists.current !== opts.msg_list) {
// If we navigated away from a view while we were fetching
// messages for it, don't attempt to move the currently
Expand Down Expand Up @@ -768,6 +767,11 @@ export function update_selection(opts) {

const then_scroll = !preserve_pre_narrowing_screen_position;

// Here we render the actual message list to the DOM with the
// target selected message, using the force_rerender parameter.
//
// TODO: Probably this should accept the offset parameter rather
// than calling `set_message_offset` just after.
message_lists.current.select_id(msg_id, {
then_scroll,
use_closest: true,
Expand Down

0 comments on commit 652fea9

Please sign in to comment.