Redesign DDL Discussion

[donyunardi]

Roadmap

• Redesign DDL NEST-roadmap#73

Start the discussion on how we'd redesign the current DDL concept.
As discussed, we'd like to allocate 2 devs for this.
Please self-assign if interested.

[gogonzo]

Please take into consideration following idea:
https://drive.google.com/file/d/1m4oji74_u3q9Or12FuDXCb-lbHkPOOKV/view?usp=sharing

• Make an object containing as little as possible (data, code, join keys) - TData is a good candidate
• Object should be able to reuse in other parts of the teal (to initialize filter-panel, tlg-modules)
• Please avoid recreation of the dataset when mutate

[nikolas-burkoff]

This also ties in with #937

[asbates]

Note I assigned myself to this but I wouldn't count me as part of the 2 devs. Mostly I'd just like to tag along to learn.

[nikolas-burkoff]

So there are 2 separate issues here:

• How would DDL fit into teal apps taking into account the design above
• How would the internals of teal.data change to simplify DDL (this includes the calls users would make to create DDL apps)

There is then a subsequent issue of how to effect this change - you don't want to keep changing teal::init in every release as it's such a key function for users

[nikolas-burkoff]

• How would DDL fit into teal apps taking into account the design above

I'm pretty confident it's like this:

Non-DDL case:

DDL case:
Not pulled data:

And Pulled:

library(shiny)
library(shinyjs)
library(teal)

# helper js and css needed for disabling tabs in a package
# these would be separate files of course see
# https://stackoverflow.com/questions/64324152/shiny-disable-tabpanel-using-shinyjs
jscode <- "
shinyjs.disableTab = function(name) {
  var tab = $('.nav li a[data-value=' + name + ']');
  tab.bind('click.tab', function(e) {
    e.preventDefault();
    return false;
  });
  tab.addClass('disabled');
}

shinyjs.enableTab = function(name) {
  var tab = $('.nav li a[data-value=' + name + ']');
  tab.unbind('click.tab');
  tab.removeClass('disabled');
}
"

css <- "
.nav li a.disabled {
  background-color: #aaa !important;
  color: #333 !important;
  cursor: not-allowed !important;
  border-color: #aaa !important;
}"


# 2 separate cases for data for the application
# 1st case you have the data so it is in a tdata class
tdata_case <- new_tdata(data = list(iris = iris, mtcars = mtcars))
# 2nd case you do not have the data yet - it is in a different class for now called
# tdata.delayed -> the structure, methds etc. on this class are all to be sorted out later
# this is a dummy example
tdata_delayed_case <- structure(list(data = list(iris = NULL, mtcars = NULL)), class = "tdata.delayed")


# An example teal module to show how teal::init would work both ui and server are straightforward
# and could be used inside a non-teal shiny app
nik_module <- function(label = "label") {
  module(
    # in this example the ui does not have access to any data-related stuff
    # in general it can only have access to items that are available in both tdata and tdata.delayed
    # therefore names of data would be OK - if you want to specify anything else (e.g. what values
    # can go in the data extracts) then either:
    # 1) this must be done in the server where there is data OR
    # 2) we could have the "data model" specified and available to both the UI and server function
    # (this sort of thing might work well with chevron2teal for example)
    # this change is related to https://github.com/insightsengineering/NEST-roadmap/issues/36
    ui = function(id) {
      ns <- NS(id)
      tagList(
        uiOutput(ns("dataname_ui")),
        verbatimTextOutput(ns("table"))
      )
    },
    server = function(id, data) {
      moduleServer(id, function(input, output, session) {
        output$dataname_ui <- renderUI({
          selectInput(session$ns("datanames"), label = "Choose data", choices = names(data))
        })

        output$table <- renderPrint({
          req(input$datanames)
          data[[input$datanames]]()
        })
      })
    },
    label = label
  )
}


# These modules handle data loading (in a package they would be s3 dispatches)
# and these would live in teal

# If you already have the data then there is no UI and...
DDL_module_ui.tdata <- function(id) {
  NULL
}

#...the server just returns the tdata object and the fact that the data
# has been pulled (i.e. pulled > 0)
DDL_module_srv.tdata <- function(id, data) {
  moduleServer(id, function(input, output, session) {
    return(
      list(tdata = data, pulled = reactive(1))
    )
  })
}

# in the non DDL case there needs to be some custom UI for pulling
# the data this would come from teal.data (and be specific to the data being pulled)
# but would be placed in a tab panel -> here we just have a dummy button to "pull" the data
DDL_module_ui.tdata_delayed <- function(id) {
  ns <- NS(id)
  tabPanel(
    title = "DDL",
    value = "DDL",
    tagList(
      actionButton(ns("pull"), "Get Data")
    )
  )
}

# the server function for non-delayed data - the actual pull function
# would live with the tdata_delayed object but the overall structure
# would live in teal - the data arguement here is of type tdata.delayed
# things like error handling including when re-pulling the data fails
# need to be handled etc.
DDL_module_srv.tdata_delayed <- function(id, data) {

  moduleServer(id, function(input, output, session) {
    # number of times the data has been pulled
    pulled <- reactiveVal(0)

    # skeleton tdata object which will be filled in with the pulled data
    tdata <- new_tdata(
      data = structure(lapply(names(data$data), \(x) reactiveVal(data.frame())), names = names(data$data))
    )

    # when you press the button to pull the data
    observeEvent(input$pull, {

      # custom pulling - in this case just hard coding the iris and mtcars data
      tdata$iris(iris)
      tdata$mtcars(mtcars)

      # increas pulling counter
      pulled(pulled()+1)
      shiny::showNotification("Data has been pulled into the app", type = "message")
    })

    # return the tdata object and #of times the data has been pulled
    return(list(
      tdata = tdata,
      pulled = reactive(pulled())))
  })
}


# this is the much simpler version of teal::init -
# there is a lot missing here (filterpanel, allowing nested modules, sanitizing the tabpanel ids
# etc. etc.) but it shows how much simpler this can be than teal::init and removes need for
# insertUI etc.
new_init <- function(data, modules) {

  # straightfoward ui function adding tabs for each module (would need to be adapted for nested modules etc.)
  ui <- function() {
    fluidPage(
      useShinyjs(),
      extendShinyjs(text = jscode, functions = c('disableTab','enableTab')),
      inlineCSS(css),
      do.call(
        tabsetPanel,
        c(
          list(
            id = "mainTabs",
            if (is(data, "tdata")) { # would be S3 dispatch in package
              DDL_module_ui.tdata(id = "DDL")
            } else {
              DDL_module_ui.tdata_delayed(id = "DDL")
            }
          ),
          lapply(
            names(modules$children),
            function(id) tabPanel(
              modules$children[[id]]$ui(id),
              id = modules$children[[id]]$label, # need to be sanitized etc.
              value = modules$children[[id]]$label,
              title = modules$children[[id]]$label
            )
          )
        )
      )
    )
  }

  # simplified server function
  srv <- function(input, output, session) {

    # disable all tabs (except the DDL ones...)
    lapply(modules$children, function(x) shinyjs::js$disableTab(x$label))

    DDL_output <- if (is(data, "tdata")) { # would be S3 dispatch of course
      DDL_module_srv.tdata("DDL", data)
    } else {
      DDL_module_srv.tdata_delayed("DDL", data)
    }

    # when the data is pulled enable the module tabs - in the non-DDL case this
    # happens onload as pulled() = 1
    # note the data can be re-pulled (although a call to hide/disable the DDL
    # tab could be added)
    observeEvent(DDL_output$pulled(), {
      if(DDL_output$pulled() > 0) {
        lapply(modules$children, function(x) shinyjs::js$enableTab(x$label))
        updateTabsetPanel("mainTabs", session = session, selected = modules$children[[1]]$label)
      }
    })

    # server functions for the modules, note the data is DDL_output$tdata
    sapply(names(modules$children), USE.NAMES = TRUE, function(id) {
      do.call(modules$children[[id]]$server, list(id = id, data = DDL_output$tdata))
    })
  }

  return(
    list(
      ui = ui,
      server = srv
    )
  )
}

######
# Example apps for ddl and non ddl cases

app_ddl <- new_init(
  data = tdata_delayed_case,
  modules = modules(nik_module("X"), nik_module("Y"))
)

runApp(app_ddl)

app_not_ddl <- new_init(
  data = tdata_case,
  modules = modules(nik_module("X"), nik_module("Y"))
)

runApp(app_not_ddl)

[nikolas-burkoff]

See #534

We should also use a navbar page with menus to define the UI so this:

navbarPage(
    "NEST APP",
    tabPanel("DDL"),
    tabPanel("One module"),
    navbarMenu(
      "Nested modules",
      tabPanel("Output 1"),
      tabPanel("Output 2")
    ),
    tabPanel("Another module"),
    navbarMenu(
      "Nested modules 2",
      tabPanel("Output 3"),
      tabPanel("Output 4")
    ),
  )

For

modules(
  module("One module"),
  modules("Nested Modules", module("Output 1"), module("Output 2")),
  module("Another module"),
  modules("Nested Modules 2", module("Output 3"), module("Output 4"))
)

One really key advantage of this is we don't need separate server and ui functions for teal.modules as the teal.module server functions can be accessed directly (and tbh we can probably remove the teal.modules object entirely)

[nikolas-burkoff]

In order to remove the use of insertUI - the UI of the teal_modules needs to be able to be created before we have the data

Maybe we could could cheat and have the shiny ui function for teal modules as just uiOutput(ns("module")) and then the shiny server be: output$module <- renderUI(teal_module$ui())

but to solve it properly we need to do the much bigger:

Handle resolved delayed differently

• we could make the resolving happen in the server functions
• we could change teal.transform functions to allow specifying columns of data without the data being there yet (possibly with some check when the data arrives)
  [This interacts with https://github.com/Design data extract and data merge NEST-roadmap#36]

---

Remove insertUI

i.e. #669

• module structure is created on app load (remove teal_with_splash_srv etc.)
• use navmenus to simplify teal::init (then can remove teal.modules object)
  [at this point you can only pull data once and then we can hide the DDL page] Refactor nested_tabs module #534

Allow data to be re-pulled

• change Teal.data to allow data to be pulled more than once i.e. https://github.com/insightsengineering/coredev-tasks/issues/256

Allow teal::init to use tdata object

• cdisc information moved into join_keys [Feature Request]: Change JoinKeys to something simpler teal.data#78 (comment)

TealData$pull() outputs a tdata object

related to #937

• tdata definition moved into teal.data
• changes in teal::init to support this [How does this timeline work with changes to the interaction of teal::init with the filter panel]

Simplify API for teal.data and connectors [big] for example:

This is related to https://github.com/insightsengineering/coredev-tasks/issues/338

• have one execution environment for pulling and user gives a script to pull/preprocess the data
• have one ui/server function to get the data instead of nested layers
• remove objects like Tealdatasetconnector, TealDataConnector (but keep TealConnection) and the associated ui/server
• simple wrappers to create the above for common cases

[gogonzo]

@nikolas-burkoff I agree with you regarding steps. Solving problem of resolve delay should be a priority as data_extract_ui the only place where data are used in the ui (indirectly by using resolved data_extract_spec). We need to first fix data_extract_ui/srv maybe within this increment?
Then the rest should be straightforward and initializing UI could be possible before shiny::runApp

[nikolas-burkoff]

indirectly by using resolved data_extract_spec

Is it the case that it is always for a data_extract_spec though? No, in the longitudinal app we have things like:

cs_box_facet_vars <- choices_selected(
  choices = variable_choices("ADLB", subset = box_facet_vars),
  selected = "AVISITCD"
)

which are "delayed_variable_choices" but there's no d-e-s in teal.goshawk and in that case it's used in a plain old optionalSelectInput - though I guess there's not the need for the data in order to create that UI when you explicitly list the column names - similarly for d-e-s I guess

[gogonzo]

there's not the need for the data in order to create that UI

Exactly, tlg-modules individually doesn't know anything about unresolved class. Tlg-modules receive fully specified instructions. Shiny modules handling data-extract doesn't work with unresolved neither. I had a time to draw a flow chart with simplification of the process:

Bit of the explanation in the beginning:

• Dashed rectangles represent the srv/ui modules. If module is inside of another it means that parent module (enclosing) called child (enclosed) module.
• a tube represents observer
• rectangle represent the object - it don't make an distinction whether it's wrapped in the reactive or not
• arrow represents some action

I'm skipping first two teal modules as the most interesting stuff starts from srv_teal and ui_teal.
TealData object gets to the srv_teal which is detected by the observer (1). It triggers creation of the datasets (FilteredData), insert of the ui_tabs_with_filters (2) and call srv_tabs_with_filters. ui/srv_tabs_with_filters call ui/srv_nested_tabs. In ui_nested_tabs each tlg-module is called but to call this module we need to resolve delayed objects (3), the same happens on the server side (4). Step (3) is really problematic for us as de facto we need data on the ui side.

---

@nikolas-burkoff found one solution to initialize UI without the need for data. It seems possible to skip the insertUI part and initialize all UI modules. To do this we just need to initialize all tabs and place uiOutput in ui_nested_tabs and renderUI in srv_nested_tabs.

Unfortunatelly this is not enough because in ui/srv_tabs_with_filters we also need to create a filter-panel, which requires datasets (FilteredData) object on the UI side. It can be achieved in the same way but ui/srv_filter_panel must be taken out from the FilteredData class.

Note: Apart from possible "quick" solutions we need serious discussion about sense of resolve_delay and possible alternatives @donyunardi .

[nikolas-burkoff]

Further details for:

Simplify API for teal.data and connectors [big] for example:

-have one execution environment for pulling and user gives a script to pull/preprocess the data
-have one ui/server function to get the data instead of nested layers
-remove objects like Tealdatasetconnector, TealDataConnector (but keep TealConnection) and the associated ui/server
-simple wrappers to create the above for common cases

You could imagine something along the lines of this in the completely general case (how to handle codedepends/do we care?)
note this is a rough sketch

  tdata.delayed(

    code = {
      conn <- open_connection(  {{"username"}}, {{"password"}}) 
      adsl <- read_data(conn) 
      adae <- read_data(conn)
      close_connection(conn)
      adsl <- adsl[1:{{n}}, ] 
    },
    tdata_args = list( #same args as new_tdata so easy to switch between DDL and non-DDL
      data = list(ADSL = adsl, ADAE = adae) # taken from code environment above  
      join_keys = cdisc_join_keys(adsl, adae)
      ...
    )
    ui = function(id){
      tagList(
        textInput(ns("username"), "Enter Username"),
        textInput(ns("password"), "Enter Password),
        sliderInput(ns("n"), "Choose n",...)
      )
      actionButton(ns("submit"), "Submit)
    },
    code_mapping = list(
      password = list(interactive = "password", default = askpass::askpassword()), # linking {{password}} to ui-id
      username = 
      n =    
    )
    server = function(id) {  #would be default so not needed to be added by users
      observeEvent(input$submit, <<execute code + create tdata object>>)
    }

  )

with much more helpful shortcuts for common cases which create code + ui (but not modules within modules) + code_mapping for you

[nikolas-burkoff]

Closing this issue as most things here will be handled at a much later date. However, things that can be handled now:

First handle join keys better:

• Add cdisc_data information into join_keys teal.data#120
• [Feature Request]: Change JoinKeys to something simpler teal.data#78

Then get tdata to be output from TealData

• Output of TealData should be tdata teal.data#121
• Use tdata inside teal::init #783

And could then do

• https://github.com/insightsengineering/coredev-tasks/issues/256

---

Separately we could handle:

• Refactor nested_tabs module #534

---

After the above we could, though may choose not, to attack #937

Following this we need:

• the refactored filter panel inside teal.init Implement new design of the filter panel teal.slice#48
• decide on how to handle resolved_delayed which may affects how users pass arguments to modules
• related to https://github.com/insightsengineering/NEST-roadmap/issues/36] which should be solved
• Then we can finally implement Redesign DDL Discussion #764 (comment) which closes Remove insertUI in the teal::init #669
• Then teal 1.0 needs moving the reporter and SRC outside of the modules and into teal

[mhallal1]

@nikolas-burkoff @pawelru @gogonzo
Adding the comments about routing suggestion from yesterday:

1. shiny.router (https://appsilon.github.io/shiny.router/index.html) does not actually do routing, it initializes the whole UI and hides the unclicked pages (see below) which does not serve the idea we had of initializing the other tabs UIs once the data is pulled in the first tab.
   

2. The idea behind brochure (https://github.com/ColinFay/brochure) package is cool but it is not maintained and still in experimental phase.