Update FilteredData.R #57

Polkas · 2022-06-30T09:46:04Z

closes #55
linked to insightsengineering/teal#681

Refactor the FIlterPanel

example_module2 <- function(label = "example teal module - no filter panel") {
  checkmate::assert_string(label)
  module(
    label,
    server = function(id, datasets) {
      moduleServer(id, function(input, output, session) {
        output$text <- renderPrint(datasets$get_data(input$dataname, filtered = TRUE))
      })
    },
    ui = function(id, datasets) {
      ns <- NS(id)
      teal.widgets::standard_layout(
        output = verbatimTextOutput(ns("text")),
        encoding = selectInput(ns("dataname"), "Choose a dataset", choices = datasets$datanames())
      )
    },
    filters = NULL
  )
}

example_module <- function(label = "example teal module", filters = "all") {
  checkmate::assert_string(label)
  module(
    label,
    server = function(id, datasets) {
      moduleServer(id, function(input, output, session) {
        output$text <- renderPrint(datasets$get_data(input$dataname, filtered = TRUE))
      })
    },
    ui = function(id, datasets) {
      ns <- NS(id)
      teal.widgets::standard_layout(
        output = verbatimTextOutput(ns("text")),
        encoding = selectInput(ns("dataname"), "Choose a dataset", choices = datasets$datanames())
      )
    },
    filters = filters
  )
}

app <- teal::init(data = list("iris" = iris, "airquality" = airquality),
                  modules = list(modules(example_module(), example_module(filters = "airquality"), example_module2(), example_module2()), 
                                 example_module(filters = "iris"), example_module2()))
shiny::shinyApp(app$ui, app$server)

github-actions · 2022-06-30T09:51:40Z

Unit Tests Summary

    1 files   29 suites 18s ⏱️
421 tests 421 ✔️ 0 💤 0 ❌
727 runs 727 ✔️ 0 💤 0 ❌

Results for commit d107426.

♻️ This comment has been updated with latest results.

github-actions · 2022-06-30T09:52:29Z

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/call_utils.R                  156       4  97.44%   142-145
R/CDISCFilteredData.R           153      27  82.35%   113, 122, 126, 140-149, 251-258, 276-279, 285, 350-353
R/choices_labeled.R              49      14  71.43%   19, 30, 35, 45-50, 62, 66-70
R/eval_expr_with_msg.R           16       6  62.50%   8-13
R/filter_panel_api.R             10       0  100.00%
R/filtered_data_wrappers.R       17      17  0.00%    134-169
R/FilteredData.R                429     299  30.30%   131, 215-219, 253, 255, 257, 260, 275-289, 330-344, 464-466, 593-959, 983, 990-991
R/FilteredDataset.R             250      84  66.40%   100, 187, 283, 333-373, 442-448, 489-499, 664-706
R/FilterState.R                1362     718  47.28%   160, 353-354, 451, 636-650, 712-762, 903-930, 1068-1177, 1202-1208, 1382-1542, 1666-1671, 1675-1681, 1694, 1698, 1830-1947, 2004-2010, 2023, 2155-2272, 2300-2306, 2319, 2405-2407, 2473-2634, 2663-2669, 2683
R/FilterStates.R               1413     595  57.89%   101, 119, 223, 302, 381, 432-618, 632-633, 751-787, 942-951, 1008, 1074, 1115, 1176-1181, 1316-1437, 1452, 1486, 1554-1559, 1579-1585, 1592-1597, 1604-1605, 1607, 1768-1777, 1810-1817, 1825-1832, 1860-2029, 2069-2075, 2092-2098, 2105-2110, 2118-2119, 2121, 2169-2170, 2251-2369, 2416, 2449, 2470
R/include_css_js.R                5       5  0.00%    12-16
R/init_filitered_data.R          62       0  100.00%
R/MAEFilteredDataset.R          212      59  72.17%   25, 122, 254-330
R/Queue.R                        50      23  54.00%   22, 40-72, 96-113, 154
R/resolve_state.R                23       0  100.00%
R/utils.R                        74      13  82.43%   123-129, 141-146
R/variable_types.R               48      33  31.25%   42-47, 57, 70-105
R/zzz.R                           6       6  0.00%    3-11
TOTAL                          4335    1903  56.10%

Results for commit: 0da0166

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

kpagacz · 2022-06-30T09:54:24Z

The issue is a bit more complex because the root cause is shiny not recalculating "invisible" outputs. Therefore, this buggy behaviour will be present not only when active_datanames() == 0 but it will be present in general when the filter panel is hidden during the transit between tabs (I could reproduce this to some extent).

I will take over this PR :)

* Fixed a bug when the filter panel overview would not recalculate when changing a new module in cases when the panel was hidden during a transition between the modules. * Changed the behaviour of the burger button when there are no active datanames in the filter panel. Closes #55

kpagacz · 2022-06-30T11:17:59Z

This should be ready to review. I changed the behaviour of the burger button to show the filter panel even when there are no active datanames. The previous behaviour was messy - the toggle state was confused with the manual hiding of the panel in such cases, which caused a need for an additional click to actually get in sync with the rest of the tabs and on top of that when the burger was clicked it would resize the encoding and the main panel to fit the hidden filter panel. It effectively was worse than doing nothing.

If it is unclear where the actual fix is it is in

shiny::outputOption(output, "table", suspendWhenHidden = FALSE)

that forces the table to recalculate even when the overview is hidden in the UI.

As a bonus I refactored the long server function a little bit, should be more readable right now.

EDIT: This one took an unreasonable amount of time to debug mainly due to what I consider an unintuitive behaviour of shiny: if it scans whether the recalculation is at all needed when something is hidden using js, it should follow that it scans whether something needs recalculation when something is shown using js. Except it does not.

kpagacz · 2022-06-30T11:20:50Z

R/FilteredData.R

@@ -899,6 +857,7 @@ FilteredData <- R6::R6Class( # nolint
            table_html
          })

+          shiny::outputOptions(output, "table", suspendWhenHidden = FALSE)


Here's the actual fix.

I will reduce the PR to only this line, if it is enough.

kpagacz · 2022-06-30T11:22:50Z

R/FilteredData.R

+          private$hide_inactive_datasets(active_datanames)
+          if (length(active_datanames()) == 0 || is.null(active_datanames())) {
+            # hide the filter panel UI when the active module does not use any datasets from the panel
+            shinyjs::runjs("if (filter_open) toggle_sidebar();")
+          }


Make note of this reversed order of business here compared to main.

Here we:

hide inactive dataset names from the panel

Hide the panel

On main we do the reverse which leads to the lack of update of the names in the filter panel because shiny does not see a need to update anything in the UI if it is hidden anyway. Sadly, when the UI is dynamically shown, it does not detect that these inactive dataset names should be removed from the UI. That led to the active datanames being a copy of the last module's that had active datanames.

Polkas · 2022-06-30T11:33:23Z

try with a module which have a filters = NULL.

example_module <- function(label = "example teal module") {
  checkmate::assert_string(label)
  module(
    label,
    server = function(id, datasets) {
      moduleServer(id, function(input, output, session) {
        output$text <- renderPrint(datasets$get_data(input$dataname, filtered = TRUE))
      })
    },
    ui = function(id, datasets) {
      ns <- NS(id)
      teal.widgets::standard_layout(
        output = verbatimTextOutput(ns("text")),
        encoding = selectInput(ns("dataname"), "Choose a dataset", choices = datasets$datanames())
      )
    },
    filters = NULL
  )
}

It not works correctly for such scenario now. If you add other modules too, then this example module will close a FilterPanel globally not only locally.

Polkas · 2022-06-30T11:49:18Z

I think the separation of the toggle js function from active_datanames was done on purpose.
The latter one is connected with e.g. filters=NULL on the level of the module. This was added on the request of the end user, when a module has filter=NULL then it have to be always closed, the burger button is redundant for it. In other words both functionalities have to live in separation.

kpagacz · 2022-06-30T13:52:30Z

I pivoted on my initial idea and implemented it as it should be - using events :)

Should be working now as intended. Here is the example I used in the development:

devtools::load_all("teal.slice")
devtools::load_all("teal")

example_module <- function(label = "example teal module") {
  checkmate::assert_string(label)
  module(
    label,
    server = function(id, datasets) {
      moduleServer(id, function(input, output, session) {
        output$text <- renderPrint(datasets$get_data(input$dataname, filtered = TRUE))
      })
    },
    ui = function(id, datasets) {
      ns <- NS(id)
      teal.widgets::standard_layout(
        output = verbatimTextOutput(ns("text")),
        encoding = selectInput(ns("dataname"), "Choose a dataset", choices = datasets$datanames())
      )
    },
    filters = NULL
  )
}

app <- teal::init(data = list("iris" = iris), modules = list(example_module(), teal::example_module()))
shiny::shinyApp(app$ui, app$server)

R/FilteredData.R

gogonzo · 2022-07-01T12:31:27Z

We can discuss this more. At this moment module with filter = NULL have a burger which makes an empty space. I think burger should be disabled for module without filters

app

options(teal.log_level = logger::TRACE)
devtools::load_all("teal")
#library(teal.modules.general)
library(scda)
#logger::log_threshold(logger::TRACE, namespace = "teal")

ADSL <- synthetic_cdisc_data("latest")$adsl
ADTTE <- synthetic_cdisc_data("latest")$adtte
ADRS <- synthetic_cdisc_data("latest")$adrs

reporter_module <- function(label = "example teal module") {
  checkmate::assert_string(label)
  module(
    label,
    server = function(id, datasets, reporter) {
      moduleServer(id, function(input, output, session) {
        teal.reporter::simple_reporter_srv(
          id = "reporter",
          reporter = reporter,
          card_fun = function(card) card
        )
        output$text <- renderPrint(datasets$get_data(input$dataname, filtered = TRUE))
      })
    },
    ui = function(id, datasets) {
      ns <- NS(id)
      teal.widgets::standard_layout(
        output = tagList(
          teal.reporter::simple_reporter_ui(ns("reporter")),
          verbatimTextOutput(ns("text"))
        ),
        encoding = selectInput(ns("dataname"), "Choose a dataset", choices = datasets$datanames())
      )
    },
    filters = "all"
  )
}

adsl_module <- module(
  label = "ADSL module",
  ui =  function(id, ...) {
    ns <- NS(id)
    div(
      h1("adsl module"),
      verbatimTextOutput(ns("out")),
      selectInput(ns("button"), "select val", choices = c("a", "b"), selected = "a")
    )
  },
  server = function(id, datasets, ...) moduleServer(id, function(input, output, session) {
    cat("\nmodule-init input$button is: ", input$button)
    observeEvent(input$button, cat("\nobserver input$button is: ", input$button))
    output$out <- renderPrint(datasets$get_data("ADSL", filtered = TRUE))
  }),
  filters = "ADSL"
)
adtte_module <- module(
  label = "ADTTE module",
  ui =  function(id, ...) {
    div(
      h1("adtte module"),
      verbatimTextOutput(NS(id)("out"))
    )
  },
  server = function(id, datasets, ...) {
    moduleServer(id, function(input, output, session) {
      output$out <- renderPrint(datasets$get_data("ADTTE", filtered = TRUE))
    })
  },
  filters = "ADTTE"
)

adrs_module <- module(
  label = "ADRS module",
  ui =  function(id, ...) {
    div("adrs module")
    verbatimTextOutput(NS(id)("out"))
  },
  server = function(id, datasets, ...) {
    moduleServer(id, function(input, output, session) {
      output$out <- renderPrint(datasets$get_data("ADRS", filtered = TRUE))
    })
  },
  filters = "ADRS"
)
null_module <- module(
  label = "NULL module",
  ui = function(id, ...) {
    div("NULL module")
    verbatimTextOutput(NS(id)("out"))
  },
  server = function(id, datasets, ...) {
    moduleServer(id, function(input, output, session) {
      output$out <- renderPrint(datasets$get_data("ADRS", filtered = TRUE))
    })
  },
  filters = NULL
)


app <- init(
  data = cdisc_data(
    cdisc_dataset("ADSL", ADSL),
    cdisc_dataset("ADTTE", ADTTE),
    cdisc_dataset("ADRS", ADRS)
  ),
  modules = modules(
    modules(
      label = "tabs 1",
      modules(
        label = "tab 11",
        adsl_module,
        adtte_module
      ),
      adrs_module
    ),
    modules(
      label = "tabs 2",
      null_module,
      reporter_module()
    )
  ),
  filter = list(
    ADSL = list(
      SEX = list("M", keep_na = TRUE),
      COUNTRY = c("USA", NA),
      AGE = default_filter()
    )
  )
)

runApp(app)

Polkas · 2022-07-12T13:01:09Z

R/FilteredData.R

@@ -592,7 +592,7 @@ FilteredData <- R6::R6Class( # nolint
    ui_filter_panel = function(id) {
      ns <- NS(id)
      div(
-        id = ns("filter_panel_whole"), # used for hiding / showing
+        id = ns(NULL), # used for hiding / showing


good move as we do not need additional layer here

Polkas · 2022-07-12T13:01:39Z

R/FilteredData.R

-            },
-            ignoreNULL = FALSE
-          )
+          private$active_datanames_observer(active_datanames, session$ns(NULL))


The functionality transferred to private

Polkas · 2022-07-12T18:22:42Z

R/FilteredData.R

+            # shinyjs takes care of the namespace around the id
+            shinyjs::show(id_add_filter)
+            shinyjs::show(id_filter_dataname)
+          } else {
+            shinyjs::hide(id_add_filter)
+            shinyjs::hide(id_filter_dataname)


it comes from shinyjs:::jsFuncHelper where session$ns() is used around id.

gogonzo

I don't think there could be a good solution in the current design of the FilteredData. I think it should be like this:

teal can hide filter panel if active_datanames is null. It means hiding whole panel and disabling burger should be in teal only.
teal.slice can hide datasets in the filter-panel if they are not within active_datanames. The js functions to hide particulat datasets should be in teal.slice

gogonzo · 2022-07-13T10:47:39Z

R/FilteredData.R

            },
-            ignoreNULL = FALSE
+            priority = 1


why is it needed? I think it's fine without priority

it will work without the priority = 1, I assumed Konrad made some research that it boost performance

NEWS.md

gogonzo

👍

Update FilteredData.R

748185a

Polkas added the core label Jun 30, 2022

Polkas mentioned this pull request Jun 30, 2022

Refactor filter panel - summary and collapsible #55

Closed

Polkas self-assigned this Jun 30, 2022

kpagacz reviewed Jun 30, 2022

View reviewed changes

changed the logic to use events

75f8e8e

kpagacz reviewed Jun 30, 2022

View reviewed changes

R/FilteredData.R Outdated Show resolved Hide resolved

wrap filter_panel_server in a div with id: ns(filter_panel)

21ef302

gogonzo self-assigned this Jul 1, 2022

Merge branch 'main' into 55_burger@main

77fb274

Polkas commented Jul 12, 2022

View reviewed changes

news

fcfafb0

Polkas commented Jul 12, 2022

View reviewed changes

Polkas mentioned this pull request Jul 13, 2022

feat: added support for events from the filter panel insightsengineering/teal#681

Merged

gogonzo requested changes Jul 13, 2022

View reviewed changes

transfer to teal

37fc7fa

gogonzo reviewed Jul 13, 2022

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

gogonzo approved these changes Jul 13, 2022

View reviewed changes

Polkas added 3 commits July 13, 2022 12:54

NEWS

59f11a5

styler

cccd0a4

Merge branch 'main' into 55_burger@main

d107426

Polkas merged commit ff86485 into main Jul 13, 2022

Polkas deleted the 55_burger@main branch July 13, 2022 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update FilteredData.R #57

Update FilteredData.R #57

Polkas commented Jun 30, 2022 •

edited

Loading

github-actions bot commented Jun 30, 2022 •

edited

Loading

github-actions bot commented Jun 30, 2022 •

edited

Loading

kpagacz commented Jun 30, 2022

kpagacz commented Jun 30, 2022 •

edited

Loading

kpagacz Jun 30, 2022

Polkas Jun 30, 2022

kpagacz Jun 30, 2022 •

edited

Loading

Polkas commented Jun 30, 2022 •

edited

Loading

Polkas commented Jun 30, 2022

kpagacz commented Jun 30, 2022

gogonzo commented Jul 1, 2022

Polkas Jul 12, 2022

Polkas Jul 12, 2022

Polkas Jul 12, 2022

gogonzo left a comment

gogonzo Jul 13, 2022

Polkas Jul 13, 2022

gogonzo left a comment

Update FilteredData.R #57

Update FilteredData.R #57

Conversation

Polkas commented Jun 30, 2022 • edited Loading

github-actions bot commented Jun 30, 2022 • edited Loading

Unit Tests Summary

github-actions bot commented Jun 30, 2022 • edited Loading

Code Coverage Summary

kpagacz commented Jun 30, 2022

kpagacz commented Jun 30, 2022 • edited Loading

kpagacz Jun 30, 2022

Choose a reason for hiding this comment

Polkas Jun 30, 2022

Choose a reason for hiding this comment

kpagacz Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Polkas commented Jun 30, 2022 • edited Loading

Polkas commented Jun 30, 2022

kpagacz commented Jun 30, 2022

gogonzo commented Jul 1, 2022

Polkas Jul 12, 2022

Choose a reason for hiding this comment

Polkas Jul 12, 2022

Choose a reason for hiding this comment

Polkas Jul 12, 2022

Choose a reason for hiding this comment

gogonzo left a comment

Choose a reason for hiding this comment

gogonzo Jul 13, 2022

Choose a reason for hiding this comment

Polkas Jul 13, 2022

Choose a reason for hiding this comment

gogonzo left a comment

Choose a reason for hiding this comment

Polkas commented Jun 30, 2022 •

edited

Loading

github-actions bot commented Jun 30, 2022 •

edited

Loading

github-actions bot commented Jun 30, 2022 •

edited

Loading

kpagacz commented Jun 30, 2022 •

edited

Loading

kpagacz Jun 30, 2022 •

edited

Loading

Polkas commented Jun 30, 2022 •

edited

Loading