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

Better result field #421

Merged
merged 10 commits into from
Oct 7, 2024
Merged

Better result field #421

merged 10 commits into from
Oct 7, 2024

Conversation

nbenn
Copy link
Collaborator

@nbenn nbenn commented Sep 25, 2024

Change result field implementation such that

  • we no longer need a reactive poll per result field for the data,
  • the (serialized) field "value" is only the stack name the field refers to (not data),
  • (optional) we no longer need a reactive poll for the stack selector drop-down.

@nbenn
Copy link
Collaborator Author

nbenn commented Sep 25, 2024

@DivadNojnarg this is a (hacky) start to removing the reactive polls. I think it mostly works (although I am not super confident I did not break any edge-cases).

Previously the "value" of a result field was data. Now it is a string (referring to some stack in the workspace). This comes with a (major) breaking change: while before you could happily do something like

blk2 <- new_join_block(daty, type = "inner", by = "name")

it no longer makes sense to consider a result_field object outside a workspace context. Furthermore, trying to remove the reactive poll for stack names, I "installed" a workspace-level reactive and this now leaves us with something semi-functional in a "result-field-server"-only context like here

shiny::testServer(
generate_server(new_result_field()), {
expect_setequal(opts(), c("stack1", "stack2"))
}
)
})

which I worked around with

blockr/R/server.R

Lines 38 to 42 in b18f427

if (is.null(workspace_stacks)) {
workspace_stacks <- function() {
list_workspace_stacks()
}
}

(Not sure this is a good idea, but it makes the problem go away.)

I'm currently left with 2 shinytest2-related issues:

  • The tests (on gh actions) currently fail due to changes in snapshots. Unfortunately I'm having trouble updating the snapshots due to (unrelated) bootstrap version change issues

    Diff in snapshot file `blockblock-app-005.json`
    < before
    > after
    @@ 17,5 / 17,5 @@
          "my_stack-block_1-res": {
            "x": {
    <         "style": "bootstrap5",
    >         "style": "bootstrap4",
              "filter": "none",
              "vertical": false,
    @@ 40,41 / 40,5 @@
    

    I don't understand why my local environment causes a version downgrade. I did upgrade to the newest bslib, but this did not help. I saw that in some places we explicitly set bslib::bs_theme(5L). Perhaps in other places this could also depend on the session running the app?

  • I tried adding my own shinytest2 test for the result field (as the previous testServer-approach is kind of pointless with the new implementation). For some reason I cannot get the AppDriver$new()-call to produce a "stable" app: I always get Shiny app did not become stable in x ms. I tried figuring out whether this comes from some issue I introduce with my change or whether this is a problem with my local shinytest2 setup. Not sure what's going on currently. Do you have any ideas? Can you run the test at

    app <- AppDriver$new(
    system.file("examples/result/app.R", package = "blockr"),
    name = "result-app"
    )

@nbenn
Copy link
Collaborator Author

nbenn commented Sep 25, 2024

One more thing for a separate PR: I am now showing both the stack name and ID in the drop-down. This raises another issue for the base-UI: we have no way of setting the stack name in a running app and all stacks (created via UI) are called "Stack".

@DivadNojnarg
Copy link
Collaborator

@nbenn:

  1. To get shinytest2 running, you need to run devtools::install() to make sure your tests are running again the latest package version. I initially forgot to do this and got the same error as you.

  2. To set the stack names in a running app @JohnCoene made something long time ago. If you click on the stack title, you should be able to edit it. However, this updates only the UI. Is this something we want to bring in this PR too?

@JohnCoene
Copy link
Member

@DivadNojnarg updating the title is captured I think, it sets the attribute correctly.

@DivadNojnarg
Copy link
Collaborator

@nbenn and @JohnCoene

  • I fixed the test issues.
  • Also added some part to account for a title change from the UI. Replaced the reactive by eventReactive and listening to any stack title attribute change. That was the missing piece causing the result field dropdown not to update properly.

I think, from the UI perspective, it is not obvious that one can click on the title to change it. Maybe we need to add some guidance or a hint about what can be done with the title (like a tooltip "Click to update me...").

dplyr,
utils,
bslib,
bslib (>= 0.7.0),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

necessary to get the bslib::input_task_button added in the submit PR. To avoid confusing users.

@nbenn nbenn marked this pull request as ready for review October 7, 2024 13:02
@nbenn
Copy link
Collaborator Author

nbenn commented Oct 7, 2024

@DivadNojnarg need your approval, and then we can merge. Tanks for your inputs. Forgot that for shinytest2 we do not have loal_all() <=> install().

Copy link
Collaborator

@DivadNojnarg DivadNojnarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

@nbenn nbenn merged commit c1ef52e into main Oct 7, 2024
5 checks passed
@nbenn nbenn deleted the 366-result-field branch October 7, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants