From e02721a0c15a5c1503651a344c51d90dd8266015 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Fri, 14 Mar 2025 17:40:07 -0400 Subject: [PATCH 1/5] Add `session.bookmark.show_bookmark_url_modal()` --- shiny/bookmark/_bookmark.py | 477 +++++++++++++----------------------- 1 file changed, 175 insertions(+), 302 deletions(-) diff --git a/shiny/bookmark/_bookmark.py b/shiny/bookmark/_bookmark.py index 57a971891..8e98088bc 100644 --- a/shiny/bookmark/_bookmark.py +++ b/shiny/bookmark/_bookmark.py @@ -3,7 +3,9 @@ import warnings from abc import ABC, abstractmethod from pathlib import Path -from typing import TYPE_CHECKING, Awaitable, Callable, Literal +from typing import TYPE_CHECKING, Awaitable, Callable, Literal, NoReturn, Optional + +from htmltools import TagChild from .._docstring import add_example from .._utils import AsyncCallbacks, CancelCallback, wrap_async @@ -126,6 +128,8 @@ def on_bookmarked( This callback will be executed **after** the bookmark state is saved serverside or in the URL. + Note: `.on_bookmarked()` can only be added to the `AppSession` object, not on a a module's session. + Parameters ---------- callback @@ -215,6 +219,157 @@ async def do_bookmark(self) -> None: """ ... + async def show_bookmark_url_modal( + self, + url: str | None = None, + ) -> None: + """ + Display a modal dialog for displaying the bookmark URL. + + This method should be called when the bookmark button is clicked, and it + should display a modal dialog with the current bookmark URL. + + Parameters + ---------- + url + The URL to display in the modal dialog. If `None`, the current bookmark + URL will be retrieved using `session.bookmark.get_bookmark_url()`. + """ + import textwrap + + from htmltools import TagList, tags + + from ..ui import modal, modal_show + + title = "Bookmarked application link" + subtitle: str | None = None + if self.store == "url": + subtitle = "This link stores the current state of this application." + elif self.store == "server": + subtitle = ( + "The current state of this application has been stored on the server." + ) + + if url is None: + url = await self.get_bookmark_url() + + subtitle_tag = TagList( + tags.br(), + ( + tags.span(subtitle + " ", class_="text-muted") + if isinstance(subtitle, str) + else subtitle + ), + tags.span(id="shiny-bookmark-copy-text", class_="text-muted"), + ) + # Need separate show and shown listeners. The show listener sizes the + # textarea just as the modal starts to fade in. The 200ms delay is needed + # because if we try to resize earlier, it can't calculate the text height + # (scrollHeight will be reported as zero). The shown listener selects the + # text; it's needed because because selection has to be done after the fade- + # in is completed. + modal_js = tags.script( + textwrap.dedent( + """ + $('#shiny-modal'). + one('show.bs.modal', function() { + setTimeout(function() { + var $textarea = $('#shiny-modal textarea'); + $textarea.innerHeight($textarea[0].scrollHeight); + }, 200); + }); + $('#shiny-modal') + .one('shown.bs.modal', function() { + $('#shiny-modal textarea').select().focus(); + }); + $('#shiny-bookmark-copy-text') + .text(function() { + if (/Mac/i.test(navigator.userAgent)) { + return 'Press \u2318-C to copy.'; + } else { + return 'Press Ctrl-C to copy.'; + } + }); + """ + ) + ) + + url_modal = modal( + tags.textarea( + url, + class_="form-control", + rows="1", + style="resize: none;", + readonly="readonly", + ), + subtitle_tag, + modal_js, + title=title, + easy_close=True, + ) + + modal_show(url_modal) + return + + +# #' Generate a modal dialog that displays a URL +# #' +# #' The modal dialog generated by `urlModal` will display the URL in a +# #' textarea input, and the URL text will be selected so that it can be easily +# #' copied. The result from `urlModal` should be passed to the +# #' [showModal()] function to display it in the browser. +# #' +# #' @param url A URL to display in the dialog box. +# #' @param title A title for the dialog box. +# #' @param subtitle Text to display underneath URL. +# #' @export +# urlModal <- function(url, title = "Bookmarked application link", subtitle = NULL) { + +# subtitleTag <- tagList( +# br(), +# span(class = "text-muted", subtitle), +# span(id = "shiny-bookmark-copy-text", class = "text-muted") +# ) + +# modalDialog( +# title = title, +# easyClose = TRUE, +# tags$textarea(class = "form-control", rows = "1", style = "resize: none;", +# readonly = "readonly", +# url +# ), +# subtitleTag, +# # Need separate show and shown listeners. The show listener sizes the +# # textarea just as the modal starts to fade in. The 200ms delay is needed +# # because if we try to resize earlier, it can't calculate the text height +# # (scrollHeight will be reported as zero). The shown listener selects the +# # text; it's needed because because selection has to be done after the fade- +# # in is completed. +# tags$script( +# "$('#shiny-modal'). +# one('show.bs.modal', function() { +# setTimeout(function() { +# var $textarea = $('#shiny-modal textarea'); +# $textarea.innerHeight($textarea[0].scrollHeight); +# }, 200); +# }); +# $('#shiny-modal') +# .one('shown.bs.modal', function() { +# $('#shiny-modal textarea').select().focus(); +# }); +# $('#shiny-bookmark-copy-text') +# .text(function() { +# if (/Mac/i.test(navigator.userAgent)) { +# return 'Press \u2318-C to copy.'; +# } else { +# return 'Press Ctrl-C to copy.'; +# } +# }); +# " +# ) +# ) +# } + class BookmarkApp(Bookmark): _session: AppSession @@ -480,13 +635,12 @@ async def do_bookmark(self) -> None: with session_context(self._session): await self._on_bookmarked_callbacks.invoke(full_url) else: - # `session.bookmark.show_modal(url)` - - # showBookmarkUrlModal(url) + # Barret: # This action feels weird. I don't believe it should occur - # Instead, I believe it should update the query string automatically. - # `session.bookmark.update_query_string(url)` - raise NotImplementedError("Show bookmark modal not implemented") + # Instead, I believe it should update the query string and set the user's clipboard with a UI notification in the corner. + with session_context(self._session): + await self.show_bookmark_url_modal(full_url) + except Exception as e: msg = f"Error bookmarking state: {e}" from ..ui._notification import notification_show @@ -523,13 +677,14 @@ def __init__(self, session_proxy: SessionProxy): from ..session import session_context - @self._session._parent.bookmark.on_bookmarked - async def scoped_on_bookmarked(url: str) -> None: - if self._on_bookmarked_callbacks.count() == 0: - return - - with session_context(self._session): - await self._on_bookmarked_callbacks.invoke(url) + # # Disabling `on_bookmarked` within BookmarkProxy so we can tell when + # # the user has not registered a `on_bookmarked` callback. + # @self._session._parent.bookmark.on_bookmarked + # async def scoped_on_bookmarked(url: str) -> None: + # if self._on_bookmarked_callbacks.count() == 0: + # return + # with session_context(self._session): + # await self._on_bookmarked_callbacks.invoke(url) ns_prefix = str(self._ns + self._ns._sep) @@ -590,6 +745,12 @@ async def _scoped_on_bookmark(self, root_state: BookmarkState) -> None: raise ValueError("All scope values must be named.") root_state.values[str(self._ns(key))] = value + def on_bookmarked( + self, + callback: Callable[[str], None] | Callable[[str], Awaitable[None]], + ) -> NoReturn: + raise RuntimeError("`on_bookmarked` is not supported in BookmarkProxy.") + @property def store(self) -> BookmarkStore: return self._session._parent.bookmark.store @@ -676,65 +837,6 @@ def on_restored( return lambda: None -# #' Generate a modal dialog that displays a URL -# #' -# #' The modal dialog generated by `urlModal` will display the URL in a -# #' textarea input, and the URL text will be selected so that it can be easily -# #' copied. The result from `urlModal` should be passed to the -# #' [showModal()] function to display it in the browser. -# #' -# #' @param url A URL to display in the dialog box. -# #' @param title A title for the dialog box. -# #' @param subtitle Text to display underneath URL. -# #' @export -# urlModal <- function(url, title = "Bookmarked application link", subtitle = NULL) { - -# subtitleTag <- tagList( -# br(), -# span(class = "text-muted", subtitle), -# span(id = "shiny-bookmark-copy-text", class = "text-muted") -# ) - -# modalDialog( -# title = title, -# easyClose = TRUE, -# tags$textarea(class = "form-control", rows = "1", style = "resize: none;", -# readonly = "readonly", -# url -# ), -# subtitleTag, -# # Need separate show and shown listeners. The show listener sizes the -# # textarea just as the modal starts to fade in. The 200ms delay is needed -# # because if we try to resize earlier, it can't calculate the text height -# # (scrollHeight will be reported as zero). The shown listener selects the -# # text; it's needed because because selection has to be done after the fade- -# # in is completed. -# tags$script( -# "$('#shiny-modal'). -# one('show.bs.modal', function() { -# setTimeout(function() { -# var $textarea = $('#shiny-modal textarea'); -# $textarea.innerHeight($textarea[0].scrollHeight); -# }, 200); -# }); -# $('#shiny-modal') -# .one('shown.bs.modal', function() { -# $('#shiny-modal textarea').select().focus(); -# }); -# $('#shiny-bookmark-copy-text') -# .text(function() { -# if (/Mac/i.test(navigator.userAgent)) { -# return 'Press \u2318-C to copy.'; -# } else { -# return 'Press Ctrl-C to copy.'; -# } -# }); -# " -# ) -# ) -# } - - # #' Display a modal dialog for bookmarking # #' # #' This is a wrapper function for [urlModal()] that is automatically @@ -757,232 +859,3 @@ def on_restored( # showModal(urlModal(url, subtitle = subtitle)) # } - -# #' Enable bookmarking for a Shiny application -# #' -# #' @description -# #' -# #' There are two types of bookmarking: saving an application's state to disk on -# #' the server, and encoding the application's state in a URL. For state that has -# #' been saved to disk, the state can be restored with the corresponding state -# #' ID. For URL-encoded state, the state of the application is encoded in the -# #' URL, and no server-side storage is needed. -# #' -# #' URL-encoded bookmarking is appropriate for applications where there not many -# #' input values that need to be recorded. Some browsers have a length limit for -# #' URLs of about 2000 characters, and if there are many inputs, the length of -# #' the URL can exceed that limit. -# #' -# #' Saved-on-server bookmarking is appropriate when there are many inputs, or -# #' when the bookmarked state requires storing files. -# #' -# #' @details -# #' -# #' For restoring state to work properly, the UI must be a function that takes -# #' one argument, `request`. In most Shiny applications, the UI is not a -# #' function; it might have the form `fluidPage(....)`. Converting it to a -# #' function is as simple as wrapping it in a function, as in -# #' \code{function(request) \{ fluidPage(....) \}}. -# #' -# #' By default, all input values will be bookmarked, except for the values of -# #' passwordInputs. fileInputs will be saved if the state is saved on a server, -# #' but not if the state is encoded in a URL. -# #' -# #' When bookmarking state, arbitrary values can be stored, by passing a function -# #' as the `onBookmark` argument. That function will be passed a -# #' `ShinySaveState` object. The `values` field of the object is a list -# #' which can be manipulated to save extra information. Additionally, if the -# #' state is being saved on the server, and the `dir` field of that object -# #' can be used to save extra information to files in that directory. -# #' -# #' For saved-to-server state, this is how the state directory is chosen: -# #' \itemize{ -# #' \item If running in a hosting environment such as Shiny Server or -# #' Connect, the hosting environment will choose the directory. -# #' \item If running an app in a directory with [runApp()], the -# #' saved states will be saved in a subdirectory of the app called -# #' shiny_bookmarks. -# #' \item If running a Shiny app object that is generated from code (not run -# #' from a directory), the saved states will be saved in a subdirectory of -# #' the current working directory called shiny_bookmarks. -# #' } -# #' -# #' When used with [shinyApp()], this function must be called before -# #' `shinyApp()`, or in the `shinyApp()`'s `onStart` function. An -# #' alternative to calling the `enableBookmarking()` function is to use the -# #' `enableBookmarking` *argument* for `shinyApp()`. See examples -# #' below. -# #' -# #' @param store Either `"url"`, which encodes all of the relevant values in -# #' a URL, `"server"`, which saves to disk on the server, or -# #' `"disable"`, which disables any previously-enabled bookmarking. -# #' -# #' @seealso [onBookmark()], [onBookmarked()], -# #' [onRestore()], and [onRestored()] for registering -# #' callback functions that are invoked when the state is bookmarked or -# #' restored. -# #' -# #' Also see [updateQueryString()]. -# #' -# #' @export -# #' @examples -# #' ## Only run these examples in interactive R sessions -# #' if (interactive()) { -# #' -# #' # Basic example with state encoded in URL -# #' ui <- function(request) { -# #' fluidPage( -# #' textInput("txt", "Text"), -# #' checkboxInput("chk", "Checkbox"), -# #' bookmarkButton() -# #' ) -# #' } -# #' server <- function(input, output, session) { } -# #' enableBookmarking("url") -# #' shinyApp(ui, server) -# #' -# #' -# #' # An alternative to calling enableBookmarking(): use shinyApp's -# #' # enableBookmarking argument -# #' shinyApp(ui, server, enableBookmarking = "url") -# #' -# #' -# #' # Same basic example with state saved to disk -# #' enableBookmarking("server") -# #' shinyApp(ui, server) -# #' -# #' -# #' # Save/restore arbitrary values -# #' ui <- function(req) { -# #' fluidPage( -# #' textInput("txt", "Text"), -# #' checkboxInput("chk", "Checkbox"), -# #' bookmarkButton(), -# #' br(), -# #' textOutput("lastSaved") -# #' ) -# #' } -# #' server <- function(input, output, session) { -# #' vals <- reactiveValues(savedTime = NULL) -# #' output$lastSaved <- renderText({ -# #' if (!is.null(vals$savedTime)) -# #' paste("Last saved at", vals$savedTime) -# #' else -# #' "" -# #' }) -# #' -# #' onBookmark(function(state) { -# #' vals$savedTime <- Sys.time() -# #' # state is a mutable reference object, and we can add arbitrary values -# #' # to it. -# #' state$values$time <- vals$savedTime -# #' }) -# #' onRestore(function(state) { -# #' vals$savedTime <- state$values$time -# #' }) -# #' } -# #' enableBookmarking(store = "url") -# #' shinyApp(ui, server) -# #' -# #' -# #' # Usable with dynamic UI (set the slider, then change the text input, -# #' # click the bookmark button) -# #' ui <- function(request) { -# #' fluidPage( -# #' sliderInput("slider", "Slider", 1, 100, 50), -# #' uiOutput("ui"), -# #' bookmarkButton() -# #' ) -# #' } -# #' server <- function(input, output, session) { -# #' output$ui <- renderUI({ -# #' textInput("txt", "Text", input$slider) -# #' }) -# #' } -# #' enableBookmarking("url") -# #' shinyApp(ui, server) -# #' -# #' -# #' # Exclude specific inputs (The only input that will be saved in this -# #' # example is chk) -# #' ui <- function(request) { -# #' fluidPage( -# #' passwordInput("pw", "Password"), # Passwords are never saved -# #' sliderInput("slider", "Slider", 1, 100, 50), # Manually excluded below -# #' checkboxInput("chk", "Checkbox"), -# #' bookmarkButton() -# #' ) -# #' } -# #' server <- function(input, output, session) { -# #' setBookmarkExclude("slider") -# #' } -# #' enableBookmarking("url") -# #' shinyApp(ui, server) -# #' -# #' -# #' # Update the browser's location bar every time an input changes. This should -# #' # not be used with enableBookmarking("server"), because that would create a -# #' # new saved state on disk every time the user changes an input. -# #' ui <- function(req) { -# #' fluidPage( -# #' textInput("txt", "Text"), -# #' checkboxInput("chk", "Checkbox") -# #' ) -# #' } -# #' server <- function(input, output, session) { -# #' observe({ -# #' # Trigger this observer every time an input changes -# #' reactiveValuesToList(input) -# #' session$doBookmark() -# #' }) -# #' onBookmarked(function(url) { -# #' updateQueryString(url) -# #' }) -# #' } -# #' enableBookmarking("url") -# #' shinyApp(ui, server) -# #' -# #' -# #' # Save/restore uploaded files -# #' ui <- function(request) { -# #' fluidPage( -# #' sidebarLayout( -# #' sidebarPanel( -# #' fileInput("file1", "Choose CSV File", multiple = TRUE, -# #' accept = c( -# #' "text/csv", -# #' "text/comma-separated-values,text/plain", -# #' ".csv" -# #' ) -# #' ), -# #' tags$hr(), -# #' checkboxInput("header", "Header", TRUE), -# #' bookmarkButton() -# #' ), -# #' mainPanel( -# #' tableOutput("contents") -# #' ) -# #' ) -# #' ) -# #' } -# #' server <- function(input, output) { -# #' output$contents <- renderTable({ -# #' inFile <- input$file1 -# #' if (is.null(inFile)) -# #' return(NULL) -# #' -# #' if (nrow(inFile) == 1) { -# #' read.csv(inFile$datapath, header = input$header) -# #' } else { -# #' data.frame(x = "multiple files") -# #' } -# #' }) -# #' } -# #' enableBookmarking("server") -# #' shinyApp(ui, server) -# #' -# #' } -# enableBookmarking <- function(store = c("url", "server", "disable")) { -# store <- match.arg(store) -# shinyOptions(bookmarkStore = store) -# } From 2bab7430fcd0fbc016de56d0c74010e98a243909 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Fri, 14 Mar 2025 17:40:44 -0400 Subject: [PATCH 2/5] Cleanup tests --- .../bookmark/modules/app-core-recursive.py | 18 ---------- .../shiny/bookmark/modules/app-core.py | 29 ---------------- .../shiny/bookmark/modules/app-express.py | 33 ++----------------- 3 files changed, 3 insertions(+), 77 deletions(-) diff --git a/tests/playwright/shiny/bookmark/modules/app-core-recursive.py b/tests/playwright/shiny/bookmark/modules/app-core-recursive.py index a3dfaf70d..38a6ee3d2 100644 --- a/tests/playwright/shiny/bookmark/modules/app-core-recursive.py +++ b/tests/playwright/shiny/bookmark/modules/app-core-recursive.py @@ -55,7 +55,6 @@ def value(): @reactive.effect @reactive.event(input.btn1, input.btn2, input.dyn1, input.dyn2, ignore_init=True) async def _(): - # print("app-Bookmarking!") await session.bookmark() session.bookmark.exclude.append("btn2") @@ -68,7 +67,6 @@ def _(state: BookmarkState) -> None: @session.bookmark.on_restore def _(restore_state: RestoreState) -> None: - # print("app-Restore state:", restore_state.values) if "btn2" in restore_state.values: @@ -86,7 +84,6 @@ def _(restore_state: RestoreState) -> None: def app_ui(request: Request) -> ui.Tag: - # print("app-Making UI") return ui.page_fixed( ui.output_code("bookmark_store"), "Click Buttons to update bookmark", @@ -103,22 +100,7 @@ def server(input: Inputs, output: Outputs, session: Session): def bookmark_store(): return f"{session.bookmark.store}" - @session.bookmark.on_bookmark - async def on_bookmark(state: BookmarkState) -> None: - print( - "app-On Bookmark", - "\nInputs: ", - await state.input._serialize(exclude=state.exclude, state_dir=None), - "\nValues: ", - state.values, - "\n\n", - ) - # session.bookmark.update_query_string() - - pass - session.bookmark.on_bookmarked(session.bookmark.update_query_string) - # session.bookmark.on_bookmarked(session.bookmark.show_modal) SHINY_BOOKMARK_STORE: Literal["url", "server"] = os.getenv( diff --git a/tests/playwright/shiny/bookmark/modules/app-core.py b/tests/playwright/shiny/bookmark/modules/app-core.py index d39b1ada5..ed57f9cda 100644 --- a/tests/playwright/shiny/bookmark/modules/app-core.py +++ b/tests/playwright/shiny/bookmark/modules/app-core.py @@ -30,9 +30,6 @@ def mod_btn(idx: int): ui.output_ui("ui_html"), ui.output_code("value"), width="200px", - # fill=True, - # fillable=True, - # height="75px", ), ui.hr(), ) @@ -60,7 +57,6 @@ def value(): @reactive.effect @reactive.event(input.btn1, input.btn2, input.dyn1, input.dyn2, ignore_init=True) async def _(): - # print("app-Bookmarking!") await session.bookmark() session.bookmark.exclude.append("btn2") @@ -73,7 +69,6 @@ def _(state: BookmarkState) -> None: @session.bookmark.on_restore def _(restore_state: RestoreState) -> None: - # print("app-Restore state:", restore_state.values) if "btn2" in restore_state.values: @@ -88,15 +83,10 @@ def _(restore_state: RestoreState) -> None: def app_ui(request: Request) -> ui.Tag: - # print("app-Making UI") return ui.page_fixed( ui.output_code("bookmark_store"), "Click Button to update bookmark", - # ui.input_action_button("btn", "Button"), *[mod_btn(f"mod{i}", i) for i in reversed(range(k))], - # ui.input_radio_buttons("btn", "Button", choices=["a", "b", "c"], selected="a"), - # ui.output_code("code"), - # ui.input_bookmark_button(), ) @@ -110,26 +100,7 @@ def bookmark_store(): for i in reversed(range(k)): btn_server(f"mod{i}", i) - @session.bookmark.on_bookmark - async def on_bookmark(state: BookmarkState) -> None: - # print( - # "app-On Bookmark", - # "\nInputs: ", - # await state.input._serialize(exclude=state.exclude, state_dir=None), - # "\nValues: ", - # state.values, - # "\n\n", - # ) - # session.bookmark.update_query_string() - - pass - session.bookmark.on_bookmarked(session.bookmark.update_query_string) - # session.bookmark.on_bookmarked(session.bookmark.show_modal) - - # @render.code - # def code(): - # return f"{input.btn()}" SHINY_BOOKMARK_STORE: Literal["url", "server"] = os.getenv( diff --git a/tests/playwright/shiny/bookmark/modules/app-express.py b/tests/playwright/shiny/bookmark/modules/app-express.py index 8733cce22..8e6066d0c 100644 --- a/tests/playwright/shiny/bookmark/modules/app-express.py +++ b/tests/playwright/shiny/bookmark/modules/app-express.py @@ -89,33 +89,6 @@ def _(restore_state: RestoreState) -> None: recursive_mod(f"mod{i}", i) -# ui.input_radio_buttons("btn", "Button", choices=["a", "b", "c"], selected="a") - - -# @render.code -# def code(): -# return f"{input.btn()}" - - -# ui.input_bookmark_button() - - -# @session.bookmark.on_bookmark -# async def on_bookmark(state: BookmarkState) -> None: -# print( -# "app-On Bookmark", -# "\nInputs: ", -# await state.input._serialize(exclude=state.exclude, state_dir=None), -# "\nValues: ", -# state.values, -# "\n\n", -# ) -# # session.bookmark.update_query_string() - - -@session.bookmark.on_bookmarked -async def _(url: str): - await session.bookmark.update_query_string(url) - - -# session.bookmark.on_bookmarked(session.bookmark.show_modal) +# @session.bookmark.on_bookmarked +# async def _(url: str): +# await session.bookmark.update_query_string(url) From c7b788f4a30761d690d72340f023776b9743ab04 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 17 Mar 2025 15:10:23 -0400 Subject: [PATCH 3/5] Chain more modules in test app --- .../shiny/bookmark/modules/app-core-recursive.py | 11 ++++++----- .../shiny/bookmark/modules/app-core.py | 6 +++--- .../shiny/bookmark/modules/app-express.py | 16 ++++++++-------- .../bookmark/modules/test_bookmark_modules.py | 6 +++--- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/tests/playwright/shiny/bookmark/modules/app-core-recursive.py b/tests/playwright/shiny/bookmark/modules/app-core-recursive.py index 38a6ee3d2..0ea12baeb 100644 --- a/tests/playwright/shiny/bookmark/modules/app-core-recursive.py +++ b/tests/playwright/shiny/bookmark/modules/app-core-recursive.py @@ -29,7 +29,7 @@ def mod_btn(idx: int = 1): width="200px", ), ui.hr(), - mod_btn(f"sub{idx}", idx - 1) if idx > 0 else None, + mod_btn(f"sub{idx - 1}", idx - 1) if idx > 0 else None, ) @@ -77,10 +77,13 @@ def _(restore_state: RestoreState) -> None: ui.update_radio_buttons("dyn2", selected=restore_state.values["dyn2"]) if idx > 0: - btn_server(f"sub{idx}", idx - 1) + btn_server(f"sub{idx - 1}", idx - 1) + else: + # Attempt to call on_bookmarked at the very end of the proxy chain + session.bookmark.on_bookmarked(session.bookmark.update_query_string) -k = 2 +k = 4 def app_ui(request: Request) -> ui.Tag: @@ -100,8 +103,6 @@ def server(input: Inputs, output: Outputs, session: Session): def bookmark_store(): return f"{session.bookmark.store}" - session.bookmark.on_bookmarked(session.bookmark.update_query_string) - SHINY_BOOKMARK_STORE: Literal["url", "server"] = os.getenv( "SHINY_BOOKMARK_STORE", "url" diff --git a/tests/playwright/shiny/bookmark/modules/app-core.py b/tests/playwright/shiny/bookmark/modules/app-core.py index ed57f9cda..c293380e4 100644 --- a/tests/playwright/shiny/bookmark/modules/app-core.py +++ b/tests/playwright/shiny/bookmark/modules/app-core.py @@ -79,7 +79,7 @@ def _(restore_state: RestoreState) -> None: ui.update_radio_buttons("dyn2", selected=restore_state.values["dyn2"]) -k = 2 +k = 4 def app_ui(request: Request) -> ui.Tag: @@ -93,6 +93,8 @@ def app_ui(request: Request) -> ui.Tag: # Needs access to the restore context to the dynamic UI def server(input: Inputs, output: Outputs, session: Session): + session.bookmark.on_bookmarked(session.bookmark.update_query_string) + @render.code def bookmark_store(): return f"{session.bookmark.store}" @@ -100,8 +102,6 @@ def bookmark_store(): for i in reversed(range(k)): btn_server(f"mod{i}", i) - session.bookmark.on_bookmarked(session.bookmark.update_query_string) - SHINY_BOOKMARK_STORE: Literal["url", "server"] = os.getenv( "SHINY_BOOKMARK_STORE", "url" diff --git a/tests/playwright/shiny/bookmark/modules/app-express.py b/tests/playwright/shiny/bookmark/modules/app-express.py index 8e6066d0c..941bf918a 100644 --- a/tests/playwright/shiny/bookmark/modules/app-express.py +++ b/tests/playwright/shiny/bookmark/modules/app-express.py @@ -15,13 +15,18 @@ app_opts(bookmark_store=SHINY_BOOKMARK_STORE) +@session.bookmark.on_bookmarked +async def _(url: str): + await session.bookmark.update_query_string(url) + + @render.code def bookmark_store(): return f"{session.bookmark.store}" @module -def recursive_mod(input: Inputs, output: Outputs, session: Session, recurse: int = 3): +def ex_mod(input: Inputs, output: Outputs, session: Session, recurse: int = 3): ui.h3(f"Module {recurse}") with ui.layout_column_wrap(width="200px"): @@ -84,11 +89,6 @@ def _(restore_state: RestoreState) -> None: "Click Button to update bookmark" -k = 2 +k = 4 for i in reversed(range(k)): - recursive_mod(f"mod{i}", i) - - -# @session.bookmark.on_bookmarked -# async def _(url: str): -# await session.bookmark.update_query_string(url) + ex_mod(f"mod{i}", i) diff --git a/tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py b/tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py index 31306d9cc..52c596aa6 100644 --- a/tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py +++ b/tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py @@ -12,11 +12,11 @@ "app_name,mod0_key,mod1_key", [ # Express mode - ("app-express.py", "mod0", "mod1"), + ("app-express.py", "mod0", "mod3"), # Core mode - ("app-core.py", "mod0", "mod1"), + ("app-core.py", "mod0", "mod3"), # Recursive modules within core mode - ("app-core-recursive.py", "mod1-sub1", "mod1"), + ("app-core-recursive.py", "mod3", "mod3-sub2-sub1-sub0"), ], ) @pytest.mark.parametrize("bookmark_store", ["url", "server"]) From 44e48116b7baa5a0c11bcd8a54f174dd48101e4c Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 17 Mar 2025 16:27:53 -0400 Subject: [PATCH 4/5] BookmarkProxy classes will not register any handlers until requested --- shiny/bookmark/_bookmark.py | 239 ++++++++++++++-------------------- shiny/bookmark/_save_state.py | 2 - 2 files changed, 95 insertions(+), 146 deletions(-) diff --git a/shiny/bookmark/_bookmark.py b/shiny/bookmark/_bookmark.py index 37b9b2b53..2e8832733 100644 --- a/shiny/bookmark/_bookmark.py +++ b/shiny/bookmark/_bookmark.py @@ -3,9 +3,7 @@ import warnings from abc import ABC, abstractmethod from pathlib import Path -from typing import TYPE_CHECKING, Awaitable, Callable, Literal, NoReturn, Optional - -from htmltools import TagChild +from typing import TYPE_CHECKING, Awaitable, Callable, Literal from .._docstring import add_example from .._utils import AsyncCallbacks, CancelCallback, wrap_async @@ -34,6 +32,7 @@ class Bookmark(ABC): _on_get_exclude: list[Callable[[], list[str]]] """Callbacks that BookmarkProxy classes utilize to help determine the list of inputs to exclude from bookmarking.""" + exclude: list[str] """A list of scoped Input names to exclude from bookmarking.""" @@ -129,8 +128,6 @@ def on_bookmarked( This callback will be executed **after** the bookmark state is saved serverside or in the URL. - Note: `.on_bookmarked()` can only be added to the `AppSession` object, not on a a module's session. - Parameters ---------- callback @@ -138,6 +135,7 @@ def on_bookmarked( should accept a single argument, the string representing the query parameter component of the URL. """ + return self._on_bookmarked_callbacks.register(wrap_async(callback)) def on_restore( @@ -313,65 +311,6 @@ async def show_bookmark_url_modal( return -# #' Generate a modal dialog that displays a URL -# #' -# #' The modal dialog generated by `urlModal` will display the URL in a -# #' textarea input, and the URL text will be selected so that it can be easily -# #' copied. The result from `urlModal` should be passed to the -# #' [showModal()] function to display it in the browser. -# #' -# #' @param url A URL to display in the dialog box. -# #' @param title A title for the dialog box. -# #' @param subtitle Text to display underneath URL. -# #' @export -# urlModal <- function(url, title = "Bookmarked application link", subtitle = NULL) { - -# subtitleTag <- tagList( -# br(), -# span(class = "text-muted", subtitle), -# span(id = "shiny-bookmark-copy-text", class = "text-muted") -# ) - -# modalDialog( -# title = title, -# easyClose = TRUE, -# tags$textarea(class = "form-control", rows = "1", style = "resize: none;", -# readonly = "readonly", -# url -# ), -# subtitleTag, -# # Need separate show and shown listeners. The show listener sizes the -# # textarea just as the modal starts to fade in. The 200ms delay is needed -# # because if we try to resize earlier, it can't calculate the text height -# # (scrollHeight will be reported as zero). The shown listener selects the -# # text; it's needed because because selection has to be done after the fade- -# # in is completed. -# tags$script( -# "$('#shiny-modal'). -# one('show.bs.modal', function() { -# setTimeout(function() { -# var $textarea = $('#shiny-modal textarea'); -# $textarea.innerHeight($textarea[0].scrollHeight); -# }, 200); -# }); -# $('#shiny-modal') -# .one('shown.bs.modal', function() { -# $('#shiny-modal textarea').select().focus(); -# }); -# $('#shiny-bookmark-copy-text') -# .text(function() { -# if (/Mac/i.test(navigator.userAgent)) { -# return 'Press \u2318-C to copy.'; -# } else { -# return 'Press Ctrl-C to copy.'; -# } -# }); -# " -# ) -# ) -# } - - class BookmarkApp(Bookmark): _root_session: AppSession """ @@ -507,38 +446,6 @@ async def invoke_on_restored_callbacks(): return - def on_bookmark( - self, - callback: ( - Callable[[BookmarkState], None] | Callable[[BookmarkState], Awaitable[None]] - ), - /, - ) -> CancelCallback: - return self._on_bookmark_callbacks.register(wrap_async(callback)) - - def on_bookmarked( - self, - callback: Callable[[str], None] | Callable[[str], Awaitable[None]], - /, - ) -> CancelCallback: - return self._on_bookmarked_callbacks.register(wrap_async(callback)) - - def on_restore( - self, - callback: ( - Callable[[RestoreState], None] | Callable[[RestoreState], Awaitable[None]] - ), - ) -> CancelCallback: - return self._on_restore_callbacks.register(wrap_async(callback)) - - def on_restored( - self, - callback: ( - Callable[[RestoreState], None] | Callable[[RestoreState], Awaitable[None]] - ), - ) -> CancelCallback: - return self._on_restored_callbacks.register(wrap_async(callback)) - async def update_query_string( self, query_string: str, @@ -564,7 +471,7 @@ def _get_bookmark_exclude(self) -> list[str]: for proxy_exclude_fn in self._on_get_exclude: scoped_excludes.extend(proxy_exclude_fn()) # Remove duplicates - return list(set([*self.exclude, *scoped_excludes])) + return [str(name) for name in set([*self.exclude, *scoped_excludes])] async def get_bookmark_url(self) -> str | None: if self.store == "disable": @@ -644,7 +551,7 @@ async def do_bookmark(self) -> None: # Barret: # This action feels weird. I don't believe it should occur # Instead, I believe it should update the query string and set the user's clipboard with a UI notification in the corner. - with session_context(self._session): + with session_context(self._root_session): await self.show_bookmark_url_modal(full_url) except Exception as e: @@ -674,56 +581,23 @@ def __init__(self, session_proxy: SessionProxy): self._proxy_session = session_proxy self._root_session = session_proxy._root_session - # Maybe `._get_bookmark_exclude()` should be used instead of`proxy_exclude_fns`? + # Be sure root bookmark has access to proxy's excluded values self._root_bookmark._on_get_exclude.append( lambda: [str(self._ns(name)) for name in self.exclude] ) - # When scope is created, register these bookmarking callbacks on the main - # session object. They will invoke the scope's own callbacks, if any are - # present. - - # The goal of this method is to save the scope's values. All namespaced inputs - # will already exist within the `root_state`. - self._root_bookmark.on_bookmark(self._scoped_on_bookmark) - - from ..session import session_context - - # # Disabling `on_bookmarked` within BookmarkProxy so we can tell when - # # the user has not registered a `on_bookmarked` callback. - # @self._root_bookmark.on_bookmarked - # async def scoped_on_bookmarked(url: str) -> None: - # if self._on_bookmarked_callbacks.count() == 0: - # return - # with session_context(self._proxy_session): - # await self._on_bookmarked_callbacks.invoke(url) - - ns_prefix = str(self._ns + self._ns._sep) - - @self._root_bookmark.on_restore - async def scoped_on_restore(restore_state: RestoreState) -> None: - if self._on_restore_callbacks.count() == 0: - return - - scoped_restore_state = restore_state._state_within_namespace(ns_prefix) - - with session_context(self._proxy_session): - await self._on_restore_callbacks.invoke(scoped_restore_state) - - @self._root_bookmark.on_restored - async def scoped_on_restored(restore_state: RestoreState) -> None: - if self._on_restored_callbacks.count() == 0: - return + # Note: This proxy bookmark class will not register a handler (`on_bookmark`, + # `on_bookmarked`, `on_restore`, `on_restored`) until one is requested either by + # the app author or a sub-proxy bookmark class + # These function are utilized - scoped_restore_state = restore_state._state_within_namespace(ns_prefix) - with session_context(self._proxy_session): - await self._on_restored_callbacks.invoke(scoped_restore_state) + @property + def _ns_prefix(self) -> str: + return str(self._ns + self._ns._sep) + # The goal of this method is to save the scope's values. All namespaced inputs + # will already exist within the `root_state`. async def _scoped_on_bookmark(self, root_state: BookmarkState) -> None: - # Exit if no user-defined callbacks. - if self._on_bookmark_callbacks.count() == 0: - return - from ..bookmark._bookmark import BookmarkState scoped_state = BookmarkState( @@ -737,6 +611,12 @@ async def _scoped_on_bookmark(self, root_state: BookmarkState) -> None: if root_state.dir is not None: scope_subpath = self._ns scoped_state.dir = Path(root_state.dir) / scope_subpath + # Barret - 2025-03-17: + # Having Shiny make this directory feels like the wrong thing to do here. + # Feels like we should be using the App's bookmark_save_dir function to + # determine where to save the bookmark state. + # However, R-Shiny currently creates the directory: + # https://github.com/rstudio/shiny/blob/f55c26af4a0493b082d2967aca6d36b90795adf1/R/shiny.R#L940 scoped_state.dir.mkdir(parents=True, exist_ok=True) if not scoped_state.dir.exists(): @@ -744,7 +624,6 @@ async def _scoped_on_bookmark(self, root_state: BookmarkState) -> None: f"Scope directory could not be created for {scope_subpath}" ) - # Invoke the callback on the scopeState object from ..session import session_context with session_context(self._proxy_session): @@ -757,11 +636,83 @@ async def _scoped_on_bookmark(self, root_state: BookmarkState) -> None: raise ValueError("All scope values must be named.") root_state.values[str(self._ns(key))] = value + def on_bookmark( + self, + callback: ( + Callable[[BookmarkState], None] | Callable[[BookmarkState], Awaitable[None]] + ), + ) -> CancelCallback: + if self._on_bookmark_callbacks.count() == 0: + # Register a proxy callback on the parent session + self._root_bookmark.on_bookmark(self._scoped_on_bookmark) + return super().on_bookmark(callback) + def on_bookmarked( self, callback: Callable[[str], None] | Callable[[str], Awaitable[None]], - ) -> NoReturn: - raise RuntimeError("`on_bookmarked` is not supported in BookmarkProxy.") + ) -> CancelCallback: + if self._on_bookmarked_callbacks.count() == 0: + from ..session import session_context + + # Register a proxy callback on the parent session + @self._root_bookmark.on_bookmarked + async def scoped_on_bookmarked(url: str) -> None: + if self._on_bookmarked_callbacks.count() == 0: + return + with session_context(self._proxy_session): + await self._on_bookmarked_callbacks.invoke(url) + + return super().on_bookmarked(callback) + + def on_restore( + self, + callback: ( + Callable[[RestoreState], None] | Callable[[RestoreState], Awaitable[None]] + ), + ) -> CancelCallback: + + if self._on_restore_callbacks.count() == 0: + from ..session import session_context + + # Register a proxy callback on the parent session + @self._root_bookmark.on_restore + async def scoped_on_restore(restore_state: RestoreState) -> None: + if self._on_restore_callbacks.count() == 0: + return + + scoped_restore_state = restore_state._state_within_namespace( + self._ns_prefix + ) + + with session_context(self._proxy_session): + await self._on_restore_callbacks.invoke(scoped_restore_state) + + return super().on_restore(callback) + + def on_restored( + self, + callback: ( + Callable[[RestoreState], None] | Callable[[RestoreState], Awaitable[None]] + ), + ) -> CancelCallback: + + if self._on_restored_callbacks.count() == 0: + from ..session import session_context + + # Register a proxy callback on the parent session + @self._root_bookmark.on_restored + async def scoped_on_restored(restore_state: RestoreState) -> None: + if self._on_restored_callbacks.count() == 0: + return + + scoped_restore_state = restore_state._state_within_namespace( + self._ns_prefix + ) + + with session_context(self._proxy_session): + await self._on_restored_callbacks.invoke(scoped_restore_state) + + return super().on_restored(callback) @property def store(self) -> BookmarkStore: diff --git a/shiny/bookmark/_save_state.py b/shiny/bookmark/_save_state.py index 20f7ae26f..15789ef09 100644 --- a/shiny/bookmark/_save_state.py +++ b/shiny/bookmark/_save_state.py @@ -43,8 +43,6 @@ def __init__( self.dir = None # This will be set by external functions. self.values = {} - self._always_exclude: list[str] = [] - async def _call_on_save(self): # Allow user-supplied save function to do things like add state$values, or # save data to state dir. From dd8c8bcc954e052b9d8cb5b70d32e234409a3b4b Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 17 Mar 2025 16:38:47 -0400 Subject: [PATCH 5/5] Test that modal shows up and the url isn't updated --- tests/playwright/shiny/bookmark/modal/app.py | 20 ++++++++++++++++++ .../bookmark/modal/test_bookmark_modal.py | 21 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 tests/playwright/shiny/bookmark/modal/app.py create mode 100644 tests/playwright/shiny/bookmark/modal/test_bookmark_modal.py diff --git a/tests/playwright/shiny/bookmark/modal/app.py b/tests/playwright/shiny/bookmark/modal/app.py new file mode 100644 index 000000000..68f66c4f3 --- /dev/null +++ b/tests/playwright/shiny/bookmark/modal/app.py @@ -0,0 +1,20 @@ +from starlette.requests import Request + +from shiny import App, Inputs, Outputs, Session, reactive, ui + + +def app_ui(request: Request): + return ui.page_fluid( + ui.input_radio_buttons("letter", "Choose a letter", choices=["A", "B", "C"]), + ) + + +def server(input: Inputs, ouput: Outputs, session: Session): + + @reactive.effect + @reactive.event(input.letter, ignore_init=True) + async def _(): + await session.bookmark() + + +app = App(app_ui, server, bookmark_store="url") diff --git a/tests/playwright/shiny/bookmark/modal/test_bookmark_modal.py b/tests/playwright/shiny/bookmark/modal/test_bookmark_modal.py new file mode 100644 index 000000000..e3d8fbf41 --- /dev/null +++ b/tests/playwright/shiny/bookmark/modal/test_bookmark_modal.py @@ -0,0 +1,21 @@ +import re + +from playwright.sync_api import Page, expect + +from shiny.playwright.controller import InputRadioButtons +from shiny.run import ShinyAppProc + + +def test_bookmark_modules(page: Page, local_app: ShinyAppProc): + + page.goto(local_app.url) + + letter = InputRadioButtons(page, "letter") + letter.expect_selected("A") + letter.set("C") + + expect(page.locator("div.modal-body > textarea")).to_have_value( + re.compile(r"letter=%22C%22") + ) + + assert "?" not in page.url