From 61b3c698affb9aebf29300d36f42a1d8440151a6 Mon Sep 17 00:00:00 2001 From: N-Shar-ma Date: Tue, 7 May 2024 07:20:04 +0530 Subject: [PATCH] widgets: Always render and activate widgets in the current view. Earlier we did not activate widgets if their rendered HTML was cached, and also when narrowing to the combined feed view. This caused bugs with widgets not being activated, and so not responding to any interactions. Now we unconditionally render and activate widgets in the current view, irrespective of the cached HTML or the view being narrowed to. --- web/src/narrow.js | 4 ++-- web/src/submessage.ts | 9 +++++++++ web/src/widgetize.ts | 27 ++------------------------- web/tests/widgetize.test.js | 27 +-------------------------- 4 files changed, 14 insertions(+), 53 deletions(-) diff --git a/web/src/narrow.js b/web/src/narrow.js index eb884d2261..7c28b2ffa0 100644 --- a/web/src/narrow.js +++ b/web/src/narrow.js @@ -49,13 +49,13 @@ import * as spectators from "./spectators"; import {realm} from "./state_data"; import * as stream_data from "./stream_data"; import * as stream_list from "./stream_list"; +import * as submessage from "./submessage"; import * as topic_generator from "./topic_generator"; import * as typing_events from "./typing_events"; import * as unread_ops from "./unread_ops"; import * as unread_ui from "./unread_ui"; import {user_settings} from "./user_settings"; import * as util from "./util"; -import * as widgetize from "./widgetize"; const LARGER_THAN_MAX_MESSAGE_ID = 10000000000000000; @@ -499,7 +499,7 @@ export function activate(raw_terms, opts) { // Reset the collapsed status of messages rows. condense.condense_and_collapse(message_lists.current.view.$list.find(".message_row")); message_edit.restore_edit_state_after_message_view_change(); - widgetize.set_widgets_for_list(); + submessage.process_widget_rows_in_list(message_lists.current); message_feed_top_notices.update_top_of_narrow_notices(msg_list); // We may need to scroll to the selected message after swapping diff --git a/web/src/submessage.ts b/web/src/submessage.ts index 80dc092309..0bfec26e74 100644 --- a/web/src/submessage.ts +++ b/web/src/submessage.ts @@ -2,6 +2,7 @@ import {z} from "zod"; import * as blueslip from "./blueslip"; import * as channel from "./channel"; +import type {MessageList} from "./message_lists"; import * as message_store from "./message_store"; import type {Message} from "./message_store"; import type {Submessage} from "./types"; @@ -87,6 +88,14 @@ export function get_message_events(message: Message): SubmessageEvents | undefin return clean_events; } +export function process_widget_rows_in_list(list: MessageList | undefined): void { + for (const message_id of widgetize.widget_contents.keys()) { + if (list?.get(message_id) !== undefined) { + process_submessages({message_id, $row: list.get_row(message_id)}); + } + } +} + export function process_submessages(in_opts: {$row: JQuery; message_id: number}): void { // This happens in our rendering path, so we try to limit any // damage that may be triggered by one rogue message. diff --git a/web/src/widgetize.ts b/web/src/widgetize.ts index cbbbad2aed..ea5356dc96 100644 --- a/web/src/widgetize.ts +++ b/web/src/widgetize.ts @@ -47,15 +47,7 @@ export function clear_for_testing(): void { function set_widget_in_message($row: JQuery, $widget_elem: JQuery): void { const $content_holder = $row.find(".message_content"); - - // Avoid adding the $widget_elem if it already exists. - // This can happen when the app loads in the "Recent Conversations" - // view and the user changes the view to "Combined feed". - // This is important since jQuery removes all the event handlers - // on `empty()`ing an element. - if ($content_holder.find(".widget-content").length === 0) { - $content_holder.empty().append($widget_elem); - } + $content_holder.empty().append($widget_elem); } export function activate(in_opts: WidgetOptions): void { @@ -86,15 +78,9 @@ export function activate(in_opts: WidgetOptions): void { return; } - let $widget_elem = widget_contents.get(message.id); - if ($widget_elem) { - set_widget_in_message($row, $widget_elem); - return; - } - // We depend on our widgets to use templates to build // the HTML that will eventually go in this div. - $widget_elem = $("
").addClass("widget-content"); + const $widget_elem = $("
").addClass("widget-content"); widgets.get(widget_type)!.activate({ $elem: $widget_elem, @@ -114,15 +100,6 @@ export function activate(in_opts: WidgetOptions): void { } } -export function set_widgets_for_list(): void { - for (const [idx, $widget_elem] of widget_contents) { - if (message_lists.current?.get(idx) !== undefined) { - const $row = message_lists.current.get_row(idx); - set_widget_in_message($row, $widget_elem); - } - } -} - export function handle_event(widget_event: Event & {message_id: number}): void { const $widget_elem = widget_contents.get(widget_event.message_id); diff --git a/web/tests/widgetize.test.js b/web/tests/widgetize.test.js index db74c92619..167e99aeeb 100644 --- a/web/tests/widgetize.test.js +++ b/web/tests/widgetize.test.js @@ -73,7 +73,7 @@ function test(label, f) { }); } -test("activate", ({override}) => { +test("activate", () => { // Both widgetize.activate and widgetize.handle_event are tested // here to use the "caching" of widgets const $row = $.create(""); @@ -108,7 +108,6 @@ test("activate", ({override}) => { is_event_handled = false; assert.ok(!widgetize.widget_contents.has(opts.message.id)); - $message_content.set_find_results(".widget-content", false); widgetize.activate(opts); assert.ok(is_widget_elem_inserted); @@ -116,23 +115,11 @@ test("activate", ({override}) => { assert.ok(is_event_handled); assert.equal(widgetize.widget_contents.get(opts.message.id), $widget_elem); - is_widget_elem_inserted = false; - is_widget_activated = false; - is_event_handled = false; - - $message_content.set_find_results(".widget-content", false); - widgetize.activate(opts); - - assert.ok(is_widget_elem_inserted); - assert.ok(!is_widget_activated); - assert.ok(!is_event_handled); - message_lists.current = undefined; is_widget_elem_inserted = false; is_widget_activated = false; is_event_handled = false; - $message_content.set_find_results(".widget-content", false); widgetize.activate(opts); assert.ok(!is_widget_elem_inserted); @@ -179,16 +166,4 @@ test("activate", ({override}) => { post_activate_event.message_id = 1000; widgetize.handle_event(post_activate_event); assert.ok(!is_event_handled); - - message_lists.current = {id: 2}; - /* Test narrow change message update */ - override(message_lists.current, "get", (idx) => { - assert.equal(idx, 2001); - return {}; - }); - override(message_lists.current, "get_row", (idx) => { - assert.equal(idx, 2001); - return $row; - }); - widgetize.set_widgets_for_list(); });