From a6886fb0897b42415f25c05b6fd7ee9a1c327534 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 18 Nov 2024 11:05:45 -0600 Subject: [PATCH] Send close messages on_flushed() instead of on_flush() to avoid closing before render --- js/src/output.ts | 35 +++++++++++++++-------------------- shinywidgets/_comm.py | 13 ++++++++++--- shinywidgets/static/output.js | 2 +- 3 files changed, 26 insertions(+), 24 deletions(-) diff --git a/js/src/output.ts b/js/src/output.ts index ca455a1..25990e1 100644 --- a/js/src/output.ts +++ b/js/src/output.ts @@ -9,7 +9,6 @@ import type { ErrorsMessageValue } from 'rstudio-shiny/srcts/types/src/shiny/shi ******************************************************************************/ class OutputManager extends HTMLManager { - orphaned_models: string[]; // In a soon-to-be-released version of @jupyter-widgets/html-manager, // display_view()'s first "dummy" argument will be removed... this shim simply // makes it so that our manager can work with either version @@ -101,23 +100,6 @@ class IPyWidgetOutput extends Shiny.OutputBinding { const view = await manager.create_view(model, {}); await manager.display_view(view, {el: el}); - // If the model was orphaned, close it (and thus, the view as well) now - if (manager.orphaned_models.length > 0) { - for (const model_id of manager.orphaned_models) { - const model = await manager.get_model(model_id); - if (model) { - // Closing the model removes the previous view - await model.close(); - // .close() isn't enough to remove manager's reference to it, - // and apparently the only way to remove it is through the `comm:close` event - // https://github.com/jupyter-widgets/ipywidgets/blob/303cae4/packages/base-manager/src/manager-base.ts#L330-L337 - // https://github.com/jupyter-widgets/ipywidgets/blob/303cae4/packages/base/src/widget.ts#L251-L253 - model.trigger("comm:close"); - } - } - manager.orphaned_models = []; - } - // The ipywidgets container (.lmWidget) const lmWidget = el.children[0] as HTMLElement; @@ -217,10 +199,23 @@ Shiny.addCustomMessageHandler("shinywidgets_comm_msg", (msg_txt) => { // next render. This is because we currently trigger a close _during_ the server-side // render, and thus, immediately closing the model removes the view before a new one is // ready. -manager.orphaned_models = []; Shiny.addCustomMessageHandler("shinywidgets_comm_close", (msg_txt) => { const msg = jsonParse(msg_txt); - manager.orphaned_models.push(msg.content.comm_id); + const id = msg.content.comm_id; + const model = manager.get_model(id); + if (!model) { + console.error(`Couldn't close model ${id} because it doesn't exist.`); + return; + } + model.then(m => { + // Closing the model removes the previous view + m.close(); + // .close() isn't enough to remove manager's reference to it, + // and apparently the only way to remove it is through the `comm:close` event + // https://github.com/jupyter-widgets/ipywidgets/blob/303cae4/packages/base-manager/src/manager-base.ts#L330-L337 + // https://github.com/jupyter-widgets/ipywidgets/blob/303cae4/packages/base/src/widget.ts#L251-L253 + m.trigger("comm:close"); + }); }); // At least currently, all widgets must be created within a session scope, so we can diff --git a/shinywidgets/_comm.py b/shinywidgets/_comm.py index d7c7e22..b9166cc 100644 --- a/shinywidgets/_comm.py +++ b/shinywidgets/_comm.py @@ -169,10 +169,17 @@ def _publish_msg( def _send(): run_coro_hybrid(session.send_custom_message(msg_type, msg_txt)) # type: ignore - # N.B., if we don't do this on flush, then if you initialize a widget - # outside of a reactive context, run_coro_sync() will complain with + # N.B., if messages are sent immediately, run_coro_sync() could fail with # 'async function yielded control; it did not finish in one iteration.' - session.on_flush(_send) + # if executed outside of a reactive context. + if msg_type == "shinywidgets_comm_close": + # The primary way widgets are closed are when a new widget is rendered in + # its place (see render_widget_base). By sending close on_flushed(), we + # ensure to close the 'old' widget after the new one is created. (avoiding a + # "flicker" of the old widget being removed before the new one is created) + session.on_flushed(_send) + else: + session.on_flush(_send) # This is the method that ipywidgets.widgets.Widget uses to respond to client-side changes def on_msg(self, callback: MsgCallback) -> None: diff --git a/shinywidgets/static/output.js b/shinywidgets/static/output.js index 68e109c..678af17 100644 --- a/shinywidgets/static/output.js +++ b/shinywidgets/static/output.js @@ -36,7 +36,7 @@ eval("__webpack_require__.r(__webpack_exports__);\n/* harmony export */ __webpac \***********************/ /***/ ((__unused_webpack_module, __webpack_exports__, __webpack_require__) => { -eval("__webpack_require__.r(__webpack_exports__);\n/* harmony import */ var _jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! @jupyter-widgets/html-manager */ \"@jupyter-widgets/html-manager\");\n/* harmony import */ var _jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0___default = /*#__PURE__*/__webpack_require__.n(_jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0__);\n/* harmony import */ var _comm__WEBPACK_IMPORTED_MODULE_1__ = __webpack_require__(/*! ./comm */ \"./src/comm.ts\");\n/* harmony import */ var _utils__WEBPACK_IMPORTED_MODULE_2__ = __webpack_require__(/*! ./utils */ \"./src/utils.ts\");\nvar _a;\n\n\n\n/******************************************************************************\n * Define a custom HTMLManager for use with Shiny\n ******************************************************************************/\nclass OutputManager extends _jupyter_widgets_html_manager__WEBPACK_IMPORTED_MODULE_0__.HTMLManager {\n // In a soon-to-be-released version of @jupyter-widgets/html-manager,\n // display_view()'s first \"dummy\" argument will be removed... this shim simply\n // makes it so that our manager can work with either version\n // https://github.com/jupyter-widgets/ipywidgets/commit/159bbe4#diff-45c126b24c3c43d2cee5313364805c025e911c4721d45ff8a68356a215bfb6c8R42-R43\n async display_view(view, options) {\n const n_args = super.display_view.length;\n if (n_args === 3) {\n return super.display_view({}, view, options);\n }\n else {\n // @ts-ignore\n return super.display_view(view, options);\n }\n }\n}\n// Define our own custom module loader for Shiny\nconst shinyRequireLoader = async function (moduleName, moduleVersion) {\n // shiny provides require.js and also sets `define.amd=false` to prevent