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

[Bug]: TealAppDriver setting numeric filter state #1151

Closed
averissimo opened this issue Mar 13, 2024 · 7 comments · Fixed by #1152
Closed

[Bug]: TealAppDriver setting numeric filter state #1151

averissimo opened this issue Mar 13, 2024 · 7 comments · Fixed by #1152
Assignees
Labels
bug Something isn't working core

Comments

@averissimo
Copy link
Contributor

averissimo commented Mar 13, 2024

TealAppDriver$set_active_filter_selection only works for categorical variables. It should also work for numeric ranges.

Example below of test that should work

testthat::test_that("e2e: Set numeric filter state", {
  app <- TealAppDriver$new(
    data = simple_teal_data(),
    modules = example_module(label = "Example Module"),
    timeout = default_idle_timeout
  )
  withr::defer(app$stop())

  app$wait_for_idle()

  app$add_filter_var("iris", "Sepal.Length")
  app$view()
  app$set_active_filter_selection("iris", "Sepal.Length", c(4, 5), is_numeric = TRUE)
  app$wait_for_idle()
  testthat::expect_equal(
    app$get_active_filter_selection("iris", "Sepal.Length", is_numeric = TRUE), 
    c(4, 5)
  )
})
@averissimo averissimo added bug Something isn't working core labels Mar 13, 2024
@averissimo averissimo self-assigned this Mar 13, 2024
@chlebowa
Copy link
Contributor

 app$set_active_filter_selection("iris", "Sepal.Length", c(4, 5), is_numeric = TRUE)

Are you sure you don't want to pass a teal_slice?

@averissimo
Copy link
Contributor Author

averissimo commented Mar 13, 2024

That's a nice idea passing a teal_slice. It's familiar and avoids that is_numeric weirdness.

app$set_active_filter_selection(
  teal_slice("iris", "Sepal.Length", selected = c(4, 5))
)

The operations that this call triggers would still be UI/input-based, but this would allow us to keep consistency.

Care to give insights on reasons to keep current formals @vedhav ?

ps. this would be a separate issue though as potential follow-up to this one.

@averissimo
Copy link
Contributor Author

averissimo commented Mar 13, 2024

The shinyWidgets::numericRangeInput widget requires a custom handler to change the values.

I was able to do it via javascript (#1152), but if someone knows how to hint the handler via Shiny API, please chip in.

The caveat of doing it this way is that the input boxes still hold the previous values.

image

image

Side quest: The range input does not work out of the box with shinytest2, but it has no issues with dynamic rendering as we do it 😁 rstudio/shinytest2#376

@chlebowa
Copy link
Contributor

Yeah, $setInputValue and $setInput are not entirely reliable. But doesn't shinyWidgets::updateNumericRangeInput, which uses $sendInputMessage, work?

@averissimo
Copy link
Contributor Author

For that we need access to the session object, which I don't think we can easily access :-\

@chlebowa
Copy link
Contributor

Oh, I see. I misunderstood the problem. Sorry.

@averissimo
Copy link
Contributor Author

averissimo commented Mar 14, 2024

I wasn't totally explicit in the description.

The issue here is that AppDriver API does nothing when setting for the input_id.

  • It seems that it sets them via javascript call via chromote (I'm not 100% sure though, feel free to correct me)
  • In turn, the javascript call via Shiny.setInput needs the handler suffix for some complex cases (such as the range input)

So the downstream javascript call needs to be Shiny.setInput("an_input:sw.numericRange", [min, max])

Which is what the PR does skipping all middle layers of AppDriver.

averissimo added a commit that referenced this issue Mar 20, 2024
# Pull Request

<!--- Replace `#nnn` with your issue link for reference. -->

Fixes #1151 

#### Changes description

- `shinyWidgets::numericRangeInput` uses a custom handler and seems to
require a `js: Shiny.setInputValue` call
- ~Change of explicit arguments in `{s,g}et_active_filter_selection` to
`type` to reflect this logic and allow for further extensions.~
- Removed `is_numeric` argument in favor of auto-detection of slice type
(categorical / numerical range)
  - from `{s,g}et_active_filter_selection`

---------

Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Vedha Viyash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants