BookmarkProxy classes will not register any handlers until requested

schloerke · schloerke · commit 44e48116b7ba · 2025-03-17T16:27:53.000-04:00
diff --git 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,15 +128,14 @@ 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
             The callback function to call when the session is bookmarked. This method
             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,14 +611,19 @@ 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():
                 raise FileNotFoundError(
                     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
@@ -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.
