Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request]: Implement the mechanism to flag filter-states with some id #137

Closed
3 tasks done
Tracked by #6 ...
gogonzo opened this issue Dec 8, 2022 · 11 comments
Closed
3 tasks done
Tracked by #6 ...
Assignees
Labels
core enhancement New feature or request

Comments

@gogonzo
Copy link
Contributor

gogonzo commented Dec 8, 2022

Feature description

Purpose:
We need a mechanism which allow to group filter-state(s) to obtain:

  • module specific code get_data(<module id>), get_code(<module id>).
  • module specific data
  • display only module specific filter cards
  • get_filter_state should return only module specific filters

image

We need a mechanism within FilterStates which controls which filter-state belong to which module. Name "module" in the filter-panel shouldn't be used, we need a name of this attribute to be more general (flag, target, you name it).

What I know for sure is that this attribute should be a reactive value, because in the app user will be able to enable/disable this filter in any module.

Code of Conduct

  • I agree to follow this project's Code of Conduct.

Contribution Guidelines

  • I agree to follow this project's Contribution Guidelines.

Security Policy

  • I agree to follow this project's Security Policy.
@gogonzo gogonzo added the enhancement New feature or request label Dec 8, 2022
@gogonzo gogonzo added this to the Module specific filter-panel milestone Dec 8, 2022
@donyunardi
Copy link
Contributor

donyunardi commented Dec 8, 2022

Acceptance Criteria:

  • Each filter state should have a relation to a teal module
  • get_data(module id), get_code(module id), get_filter_state(module_id) should return items related to the module_id
  • In the ui only module specific FilterState cards should be displayed.

@gogonzo
Copy link
Contributor Author

gogonzo commented Dec 16, 2022

Added blocked label because we need to decide how to set the information, through:

  • teal::init(filters = filter_settings(...))
  • set_filter_state
  • how to set the information in the running application

@gogonzo
Copy link
Contributor Author

gogonzo commented May 11, 2023

@lcd2yyz @donyunardi @asbates @chlebowa

Please imagine two modules groups modules1 and module2 and some set of filters which are mapped with modules. There are (at least) two ways to achieve module-specific.

  1. Two group of modules have a separate set of filters and changing one filter in one group doesn't influence filters in another group. API could look like below. This could be implementent solely in teal as we would create n-FilteredData for n-groups (n independent filter panels for n-groups)
teal::init(
  modules(
    # modules,
    filters = filter_settings(
      filter_var(...),
      filter_var(...)
    )
  ),
  modules(
    #modules,
    filter_settings(
      filter_var(...),
      filter_var(...)
    )
  )
)
  1. Two groups of modules share some filters. Then single FilterState need to be linked with several modules. Some first thoughts below:
filter_settings(
  filter_var(..., id = "filter1"),
  filter_var(..., id = "filter2"),
  filter_var(..., id = "filter3"),
  filter_var(..., id = "filter4"),
)


teal::init(
  modules = list(
    modules("modules1", ..., filters = c("filter1", "filter2", "filter3"),
    modules("modules2", ..., filters = c("filter1", "filter2", "filter4")
  )
)

or

filter_settings(
  filter_var(..., external_id = c("modules1", "modules2")),
  filter_var(..., external_id = c("modules1", "modules2")),
  filter_var(..., external_id = "modules1")),
  filter_var(..., external_id = "modules2"))
)

teal::init(
  modules = list(
    modules(..., "modules1"),
    modules(..., "modules2")
  )
)

Advantage of the 1st is that it won't introduce any extra code in teal.slice. Advantage of the 2nd is wider customization. I'd like to discuss how to match module names with filters as currently it looks like this:


app <- init(
  data = data,
  modules = list(
    some_module("module1"),
    some_module("module2")
  ),
  filter = filter_settings(
    ...
  )
)

Any thoughts? Thanks

@asbates
Copy link
Contributor

asbates commented May 11, 2023

I think a combination of 1. and 2. would be nice.

In 1. I like how a filter_settings is passed to each module. I prefer that over c("filter1", "filter2") in 2.

In 2. I like how it gives more options for users.

I think my ideal merger of the two would be

module_1_filters <- filter_settings(
  filter_var(..., id = "filter1"),
  filter_expression(..., id = "expr1")
)
module_2_filters <- filter_settings(
  filter_var(..., id = "filter1"),
  filter_var(..., id = "filter2")
)

teal::init(
  modules(
    # modules,
    module_1_filters
  ),
  modules(
    #modules,
    filters = module_2_filters
  )
)

I particularly like having an id for all filters because the more similarity between filter types the easier it will be for users and developers to understand.

@donyunardi
Copy link
Contributor

donyunardi commented May 11, 2023

Option 1 is good but this means the teal_slice object will be created in each module, and this will make our teal modules dependent with teal.slice.

With option 2, we can keep them separate and we only need to surface the id for each module, so I prefer this option.
Also, I like option 2 because I can see all filters configuration in one spot and module's id can be reusable for other needs (i.e. front-end stuff, etc).

Here's an example how this could possibly look like

my_filters <- filter_settings(
  # settings for module 1 and 2
  filter_var(..., module_id = c("module1", "module2")),
  
  # settings for module 1
  filter_var(..., module_id = "module1")),
  
  # settings for module 2
  filter_var(..., module_id = "module2")),
  
  # settings for all modules (module 1, 2, and 3)
  filter_var(...)
)

app <- teal::init(
  data = cdisc_data(),
  modules = modules(
    tm_t_summary(..., id = "module1"),
    tm_g_lineplot(..., id = "module2"),
    tm_g_km(..., id = "module3")
  ),
  filter = my_filters
)

@asbates
Copy link
Contributor

asbates commented May 12, 2023

@donyunardi I'm not sure the module packages would need a dependency on teal.slice. I'm not sure how it would be different than right now since init and modules are part of teal. So teal could route everything where it needs to go.

@donyunardi
Copy link
Contributor

donyunardi commented May 12, 2023

For option 1, won't the init::(modules=)argument looks something like this:

app <- teal::init(
  data = cdisc_data(),
  modules = modules(
    tm_t_summary(..., filter = filter_settings()),
    tm_g_lineplot(..., filter = filter_settings()),
    tm_g_km(..., filter = filter_settings())
  ),
  filter = some_filter
)

If so, then won't teal_slice object has to be created in tm_t_summary?

@asbates
Copy link
Contributor

asbates commented May 12, 2023

For option 1, won't the init::(modules=)argument looks something like this:

app <- teal::init(
  data = cdisc_data(),
  modules = modules(
    tm_t_summary(..., filter = filter_settings()),
    tm_g_lineplot(..., filter = filter_settings()),
    tm_g_km(..., filter = filter_settings())
  ),
  filter = some_filter
)

If so, then won't teal_slice object have to be created in tm_t_summary?

Ooooh. Wow, was I way off :).

@lcd2yyz
Copy link

lcd2yyz commented May 12, 2023

Definitely Option 2 for me. But before deciding on option 2.1 (using module id) vs 2.2 (using filter id), perhaps we need to think more in-depth about not only the default settings but also usage during app run.

I do like Dony's example code, it's very clear and clean way of assigning default filters to apply to specified modules. In fact, it can easily be applied to a module set (eg. all AE modules), when multiple modules are nested under a module set tab.

On the other hand, I'm wondering if filter ID may be a better approach if we want to allow app users to activate/apply filter from a filter queue, or to allow changes made in one interactive filter to be automatically applied to other modules with the same interactive filter? Would it be easier to keep track of the changes in each individual filter state by associating it with an ID?

@gogonzo
Copy link
Contributor Author

gogonzo commented May 15, 2023

@lcd2yyz

Definitely Option 2 for me. But before deciding on option 2.1 (using module id) vs 2.2 (using filter id), perhaps we need to think more in-depth about not only the default settings but also usage during app run.

2.1 has better encapsulation as filter_var (FilterState) have id which refer to itself, while external_id refers to the entity from outside of the filter-panel. Because of the "hierarchical filtering" we have a dilemma as independent FilterState would not be able to associate filters belonging to the same module (as they have no information about the module they are in.
It's heartbreaking but I think we need to smuggle external_id information to the FilterState (option 2.2) but I will think about this more.

@donyunardi @asbates
I think each filter_settings should create an independent filter-panel. So if we pass filter_settings to the module (or modules), then:

  • changes of particular filter in one module (or modules) wouldn't be reflected in the other
  • If somehow we create a one global filter-panel as outout of multiple filter_settings, then we would have to deal with conflicts in filter_settings specification. For example filter_settings(count_type = "all") and filter_settings(count_type = "none") -> which option should be set in one global filter-panel?

@lcd2yyz
Copy link

lcd2yyz commented May 15, 2023

Definitely Option 2 for me. But before deciding on option 2.1 (using module id) vs 2.2 (using filter id), perhaps we need to think more in-depth about not only the default settings but also usage during app run.

Sorry I just realized I made a really stupid mistake in my reply previously 🙈 - In referring options, I completely flipped the order you've listed the options. I meant to say 2.1 = filter ID, option 2.2 = module ID (external_id that @gogonzo was referring to). Just to state this correction, to avoid any confusion.

gogonzo added a commit that referenced this issue Jun 16, 2023
Closes #135 and #137 

1. `FilterStates$srv_active` uses `renderUI` instead of `insertUI`. 
2. `teal_slice` has now obligatory `id` field.
3. `teal_slice` object is now a `reactiveValues` which is passed and
stored directly to `FilterState`. This `teal_slice` object can be
returned using `FilterState$get_state`. This gives possibility that one
`teal_slice` object can be present in the same time in multiple
`FilterState` object. This simplifies little constructor of
`FilterState` as we don't have to have check again assertions on
dataname, varname etc.
4. removed `disabled` functionality as it conflicts with module specific
feature. Discussed with @lcd2yyz and we decided to develop some
alternatives to quickly activate/deactivate filters. See (5)
5. added `set_available_teal_slice` (public) and
`ui/srv_available_filters` to link `srv/ui_active` with some reactive
list of "available" slices. This gives a possibility to
activate/deactivate particular state. This reactive list of states is
set in teal which gathers all modules filters and creates a one unique
list of available filters.


Co-authored-by: Aleksander Chlebowski <aleksander.chlebowski@contractors.roche.com>
@gogonzo gogonzo closed this as completed Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants