From 01b620edc25ce291a5ed5cc4516a666dec4a0b74 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dawid=20Ka=C5=82=C4=99dkowski?=
 <dawid.kaledkowski@gmail.com>
Date: Mon, 10 Jul 2023 10:45:14 +0200
Subject: [PATCH 1/4] duplicated available slices

---
 R/FilterStates.R                   | 41 ++++++++-------
 R/FilteredData.R                   | 84 ++++++++++++++++++------------
 R/teal_slice.R                     | 31 +++++++++--
 man/get_default_slice_id.Rd        | 28 ++++++++++
 tests/testthat/test-FilteredData.R | 25 ++++++---
 5 files changed, 145 insertions(+), 64 deletions(-)
 create mode 100644 man/get_default_slice_id.Rd

diff --git a/R/FilterStates.R b/R/FilterStates.R
index 9a2492de8..bbc1de27e 100644
--- a/R/FilterStates.R
+++ b/R/FilterStates.R
@@ -200,14 +200,9 @@ FilterStates <- R6::R6Class( # nolint
     remove_filter_state = function(state) {
       shiny::isolate({
         checkmate::assert_class(state, "teal_slices")
-
-        lapply(state, function(x) {
-          state_id <- x$id
-          logger::log_trace("{ class(self)[1] }$remove_filter_state removing filter, state_id: { state_id }")
-          private$state_list_remove(state_id = state_id)
-          logger::log_trace("{ class(self)[1] }$remove_filter_state removed filter, state_id: { state_id }")
-        })
-
+        state_ids <- vapply(state, `[[`, character(1), "id")
+        logger::log_trace("{ class(self)[1] }$remove_filter_state removing filters, state_id: { toString(state_ids) }")
+        private$state_list_remove(state_ids)
         invisible(NULL)
       })
     },
@@ -635,14 +630,21 @@ FilterStates <- R6::R6Class( # nolint
         logger::log_trace("{ class(self)[1] } removing a filter, state_id: { state_id }")
         checkmate::assert_character(state_id)
         new_state_list <- private$state_list()
-        if (is.element(state_id, names(new_state_list))) {
-          if (new_state_list[[state_id]]$get_state()$locked) {
-            return(invisible(NULL))
-          }
-          new_state_list[[state_id]]$destroy_observers()
-          new_state_list[[state_id]] <- NULL
+        current_state_ids <- vapply(private$state_list(), function(x) x$get_state()$id, character(1))
+        to_remove <- state_id %in% current_state_ids
+        if (any(to_remove)) {
+          new_state_list <- Filter(
+            function(state) {
+              if (state$get_state()$id %in% state_id && !state$get_state()$locked) {
+                state$destroy_observers()
+                FALSE
+              } else {
+                TRUE
+              }
+            },
+            private$state_list()
+          )
           private$state_list(new_state_list)
-          logger::log_trace("{ class(self)[1] } removed a filter, state_id: { state_id }")
         } else {
           warning(sprintf("\"%s\" not found in state list", state_id))
         }
@@ -660,13 +662,12 @@ FilterStates <- R6::R6Class( # nolint
         logger::log_trace(
           "{ class(self)[1] }$state_list_empty removing all non-locked filters for dataname: { private$dataname }"
         )
+
         state_list <- private$state_list()
-        for (state_id in names(state_list)) {
-          private$state_list_remove(state_id)
+        if (length(state_list)) {
+          state_list_ids <- vapply(state_list, function(x) x$get_state()$id, character(1))
+          private$state_list_remove(state_list_ids)
         }
-        logger::log_trace(
-          "{ class(self)[1] }$state_list_empty removed all non-locked filters for dataname: { private$dataname }"
-        )
         invisible(NULL)
       })
     },
diff --git a/R/FilteredData.R b/R/FilteredData.R
index 7c91b0086..f6d4c9a2b 100644
--- a/R/FilteredData.R
+++ b/R/FilteredData.R
@@ -481,7 +481,7 @@ FilteredData <- R6::R6Class( # nolint
           private$module_add <- module_add
         }
 
-        lapply(datanames, function(dataname) {
+        lapply(self$datanames(), function(dataname) {
           states <- Filter(function(x) identical(x$dataname, dataname), state)
           private$get_filtered_dataset(dataname)$set_filter_state(states)
         })
@@ -590,7 +590,10 @@ FilteredData <- R6::R6Class( # nolint
     #' @return invisible `NULL`
     set_available_teal_slices = function(x) {
       checkmate::assert_class(x, "reactive")
-      private$available_teal_slices <- x
+      private$available_teal_slices <- reactive({
+        # we want to limit the available filters to the ones that are relevant for this FilteredData
+        Filter(function(x) x$dataname %in% self$datanames(), x())
+      })
       invisible(NULL)
     },
 
@@ -1072,7 +1075,6 @@ FilteredData <- R6::R6Class( # nolint
     # appropriate filter (identified by it's id)
     srv_available_filters = function(id) {
       moduleServer(id, function(input, output, session) {
-        slices <- reactive(Filter(function(slice) !isTRUE(slice$locked), private$available_teal_slices()))
         slices_interactive <- reactive(
           Filter(
             function(slice) !isTRUE(slice$fixed) && !inherits(slice, "teal_slice_expr"),
@@ -1085,8 +1087,16 @@ FilteredData <- R6::R6Class( # nolint
             private$available_teal_slices()
           )
         )
-        available_slices_id <- reactive(vapply(slices(), `[[`, character(1), "id"))
+        available_slices_id <- reactive(vapply(private$available_teal_slices(), `[[`, character(1), "id"))
         active_slices_id <- reactive(vapply(self$get_filter_state(), `[[`, character(1), "id"))
+        duplicated_slice_references <- reactive({
+          # slice refers to a particular column
+          slice_reference <- vapply(private$available_teal_slices(), get_default_slice_id, character(1))
+          is_duplicated_reference <- duplicated(slice_reference) | duplicated(slice_reference, fromLast = TRUE)
+          is_active <- available_slices_id() %in% active_slices_id()
+          is_not_expr <- vapply(private$available_teal_slices(), inherits, logical(1), "teal_slice_expr")
+          slice_reference[is_duplicated_reference & is_active & !is_not_expr]
+        })
 
         checkbox_group_element <- function(name, value, label, checked, disabled = FALSE) {
           tags$div(
@@ -1112,45 +1122,53 @@ FilteredData <- R6::R6Class( # nolint
             selected = NULL
           )
           active_slices_ids <- active_slices_id()
-
-          shiny::isolate({
-            interactive_choice_mock <- lapply(
-              slices_interactive(),
-              function(slice) {
+          duplicated_slice_refs <- duplicated_slice_references()
+          interactive_choice_mock <- lapply(
+            slices_interactive(),
+            function(slice) {
+              # we need to isolate changes in the fields of the slice (teal_slice)
+              shiny::isolate({
                 checkbox_group_element(
                   name = session$ns("available_slices_id"),
                   value = slice$id,
                   label = slice$id,
                   checked = if (slice$id %in% active_slices_ids) "checked",
-                  disabled = slice$locked
+                  disabled = slice$locked ||
+                    get_default_slice_id(slice) %in% duplicated_slice_refs &&
+                    !slice$id %in% active_slices_ids
                 )
-              }
-            )
+              })
+            }
+          )
 
-            non_interactive_choice_mock <- lapply(
-              slices_fixed(),
-              function(slice) {
+          non_interactive_choice_mock <- lapply(
+            slices_fixed(),
+            function(slice) {
+              # we need to isolate changes in the fields of the slice (teal_slice)
+              isolate(
                 checkbox_group_element(
                   name = session$ns("available_slices_id"),
                   value = slice$id,
                   label = slice$id,
                   checked = if (slice$id %in% active_slices_ids) "checked",
-                  disabled = slice$locked
+                  disabled = slice$locked ||
+                    get_default_slice_id(slice) %in% duplicated_slice_refs &&
+                    !slice$id %in% active_slices_ids
                 )
-              }
-            )
+              )
+            }
+          )
 
-            htmltools::tagInsertChildren(
-              checkbox,
-              br(),
-              tags$strong("Fixed filters"),
-              non_interactive_choice_mock,
-              tags$strong("Iteractive filters"),
-              interactive_choice_mock,
-              .cssSelector = "div.shiny-options-group",
-              after = 0
-            )
-          })
+          htmltools::tagInsertChildren(
+            checkbox,
+            br(),
+            tags$strong("Fixed filters"),
+            non_interactive_choice_mock,
+            tags$strong("Iteractive filters"),
+            interactive_choice_mock,
+            .cssSelector = "div.shiny-options-group",
+            after = 0
+          )
         })
 
         observeEvent(input$available_slices_id, ignoreNULL = FALSE, ignoreInit = TRUE, {
@@ -1159,7 +1177,7 @@ FilteredData <- R6::R6Class( # nolint
           if (length(new_slices_id)) {
             new_teal_slices <- Filter(
               function(slice) slice$id %in% new_slices_id,
-              slices()
+              private$available_teal_slices()
             )
             self$set_filter_state(new_teal_slices)
           }
@@ -1167,14 +1185,14 @@ FilteredData <- R6::R6Class( # nolint
           if (length(removed_slices_id)) {
             removed_teal_slices <- Filter(
               function(slice) slice$id %in% removed_slices_id,
-              slices()
+              self$get_filter_state()
             )
             self$remove_filter_state(removed_teal_slices)
           }
         })
 
-        observeEvent(slices(), ignoreNULL = FALSE, {
-          if (length(slices())) {
+        observeEvent(private$available_teal_slices(), ignoreNULL = FALSE, {
+          if (length(private$available_teal_slices())) {
             shinyjs::show("available_menu")
           } else {
             shinyjs::hide("available_menu")
diff --git a/R/teal_slice.R b/R/teal_slice.R
index 11eca7351..a29e814fc 100644
--- a/R/teal_slice.R
+++ b/R/teal_slice.R
@@ -148,10 +148,7 @@ teal_slice <- function(dataname,
     checkmate::assert_flag(keep_inf, null.ok = TRUE)
     checkmate::assert_flag(multiple)
     if (missing(id)) {
-      args$id <- paste(
-        Filter(length, args[c("dataname", "varname", "experiment", "arg")]),
-        collapse = " "
-      )
+      args$id <- get_default_slice_id(args)
     } else {
       checkmate::assert_string(id)
     }
@@ -316,3 +313,29 @@ trim_lines_json <- function(x) {
   substr(x_trim, trim_position - 2, trim_position) <- "..."
   x_trim
 }
+
+#' Default `teal_slice` id
+#'
+#' Function returns a default `id` for a `teal_slice` object which needs
+#' to be distinct from other `teal_slice` objects created for any
+#' `FilterStates` object. Returned `id` can be treated as a location of
+#' a vector on which `FilterState` is built:
+#' - for a `data.frame` `id` concatenates `dataname` and `varname`.
+#' - for a `MultiAssayExperiment` `id` concatenates `dataname`, `varname`,
+#' `experiment` and `arg`, so that one can add `teal_slice` for a `varname`
+#' which exists in multiple `SummarizedExperiment`s or exists in both `colData`
+#' and `rowData` of given experiment.
+#' @param x (`teal_slice` or `list`)
+#' @return (`character(1)`) `id` for a `teal_slice` object.
+#' @keywords internal
+get_default_slice_id <- function(x) {
+  shiny::isolate({
+    paste(
+      Filter(
+        length,
+        as.list(x)[c("dataname", "varname", "experiment", "arg")]
+      ),
+      collapse = " "
+    )
+  })
+}
\ No newline at end of file
diff --git a/man/get_default_slice_id.Rd b/man/get_default_slice_id.Rd
new file mode 100644
index 000000000..825da19c4
--- /dev/null
+++ b/man/get_default_slice_id.Rd
@@ -0,0 +1,28 @@
+% Generated by roxygen2: do not edit by hand
+% Please edit documentation in R/teal_slice.R
+\name{get_default_slice_id}
+\alias{get_default_slice_id}
+\title{Default \code{teal_slice} id}
+\usage{
+get_default_slice_id(x)
+}
+\arguments{
+\item{x}{(\code{teal_slice} or \code{list})}
+}
+\value{
+(\code{character(1)}) \code{id} for a \code{teal_slice} object.
+}
+\description{
+Function returns a default \code{id} for a \code{teal_slice} object which needs
+to be distinct from other \code{teal_slice} objects created for any
+\code{FilterStates} object. Returned \code{id} can be treated as a location of
+a vector on which \code{FilterState} is built:
+\itemize{
+\item for a \code{data.frame} \code{id} concatenates \code{dataname} and \code{varname}.
+\item for a \code{MultiAssayExperiment} \code{id} concatenates \code{dataname}, \code{varname},
+\code{experiment} and \code{arg}, so that one can add \code{teal_slice} for a \code{varname}
+which exists in multiple \code{SummarizedExperiment}s or exists in both \code{colData}
+and \code{rowData} of given experiment.
+}
+}
+\keyword{internal}
diff --git a/tests/testthat/test-FilteredData.R b/tests/testthat/test-FilteredData.R
index 6c172bb90..023bc87b2 100644
--- a/tests/testthat/test-FilteredData.R
+++ b/tests/testthat/test-FilteredData.R
@@ -658,7 +658,6 @@ testthat::test_that("clear_filter_states does not remove locked filters", {
     )
   )
   datasets$set_filter_state(state = fs)
-
   datasets$clear_filter_states()
 
   testthat::expect_length(shiny::isolate(datasets$get_filter_state()), 2)
@@ -905,6 +904,7 @@ fs <- teal_slices(
   teal_slice(dataname = "iris", varname = "Sepal.Width", fixed = TRUE),
   teal_slice(dataname = "iris", varname = "Petal.Length"),
   teal_slice(dataname = "iris", varname = "Petal.Width"),
+  teal_slice(dataname = "iris", varname = "Petal.Width", id = "duplicated"),
   teal_slice(dataname = "iris", title = "test", id = "test", expr = "!is.na(Species)")
 )
 fs_rv <- reactiveVal(fs)
@@ -914,13 +914,10 @@ shiny::testServer(
   datasets$srv_available_filters,
   expr = {
     testthat::test_that("slices_interactive() reactive returns interactive filters", {
-      expect_identical_slices(slices_interactive(), fs[c(1, 3, 4)])
+      expect_identical_slices(slices_interactive(), fs[c(1, 3, 4, 5)])
     })
     testthat::test_that("slices_fixed() reactive returns fixed filters and teal_slice_expr", {
-      expect_identical_slices(slices_fixed(), fs[c(2, 5)])
-    })
-    testthat::test_that("FilteredData$srv_available_slices locked slices ommited", {
-      testthat::expect_identical(slices(), fs[-1])
+      expect_identical_slices(slices_fixed(), fs[c(2, 6)])
     })
 
     testthat::test_that("FilteredData$srv_available_slices new state in external list reflected in available slices", {
@@ -928,7 +925,8 @@ shiny::testServer(
       fs_rv(c(fs_rv(), teal_slices(species_slice)))
       testthat::expect_identical(
         available_slices_id(),
-        c("iris Sepal.Width", "iris Petal.Length", "iris Petal.Width", "test", "iris Species")
+        c("iris Sepal.Length", "iris Sepal.Width", "iris Petal.Length",
+          "iris Petal.Width", "duplicated", "test", "iris Species")
       )
     })
 
@@ -949,5 +947,18 @@ shiny::testServer(
       session$setInputs(available_slices_id = NULL)
       testthat::expect_identical(active_slices_id(), "iris Sepal.Length")
     })
+
+    testthat::test_that("duplicated_slice_references() returns character(0) if none of duplicated filters is active", {
+      session$setInputs(available_slices_id = "Sepal.Length")
+      testthat::expect_identical(duplicated_slice_references(), character(0))
+    })
+
+    testthat::test_that(
+      "duplicated_slice_references() returns variable reference when any of duplicated filters is on",
+      {
+        session$setInputs(available_slices_id = "duplicated")
+        testthat::expect_identical(duplicated_slice_references(), "iris Petal.Width")
+      }
+    )
   }
 )

From 4e706c32a8997851052f0c34beac3f9273d5c057 Mon Sep 17 00:00:00 2001
From: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Date: Mon, 10 Jul 2023 09:13:08 +0000
Subject: [PATCH 2/4] [skip actions] Restyle files

---
 R/FilteredData.R                   | 4 ++--
 R/teal_slice.R                     | 2 +-
 tests/testthat/test-FilteredData.R | 6 ++++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/R/FilteredData.R b/R/FilteredData.R
index f6d4c9a2b..011bf0494 100644
--- a/R/FilteredData.R
+++ b/R/FilteredData.R
@@ -1135,7 +1135,7 @@ FilteredData <- R6::R6Class( # nolint
                   checked = if (slice$id %in% active_slices_ids) "checked",
                   disabled = slice$locked ||
                     get_default_slice_id(slice) %in% duplicated_slice_refs &&
-                    !slice$id %in% active_slices_ids
+                      !slice$id %in% active_slices_ids
                 )
               })
             }
@@ -1153,7 +1153,7 @@ FilteredData <- R6::R6Class( # nolint
                   checked = if (slice$id %in% active_slices_ids) "checked",
                   disabled = slice$locked ||
                     get_default_slice_id(slice) %in% duplicated_slice_refs &&
-                    !slice$id %in% active_slices_ids
+                      !slice$id %in% active_slices_ids
                 )
               )
             }
diff --git a/R/teal_slice.R b/R/teal_slice.R
index a29e814fc..2afcbeb5a 100644
--- a/R/teal_slice.R
+++ b/R/teal_slice.R
@@ -338,4 +338,4 @@ get_default_slice_id <- function(x) {
       collapse = " "
     )
   })
-}
\ No newline at end of file
+}
diff --git a/tests/testthat/test-FilteredData.R b/tests/testthat/test-FilteredData.R
index 023bc87b2..05a761f26 100644
--- a/tests/testthat/test-FilteredData.R
+++ b/tests/testthat/test-FilteredData.R
@@ -925,8 +925,10 @@ shiny::testServer(
       fs_rv(c(fs_rv(), teal_slices(species_slice)))
       testthat::expect_identical(
         available_slices_id(),
-        c("iris Sepal.Length", "iris Sepal.Width", "iris Petal.Length",
-          "iris Petal.Width", "duplicated", "test", "iris Species")
+        c(
+          "iris Sepal.Length", "iris Sepal.Width", "iris Petal.Length",
+          "iris Petal.Width", "duplicated", "test", "iris Species"
+        )
       )
     })
 

From 9fb0e5dc9db357e79b8898c6d4f43495a6e594cf Mon Sep 17 00:00:00 2001
From: go_gonzo <dawid.kaledkowski@gmail.com>
Date: Tue, 11 Jul 2023 09:07:21 +0200
Subject: [PATCH 3/4] don't drop include_varnames, exclude_varnames based on
 existing teal_slice

---
 R/FilteredDatasetDefault.R        | 12 ++----
 R/teal_slices.R                   |  3 --
 tests/testthat/test-teal_slices.R | 61 +++++++++++++++++++++++++------
 tests/testthat/test-utils.R       |  2 +-
 4 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/R/FilteredDatasetDefault.R b/R/FilteredDatasetDefault.R
index 871bf63d3..abeb10c2a 100644
--- a/R/FilteredDatasetDefault.R
+++ b/R/FilteredDatasetDefault.R
@@ -183,15 +183,11 @@ DefaultFilteredDataset <- R6::R6Class( # nolint
     set_filter_state = function(state) {
       shiny::isolate({
         logger::log_trace("{ class(self)[1] }$set_filter_state initializing, dataname: { private$dataname }")
-        checkmate::assert_class(state, "teal_slices")
-        lapply(state, function(x) {
-          checkmate::assert_true(
-            shiny::isolate(x$dataname) == private$dataname,
-            .var.name = "dataname matches private$dataname"
-          )
-          private$get_filter_states()[[1L]]$set_filter_state(state = state)
-          logger::log_trace("{ class(self)[1] }$set_filter_state initialized, dataname: { private$dataname }")
+        lapply(state, function(slice) {
+          checkmate::assert_true(slice$dataname == private$dataname)
         })
+        checkmate::assert_class(state, "teal_slices")
+        private$get_filter_states()[[1L]]$set_filter_state(state = state)
         invisible(NULL)
       })
     },
diff --git a/R/teal_slices.R b/R/teal_slices.R
index f33f833da..f3e61eca0 100644
--- a/R/teal_slices.R
+++ b/R/teal_slices.R
@@ -211,9 +211,6 @@ as.teal_slices <- function(x) { # nolint
   y <- NextMethod("[")
   attrs <- attributes(x)
   attrs$names <- attrs$names[i]
-  datanames <- unique(unlist(vapply(y, function(ts) shiny::isolate(ts[["dataname"]]), character(1L))))
-  attrs[["exclude_varnames"]] <- Filter(Negate(is.null), attr(x, "exclude_varnames")[datanames])
-  attrs[["include_varnames"]] <- Filter(Negate(is.null), attr(x, "include_varnames")[datanames])
   attributes(y) <- attrs
   y
 }
diff --git a/tests/testthat/test-teal_slices.R b/tests/testthat/test-teal_slices.R
index 5df684436..898c81715 100644
--- a/tests/testthat/test-teal_slices.R
+++ b/tests/testthat/test-teal_slices.R
@@ -109,39 +109,39 @@ testthat::test_that("[.teal_slices subsets properly", {
 })
 
 
-testthat::test_that("[.teal_slices also subsets the exclude_varnames attribute", {
+testthat::test_that("[.teal_slices doesn't subset the exclude_varnames attribute according to available teal_slice", {
   shiny::reactiveConsole(TRUE)
   on.exit(shiny::reactiveConsole(FALSE))
 
-  fs1 <- teal_slice("data1", "var1")
-  fs2 <- teal_slice("data1", "var2")
-  fs3 <- teal_slice("data2", "var1")
-  fs4 <- teal_slice("data2", "var2")
+  fs1 <- teal_slice(dataname = "data1", varname = "var1")
+  fs2 <- teal_slice(dataname = "data1", varname = "var2")
+  fs3 <- teal_slice(dataname = "data2", varname = "var1")
+  fs4 <- teal_slice(dataname = "data2", varname = "var2")
   fs <- teal_slices(fs1, fs2, fs3, fs4, exclude_varnames = list(data1 = "var1", data2 = "var1"))
 
   testthat::expect_identical(
     attr(fs[1], "exclude_varnames"),
-    list(data1 = "var1")
+    list(data1 = "var1", data2 = "var1")
   )
   testthat::expect_identical(
     attr(fs[2], "exclude_varnames"),
-    list(data1 = "var1")
+    list(data1 = "var1", data2 = "var1")
   )
   testthat::expect_identical(
     attr(fs[1:2], "exclude_varnames"),
-    list(data1 = "var1")
+    list(data1 = "var1", data2 = "var1")
   )
   testthat::expect_identical(
     attr(fs[3], "exclude_varnames"),
-    list(data2 = "var1")
+    list(data1 = "var1", data2 = "var1")
   )
   testthat::expect_identical(
     attr(fs[4], "exclude_varnames"),
-    list(data2 = "var1")
+    list(data1 = "var1", data2 = "var1")
   )
   testthat::expect_identical(
     attr(fs[3:4], "exclude_varnames"),
-    list(data2 = "var1")
+    list(data1 = "var1", data2 = "var1")
   )
   testthat::expect_identical(
     attr(fs[], "exclude_varnames"),
@@ -149,6 +149,45 @@ testthat::test_that("[.teal_slices also subsets the exclude_varnames attribute",
   )
 })
 
+testthat::test_that("[.teal_slices doesn't subset the include_varnames attribute according to available teal_slice", {
+  shiny::reactiveConsole(TRUE)
+  on.exit(shiny::reactiveConsole(FALSE))
+
+  fs1 <- teal_slice(dataname = "data1", varname = "var1")
+  fs2 <- teal_slice(dataname = "data1", varname = "var2")
+  fs3 <- teal_slice(dataname = "data2", varname = "var1")
+  fs4 <- teal_slice(dataname = "data2", varname = "var2")
+  fs <- teal_slices(fs1, fs2, fs3, fs4, include_varnames = list(data1 = "var1", data2 = "var1"))
+
+  testthat::expect_identical(
+    attr(fs[1], "include_varnames"),
+    list(data1 = "var1", data2 = "var1")
+  )
+  testthat::expect_identical(
+    attr(fs[2], "include_varnames"),
+    list(data1 = "var1", data2 = "var1")
+  )
+  testthat::expect_identical(
+    attr(fs[1:2], "include_varnames"),
+    list(data1 = "var1", data2 = "var1")
+  )
+  testthat::expect_identical(
+    attr(fs[3], "include_varnames"),
+    list(data1 = "var1", data2 = "var1")
+  )
+  testthat::expect_identical(
+    attr(fs[4], "include_varnames"),
+    list(data1 = "var1", data2 = "var1")
+  )
+  testthat::expect_identical(
+    attr(fs[3:4], "include_varnames"),
+    list(data1 = "var1", data2 = "var1")
+  )
+  testthat::expect_identical(
+    attr(fs[], "include_varnames"),
+    list(data1 = "var1", data2 = "var1")
+  )
+})
 
 testthat::test_that("[.teal_slices preserves count_type", {
   shiny::reactiveConsole(TRUE)
diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R
index 0e5092e25..bbc2e47a4 100644
--- a/tests/testthat/test-utils.R
+++ b/tests/testthat/test-utils.R
@@ -87,7 +87,7 @@ test_that("check_simple_name behaves as expected", {
 
 # get_teal_bs_theme ----
 testthat::test_that("get_teal_bs_theme", {
-  testthat::expect_true(is.null(get_teal_bs_theme()))
+  testthat::expect_null(get_teal_bs_theme())
   withr::with_options(list("teal.bs_theme" = bslib::bs_theme(version = "5")), {
     testthat::expect_s3_class(get_teal_bs_theme(), "bs_theme")
   })

From 9fa21264b077488538783724e46d553514ba6a89 Mon Sep 17 00:00:00 2001
From: go_gonzo <dawid.kaledkowski@gmail.com>
Date: Tue, 11 Jul 2023 14:47:36 +0200
Subject: [PATCH 4/4] @chlebowa comments

fix srv_active
---
 R/FilterState.R            |  1 -
 R/FilterStates.R           | 43 +++++++++++++++-------------
 R/FilteredData.R           | 57 +++++++++++++-------------------------
 R/FilteredDatasetDefault.R |  2 +-
 R/teal_slices.R            | 16 +++++++++++
 man/setdiff_teal_slices.Rd | 18 ++++++++++++
 6 files changed, 78 insertions(+), 59 deletions(-)
 create mode 100644 man/setdiff_teal_slices.Rd

diff --git a/R/FilterState.R b/R/FilterState.R
index 9958fe7e5..78a885eb8 100644
--- a/R/FilterState.R
+++ b/R/FilterState.R
@@ -205,7 +205,6 @@ FilterState <- R6::R6Class( # nolint
         id = id,
         function(input, output, session) {
           logger::log_trace("FilterState$server initializing module for slice: { private$get_id() } ")
-
           private$server_summary("summary")
           if (private$is_fixed()) {
             private$server_inputs_fixed("inputs")
diff --git a/R/FilterStates.R b/R/FilterStates.R
index e2299f7a4..ae0a67a49 100644
--- a/R/FilterStates.R
+++ b/R/FilterStates.R
@@ -340,47 +340,52 @@ FilterStates <- R6::R6Class( # nolint
         function(input, output, session) {
           logger::log_trace("FilterState$srv_active initializing, dataname: { private$dataname }")
           current_state <- reactive(private$state_list_get())
-          previous_state <- reactiveVal(character(0))
-          added_state_name <- reactiveVal(character(0))
+          previous_state <- reactiveVal(NULL) # FilterState list
+          added_states <- reactiveVal(NULL) # FilterState list
 
-          str_to_shiny_ns <- function(x) {
-            gsub("[^[:alnum:]]+", "_", x)
+          # gives a valid shiny ns based on a default slice id
+          fs_to_shiny_ns <- function(x) {
+            checkmate::assert_multi_class(x, c("FilterState", "FilterStateExpr"))
+            gsub("[^[:alnum:]]+", "_", get_default_slice_id(x$get_state()))
           }
 
           output$trigger_visible_state_change <- renderUI({
-            current_state <- current_state()
+            current_state()
             isolate({
               logger::log_trace("FilterStates$srv_active@1 determining added and removed filter states")
-              added_state_name(setdiff(names(current_state()), names(previous_state())))
+              # Be aware this returns a list because `current_state` is a list and not `teal_slices`.
+              added_states(setdiff_teal_slices(current_state(), previous_state()))
               previous_state(current_state())
               NULL
             })
           })
 
           output[["cards"]] <- shiny::renderUI({
-            fstates <- current_state() # rerenders when queue changes / not when the state changes
-            lapply(names(fstates), function(fname) {
-              shiny::isolate(
-                fstates[[fname]]$ui(id = session$ns(str_to_shiny_ns(fname)), parent_id = session$ns("cards"))
-              )
-            })
+            lapply(
+              current_state(), # observes only if added/removed
+              function(state) {
+                shiny::isolate( # isolates when existing state changes
+                  state$ui(id = session$ns(fs_to_shiny_ns(state)), parent_id = session$ns("cards"))
+                )
+              }
+            )
           })
 
           observeEvent(
-            added_state_name(), # we want to call FilterState module only once when it's added
+            added_states(), # we want to call FilterState module only once when it's added
             ignoreNULL = TRUE,
             {
-              logger::log_trace("FilterStates$srv_active@2 triggered by added states: { toString(added_state_name()) }")
-              fstates <- current_state()
-              lapply(added_state_name(), function(fname) {
-                fs_callback <- fstates[[fname]]$server(id = str_to_shiny_ns(fname))
+              added_state_names <- vapply(added_states(), function(x) x$get_state()$id, character(1L))
+              logger::log_trace("FilterStates$srv_active@2 triggered by added states: { toString(added_state_names) }")
+              lapply(added_states(), function(state) {
+                fs_callback <- state$server(id = fs_to_shiny_ns(state))
                 observeEvent(
                   eventExpr = fs_callback(), # when remove button is clicked in the FilterState ui
                   once = TRUE, # remove button can be called once, should be destroyed afterwards
-                  handlerExpr = private$state_list_remove(fname)
+                  handlerExpr = private$state_list_remove(state$get_state()$id)
                 )
               })
-              added_state_name(character(0))
+              added_states(NULL)
             }
           )
 
diff --git a/R/FilteredData.R b/R/FilteredData.R
index b4e108ef7..b326a5b32 100644
--- a/R/FilteredData.R
+++ b/R/FilteredData.R
@@ -475,8 +475,6 @@ FilteredData <- R6::R6Class( # nolint
         }
 
         checkmate::assert_class(state, "teal_slices")
-        datanames <- slices_field(state, "dataname")
-        checkmate::assert_subset(datanames, self$datanames())
         module_add <- attr(state, "module_add")
         if (!is.null(module_add)) {
           private$module_add <- module_add
@@ -1095,8 +1093,8 @@ FilteredData <- R6::R6Class( # nolint
           slice_reference <- vapply(private$available_teal_slices(), get_default_slice_id, character(1))
           is_duplicated_reference <- duplicated(slice_reference) | duplicated(slice_reference, fromLast = TRUE)
           is_active <- available_slices_id() %in% active_slices_id()
-          is_not_expr <- vapply(private$available_teal_slices(), inherits, logical(1), "teal_slice_expr")
-          slice_reference[is_duplicated_reference & is_active & !is_not_expr]
+          is_not_expr <- !vapply(private$available_teal_slices(), inherits, logical(1), "teal_slice_expr")
+          slice_reference[is_duplicated_reference & is_active & is_not_expr]
         })
 
         checkbox_group_element <- function(name, value, label, checked, disabled = FALSE) {
@@ -1124,48 +1122,31 @@ FilteredData <- R6::R6Class( # nolint
           )
           active_slices_ids <- active_slices_id()
           duplicated_slice_refs <- duplicated_slice_references()
-          interactive_choice_mock <- lapply(
-            slices_interactive(),
-            function(slice) {
-              # we need to isolate changes in the fields of the slice (teal_slice)
-              shiny::isolate({
-                checkbox_group_element(
-                  name = session$ns("available_slices_id"),
-                  value = slice$id,
-                  label = slice$id,
-                  checked = if (slice$id %in% active_slices_ids) "checked",
-                  disabled = slice$locked ||
-                    get_default_slice_id(slice) %in% duplicated_slice_refs &&
-                      !slice$id %in% active_slices_ids
-                )
-              })
-            }
-          )
 
-          non_interactive_choice_mock <- lapply(
-            slices_fixed(),
-            function(slice) {
-              # we need to isolate changes in the fields of the slice (teal_slice)
-              isolate(
-                checkbox_group_element(
-                  name = session$ns("available_slices_id"),
-                  value = slice$id,
-                  label = slice$id,
-                  checked = if (slice$id %in% active_slices_ids) "checked",
-                  disabled = slice$locked ||
-                    get_default_slice_id(slice) %in% duplicated_slice_refs &&
-                      !slice$id %in% active_slices_ids
-                )
+          checkbox_group_slice <- function(slice) {
+            # we need to isolate changes in the fields of the slice (teal_slice)
+            shiny::isolate({
+              checkbox_group_element(
+                name = session$ns("available_slices_id"),
+                value = slice$id,
+                label = slice$id,
+                checked = if (slice$id %in% active_slices_ids) "checked",
+                disabled = slice$locked ||
+                  get_default_slice_id(slice) %in% duplicated_slice_refs &&
+                    !slice$id %in% active_slices_ids
               )
-            }
-          )
+            })
+          }
+
+          interactive_choice_mock <- lapply(slices_interactive(), checkbox_group_slice)
+          non_interactive_choice_mock <- lapply(slices_fixed(), checkbox_group_slice)
 
           htmltools::tagInsertChildren(
             checkbox,
             br(),
             tags$strong("Fixed filters"),
             non_interactive_choice_mock,
-            tags$strong("Iteractive filters"),
+            tags$strong("Interactive filters"),
             interactive_choice_mock,
             .cssSelector = "div.shiny-options-group",
             after = 0
diff --git a/R/FilteredDatasetDefault.R b/R/FilteredDatasetDefault.R
index abeb10c2a..73b8fd43c 100644
--- a/R/FilteredDatasetDefault.R
+++ b/R/FilteredDatasetDefault.R
@@ -183,10 +183,10 @@ DefaultFilteredDataset <- R6::R6Class( # nolint
     set_filter_state = function(state) {
       shiny::isolate({
         logger::log_trace("{ class(self)[1] }$set_filter_state initializing, dataname: { private$dataname }")
+        checkmate::assert_class(state, "teal_slices")
         lapply(state, function(slice) {
           checkmate::assert_true(slice$dataname == private$dataname)
         })
-        checkmate::assert_class(state, "teal_slices")
         private$get_filter_states()[[1L]]$set_filter_state(state = state)
         invisible(NULL)
       })
diff --git a/R/teal_slices.R b/R/teal_slices.R
index f3e61eca0..e306a47fc 100644
--- a/R/teal_slices.R
+++ b/R/teal_slices.R
@@ -300,3 +300,19 @@ slices_to_list <- function(tss) {
   tss_list <- list(slices = slices_list, attributes = attrs)
   Filter(Negate(is.null), tss_list) # drop attributes if empty
 }
+
+#' `setdiff` method for `teal_slices`
+#'
+#' Compare two teal slices objects and return `teal_slices` containing slices present in `x` but not in `y`.
+#' @param x,y `teal_slices` objects
+#' @return `teal_slices`
+#' @keywords internal
+#'
+setdiff_teal_slices <- function(x, y) {
+  Filter(
+    function(xx) {
+      !any(vapply(y, function(yy) identical(yy, xx), logical(1)))
+    },
+    x
+  )
+}
diff --git a/man/setdiff_teal_slices.Rd b/man/setdiff_teal_slices.Rd
new file mode 100644
index 000000000..74a0f9c6b
--- /dev/null
+++ b/man/setdiff_teal_slices.Rd
@@ -0,0 +1,18 @@
+% Generated by roxygen2: do not edit by hand
+% Please edit documentation in R/teal_slices.R
+\name{setdiff_teal_slices}
+\alias{setdiff_teal_slices}
+\title{\code{setdiff} method for \code{teal_slices}}
+\usage{
+setdiff_teal_slices(x, y)
+}
+\arguments{
+\item{x, y}{\code{teal_slices} objects}
+}
+\value{
+\code{teal_slices}
+}
+\description{
+Compare two teal slices objects and return \code{teal_slices} containing slices present in \code{x} but not in \code{y}.
+}
+\keyword{internal}