From faa3ba864a5fb19952d285390cf0d107d6d52ac0 Mon Sep 17 00:00:00 2001 From: AntoinePrv Date: Mon, 25 May 2026 16:48:24 +0200 Subject: [PATCH 1/2] Add Active transation holder --- R/widget.R | 105 +++++++++++++++++++++++++++++++---- man/YAttrStorage.Rd | 8 ++- man/YRootStorage.Rd | 8 ++- tests/testthat/test-widget.R | 4 ++ 4 files changed, 113 insertions(+), 12 deletions(-) diff --git a/R/widget.R b/R/widget.R index 4f6431b..808d968 100644 --- a/R/widget.R +++ b/R/widget.R @@ -15,6 +15,22 @@ REMOTE_ORIGIN <- NULL if (inherits(value, "Prelim")) value else yr::Prelim$any(value) } +#' Holds the currently-active `yr::Transaction`, or NULL. Shared by a +#' WidgetBase and its storages so storage operations can join an ongoing +#' transaction instead of opening a nested one. +#' +#' We need this for syntaxix sugar because the getter/setter on widget work +#' without function calls (as Python property) so there is no alternative place +#' to pass a transaction argument. +#' +#' @noRd +TransactionState <- R6::R6Class( + "TransactionState", + public = list( + transaction = NULL + ) +) + #' Shared base for `YAttrWidget` and `YRootWidget` #' #' Owns the `yr::Doc` and the per-attribute storage registry, and provides @@ -33,6 +49,7 @@ WidgetBase <- R6::R6Class( initialize = function(ydoc = NULL) { self$ydoc <- if (is.null(ydoc)) yr::Doc$new() else ydoc private$.storages <- list() + private$.active_transaction <- TransactionState$new() }, #' @description Subscribe callbacks to attribute changes by name. Each @@ -60,7 +77,50 @@ WidgetBase <- R6::R6Class( ), private = list( - .storages = NULL + .storages = NULL, + .active_transaction = NULL, + + # Run `fn(trans)` inside a `with_transaction`, exposing `trans` as the + # active transaction to storages for the duration of the call. + with_active_transaction = function(fn, ...) { + state <- private$.active_transaction + self$ydoc$with_transaction( + function(trans) { + state$transaction <- trans + on.exit(state$transaction <- NULL) + fn(trans) + }, + ... + ) + } + ) +) + +#' Storage mixin that joins an ongoing transaction when one is active. +#' Holds a TransactionState shared with the owning widget and exposes a +#' private with_transaction() that runs fn(trans) on the active transaction +#' when set, and otherwise opens a fresh one on ydoc. +#' @noRd +YActiveTransactionStorage <- R6::R6Class( + "YActiveTransactionStorage", + private = list( + ydoc = NULL, + active_transaction = NULL, + + with_transaction = function(fn, mutable = FALSE, origin = NULL) { + active <- private$active_transaction$transaction + if (!is.null(active)) { + return(fn(active)) + } + private$ydoc$with_transaction(fn, mutable = mutable, origin = origin) + } + ), + + public = list( + initialize = function(ydoc, active_transaction) { + private$ydoc <- ydoc + private$active_transaction <- active_transaction + } ) ) @@ -74,8 +134,8 @@ WidgetBase <- R6::R6Class( #' @export YAttrStorage <- R6::R6Class( "YAttrStorage", + inherit = YActiveTransactionStorage, private = list( - ydoc = NULL, attrs = "_attrs", key = NULL ), @@ -89,10 +149,19 @@ YAttrStorage <- R6::R6Class( #' @param ydoc The `yr::Doc`. #' @param attrs Its attribute map. #' @param key Attribute key to read/write. + #' @param active_transaction A [TransactionState] shared with the owning + #' widget; when its `transaction` is non-NULL, storage reads/writes join + #' that transaction instead of opening a new one. #' @param default Default value (Prelim or any R value), or `NULL` to #' skip the initial write entirely. - initialize = function(ydoc, attrs, key, default = NULL) { - private$ydoc <- ydoc + initialize = function( + ydoc, + attrs, + key, + active_transaction, + default = NULL + ) { + super$initialize(ydoc, active_transaction) private$attrs <- attrs private$key <- key self$remote_changed <- Signal$new() @@ -101,7 +170,7 @@ YAttrStorage <- R6::R6Class( # It may already be present if joining another widget. if (!is.null(default)) { prelim_default <- .as_prelim(default) - private$ydoc$with_transaction( + private$with_transaction( function(trans) { if (is.null(private$attrs$get(trans, private$key))) { private$attrs$insert(trans, private$key, prelim_default) @@ -115,7 +184,7 @@ YAttrStorage <- R6::R6Class( #' @description Return the value stored under `key`. read = function() { - private$ydoc$with_transaction( + private$with_transaction( function(trans) private$attrs$get(trans, private$key) ) }, @@ -124,7 +193,7 @@ YAttrStorage <- R6::R6Class( #' @param value New value. #' @return `TRUE` iff the value was written. update = function(value) { - private$ydoc$with_transaction( + private$with_transaction( function(trans) { if (identical(private$attrs$get(trans, private$key), value)) { return(FALSE) @@ -186,7 +255,13 @@ YAttrWidget <- R6::R6Class( #' @param default Default value (Prelim or any R value). #' @return The newly created [YAttrStorage]. register_storage = function(name, default) { - storage <- YAttrStorage$new(self$ydoc, private$.attrs, name, default) + storage <- YAttrStorage$new( + self$ydoc, + private$.attrs, + name, + private$.active_transaction, + default + ) private$.storages[[name]] <- storage storage } @@ -208,6 +283,7 @@ YAttrWidget <- R6::R6Class( #' @export YRootStorage <- R6::R6Class( "YRootStorage", + inherit = YActiveTransactionStorage, private = list( ref = NULL, @@ -233,10 +309,14 @@ YRootStorage <- R6::R6Class( #' @param name Root name on the doc. #' @param prelim A `yr::Prelim` whose `is_text/is_map/is_array` selects #' the root kind. Content is ignored. - initialize = function(ydoc, name, prelim) { + #' @param active_transaction A [TransactionState] shared with the owning + #' widget. Stored for symmetry with [YAttrStorage]; root reads/writes + #' currently go through the ref directly and do not consult it. + initialize = function(ydoc, name, prelim, active_transaction) { if (!inherits(prelim, "Prelim")) { stop("YRootStorage requires a yr::Prelim for '", name, "'.") } + super$initialize(ydoc, active_transaction) private$ref <- private$insert_root(ydoc, name, prelim) self$remote_changed <- Signal$new() sig <- self$remote_changed @@ -287,7 +367,12 @@ YRootWidget <- R6::R6Class( #' @param prelim A `yr::Prelim` whose kind selects the root type. #' @return The newly created [YRootStorage]. register_storage = function(name, prelim) { - storage <- YRootStorage$new(self$ydoc, name, prelim) + storage <- YRootStorage$new( + self$ydoc, + name, + prelim, + private$.active_transaction + ) private$.storages[[name]] <- storage storage } diff --git a/man/YAttrStorage.Rd b/man/YAttrStorage.Rd index 40825ef..39c3c98 100644 --- a/man/YAttrStorage.Rd +++ b/man/YAttrStorage.Rd @@ -9,6 +9,9 @@ and a public `remote_changed` [Signal]). Writes via `update()` use `LOCAL_ORIGIN`; the owning [YAttrWidget] runs the `_attrs` observer and emits on `remote_changed` when a peer changes this key. } +\section{Super class}{ +\code{YActiveTransactionStorage} -> \code{YAttrStorage} +} \section{Public fields}{ \if{html}{\out{
}} \describe{ @@ -34,7 +37,7 @@ and a public `remote_changed` [Signal]). Writes via `update()` use it (a non-Prelim is wrapped as `yr::Prelim$any`). \subsection{Usage}{ \if{html}{\out{
}} - \preformatted{YAttrStorage$new(ydoc, attrs, key, default = NULL)} + \preformatted{YAttrStorage$new(ydoc, attrs, key, active_transaction, default = NULL)} \if{html}{\out{
}} } \subsection{Arguments}{ @@ -43,6 +46,9 @@ and a public `remote_changed` [Signal]). Writes via `update()` use \item{\code{ydoc}}{The `yr::Doc`.} \item{\code{attrs}}{Its attribute map.} \item{\code{key}}{Attribute key to read/write.} + \item{\code{active_transaction}}{A [TransactionState] shared with the owning +widget; when its `transaction` is non-NULL, storage reads/writes join +that transaction instead of opening a new one.} \item{\code{default}}{Default value (Prelim or any R value), or `NULL` to skip the initial write entirely.} } diff --git a/man/YRootStorage.Rd b/man/YRootStorage.Rd index f6eed21..37e261b 100644 --- a/man/YRootStorage.Rd +++ b/man/YRootStorage.Rd @@ -10,6 +10,9 @@ type is consulted; the Prelim's content is not written). `read()` returns the ref; `update()` is unsupported — callers mutate the ref directly. Remote (non-`LOCAL_ORIGIN`) changes fire `remote_changed`. } +\section{Super class}{ +\code{YActiveTransactionStorage} -> \code{YRootStorage} +} \section{Public fields}{ \if{html}{\out{
}} \describe{ @@ -33,7 +36,7 @@ Remote (non-`LOCAL_ORIGIN`) changes fire `remote_changed`. Materialize the root ref for `name` from `prelim`'s type. \subsection{Usage}{ \if{html}{\out{
}} - \preformatted{YRootStorage$new(ydoc, name, prelim)} + \preformatted{YRootStorage$new(ydoc, name, prelim, active_transaction)} \if{html}{\out{
}} } \subsection{Arguments}{ @@ -43,6 +46,9 @@ Remote (non-`LOCAL_ORIGIN`) changes fire `remote_changed`. \item{\code{name}}{Root name on the doc.} \item{\code{prelim}}{A `yr::Prelim` whose `is_text/is_map/is_array` selects the root kind. Content is ignored.} + \item{\code{active_transaction}}{A [TransactionState] shared with the owning +widget. Stored for symmetry with [YAttrStorage]; root reads/writes +currently go through the ref directly and do not consult it.} } \if{html}{\out{
}} } diff --git a/tests/testthat/test-widget.R b/tests/testthat/test-widget.R index be5e56e..0d77f13 100644 --- a/tests/testthat/test-widget.R +++ b/tests/testthat/test-widget.R @@ -114,6 +114,10 @@ mutate_in_place <- function(w, name, value) { ) } +# TODO issue that to mutate we need to with_transaction (not idomatic and +# needs privzate ydoc) plus in mutable transaction case, we also need an +# inner transaction for Storage read (though not in the case of Rootdoc) +# so potential deadlock. flavors <- list( list( name = "YAttrWidget (assign)", From 1acda8372c5dd588ea2bce3083901098f8e1cd47 Mon Sep 17 00:00:00 2001 From: AntoinePrv Date: Mon, 25 May 2026 17:55:26 +0200 Subject: [PATCH 2/2] Add with_read with_write to widgets --- R/widget.R | 19 ++++++++++++++ man/CommAttrWidget.Rd | 4 ++- man/CommRootWidget.Rd | 4 ++- man/WidgetBase.Rd | 43 +++++++++++++++++++++++++++++++ man/YAttrWidget.Rd | 2 ++ man/YRootWidget.Rd | 2 ++ tests/testthat/test-widget.R | 50 +++++++++++++++--------------------- 7 files changed, 92 insertions(+), 32 deletions(-) diff --git a/R/widget.R b/R/widget.R index 808d968..1b1a586 100644 --- a/R/widget.R +++ b/R/widget.R @@ -73,6 +73,25 @@ WidgetBase <- R6::R6Class( #' @param ... Subclass-specific arguments (e.g. `default` or `prelim`). register_storage = function(name, ...) { stop("register_storage() must be implemented by a subclass.") + }, + + #' @description Run `fn(trans)` inside a read-only transaction, exposing + #' `trans` as the active transaction so storage reads join it. + #' @param fn Function called with the transaction. + with_read = function(fn) { + private$with_active_transaction( + fn, + mutable = FALSE, + origin = LOCAL_ORIGIN + ) + }, + + #' @description Run `fn(trans)` inside a writable transaction tagged with + #' `LOCAL_ORIGIN`, exposing `trans` as the active transaction so storage + #' writes join it. + #' @param fn Function called with the transaction. + with_write = function(fn) { + private$with_active_transaction(fn, mutable = TRUE, origin = LOCAL_ORIGIN) } ), diff --git a/man/CommAttrWidget.Rd b/man/CommAttrWidget.Rd index 9fe77d2..8173d91 100644 --- a/man/CommAttrWidget.Rd +++ b/man/CommAttrWidget.Rd @@ -17,9 +17,11 @@ protocol on top of [YAttrWidget]'s map-attr storage. Constructor: \item \href{#method-CommAttrWidget-clone}{\code{CommAttrWidget$clone()}} } } -\if{html}{\out{
Inherited methods +\if{html}{\out{
Inherited methods
  • WidgetBase$connect()
    • +
  • WidgetBase$with_read()
    • +
  • WidgetBase$with_write()
  • YAttrWidget$register_storage()
  • .CommAttrWidgetBase$comm_id()
  • .CommAttrWidgetBase$initialize()
    • diff --git a/man/CommRootWidget.Rd b/man/CommRootWidget.Rd index 1f1cf9d..3ba8de5 100644 --- a/man/CommRootWidget.Rd +++ b/man/CommRootWidget.Rd @@ -16,9 +16,11 @@ Constructor: `CommRootWidget$new(ydoc = NULL, comm_metadata = NULL)`. \item \href{#method-CommRootWidget-clone}{\code{CommRootWidget$clone()}} } } -\if{html}{\out{
    Inherited methods +\if{html}{\out{
    Inherited methods
    • WidgetBase$connect()
      • +
    • WidgetBase$with_read()
      • +
    • WidgetBase$with_write()
    • YRootWidget$register_storage()
    • .CommRootWidgetBase$comm_id()
    • .CommRootWidgetBase$initialize()
      • diff --git a/man/WidgetBase.Rd b/man/WidgetBase.Rd index 913935d..fa9d622 100644 --- a/man/WidgetBase.Rd +++ b/man/WidgetBase.Rd @@ -21,6 +21,8 @@ override `register_storage()` to materialize their flavor of storage. \item \href{#method-WidgetBase-initialize}{\code{WidgetBase$new()}} \item \href{#method-WidgetBase-connect}{\code{WidgetBase$connect()}} \item \href{#method-WidgetBase-register_storage}{\code{WidgetBase$register_storage()}} + \item \href{#method-WidgetBase-with_read}{\code{WidgetBase$with_read()}} + \item \href{#method-WidgetBase-with_write}{\code{WidgetBase$with_write()}} \item \href{#method-WidgetBase-clone}{\code{WidgetBase$clone()}} } } @@ -88,6 +90,47 @@ override `register_storage()` to materialize their flavor of storage. } } +\if{html}{\out{
      }} +\if{html}{\out{}} +\if{latex}{\out{\hypertarget{method-WidgetBase-with_read}{}}} +\subsection{\code{WidgetBase$with_read()}}{ + Run `fn(trans)` inside a read-only transaction, exposing + `trans` as the active transaction so storage reads join it. + \subsection{Usage}{ + \if{html}{\out{
      }} + \preformatted{WidgetBase$with_read(fn)} + \if{html}{\out{
      }} + } + \subsection{Arguments}{ + \if{html}{\out{
      }} + \describe{ + \item{\code{fn}}{Function called with the transaction.} + } + \if{html}{\out{
      }} + } +} + +\if{html}{\out{
      }} +\if{html}{\out{}} +\if{latex}{\out{\hypertarget{method-WidgetBase-with_write}{}}} +\subsection{\code{WidgetBase$with_write()}}{ + Run `fn(trans)` inside a writable transaction tagged with + `LOCAL_ORIGIN`, exposing `trans` as the active transaction so storage + writes join it. + \subsection{Usage}{ + \if{html}{\out{
      }} + \preformatted{WidgetBase$with_write(fn)} + \if{html}{\out{
      }} + } + \subsection{Arguments}{ + \if{html}{\out{
      }} + \describe{ + \item{\code{fn}}{Function called with the transaction.} + } + \if{html}{\out{
      }} + } +} + \if{html}{\out{
      }} \if{html}{\out{}} \if{latex}{\out{\hypertarget{method-WidgetBase-clone}{}}} diff --git a/man/YAttrWidget.Rd b/man/YAttrWidget.Rd index 5c082b9..f1d5f2e 100644 --- a/man/YAttrWidget.Rd +++ b/man/YAttrWidget.Rd @@ -22,6 +22,8 @@ A single `_attrs` observer dispatches remote changes to each storage's \if{html}{\out{
      Inherited methods
      • WidgetBase$connect()
        • +
      • WidgetBase$with_read()
        • +
      • WidgetBase$with_write()
      }} \if{html}{\out{
      }} diff --git a/man/YRootWidget.Rd b/man/YRootWidget.Rd index 224a98b..9414b93 100644 --- a/man/YRootWidget.Rd +++ b/man/YRootWidget.Rd @@ -23,6 +23,8 @@ root carries its own observer that emits the attribute's
      • WidgetBase$connect()
      • WidgetBase$initialize()
        • +
      • WidgetBase$with_read()
        • +
      • WidgetBase$with_write()
    }} \if{html}{\out{
    }} diff --git a/tests/testthat/test-widget.R b/tests/testthat/test-widget.R index 0d77f13..d1427b1 100644 --- a/tests/testthat/test-widget.R +++ b/tests/testthat/test-widget.R @@ -46,14 +46,14 @@ test_that("Widget stores Prelim attribute values as CRDT types", { w <- W$new() expect_true(inherits(w$body, "TextRef")) expect_equal( - w$ydoc$with_transaction(function(t) w$body$get_string(t)), + w$with_read(function(t) w$body$get_string(t)), "hi" ) w$body <- yr::Prelim$text("bye") expect_true(inherits(w$body, "TextRef")) expect_equal( - w$ydoc$with_transaction(function(t) w$body$get_string(t)), + w$with_read(function(t) w$body$get_string(t)), "bye" ) }) @@ -88,36 +88,20 @@ make_wire <- function() { ) } -# Read the current string content from a Prelim-text-backed attribute. Works -# uniformly across flavors because both YAttrWidget (with Prelim$text default) -# and YRootWidget expose the attribute as a TextRef. -read_text <- function(w, name) { - ref <- w[[name]] - w$ydoc$with_transaction(function(t) ref$get_string(t)) -} - # Per-flavor write: YAttrWidget supports both assign-style (replacing the # attrs-map slot with a fresh Prelim text) and in-place ref mutation; # YRootWidget's active binding is read-only, so callers must mutate the ref. mutate_in_place <- function(w, name, value) { - ref <- w[[name]] - w$ydoc$with_transaction( - function(t) { - len <- ref$len(t) - if (len > 0L) { - ref$remove_range(t, 0L, len) - } - ref$push(t, value) - }, - mutable = TRUE, - origin = LOCAL_ORIGIN - ) + w$with_write(function(t) { + ref <- w[[name]] + len <- ref$len(t) + if (len > 0L) { + ref$remove_range(t, 0L, len) + } + ref$push(t, value) + }) } -# TODO issue that to mutate we need to with_transaction (not idomatic and -# needs privzate ydoc) plus in mutable transaction case, we also need an -# inner transaction for Storage read (though not in the case of Rootdoc) -# so potential deadlock. flavors <- list( list( name = "YAttrWidget (assign)", @@ -183,18 +167,24 @@ for (f in flavors) { r_to_l$subscribe(apply_into(local)) f$write(local, "foo", "synced") - expect_equal(read_text(remote, "foo"), "") # not delivered yet + expect_equal(remote$with_read(function(t) remote$foo$get_string(t)), "") # not delivered yet expect_equal(l_to_r$pending(), 1L) l_to_r$flush() - expect_equal(read_text(remote, "foo"), "synced") + expect_equal( + remote$with_read(function(t) remote$foo$get_string(t)), + "synced" + ) expect_equal(l_to_r$pending(), 0L) expect_equal(r_to_l$pending(), 0L) # REMOTE_ORIGIN apply does not echo back f$write(remote, "bar", "also-synced") - expect_equal(read_text(local, "bar"), "") + expect_equal(local$with_read(function(t) local$bar$get_string(t)), "") r_to_l$flush() - expect_equal(read_text(local, "bar"), "also-synced") + expect_equal( + local$with_read(function(t) local$bar$get_string(t)), + "also-synced" + ) } ) }