First pass into workspace UI/server #124

DivadNojnarg · 2023-12-08T22:17:37Z

TO DO:

add tests.
~~add way to edit stacks: pass blocks~~.
~~Important: Cleanup stack inputs? observers and output (like for blocks). Below is how input looks like after stacks are added, cleared and reset~~ (OUT OF THE SCOPE OF THIS PR).

<ReactiveValues> 
  Values:    myworkspace-add_stack, myworkspace-clear_stacks, myworkspace-mystack-1-jgnzeezgtmhotlp-dataset, myworkspace-mystack-1-jgnzeezgtmhotlp-remove, myworkspace-mystack-1-jgnzeezgtmhotlp-res_cell_clicked, myworkspace-mystack-1-jgnzeezgtmhotlp-res_cells_selected, myworkspace-mystack-1-jgnzeezgtmhotlp-res_columns_selected, myworkspace-mystack-1-jgnzeezgtmhotlp-res_rows_all, myworkspace-mystack-1-jgnzeezgtmhotlp-res_rows_current, myworkspace-mystack-1-jgnzeezgtmhotlp-res_rows_selected, myworkspace-mystack-1-jgnzeezgtmhotlp-res_search, myworkspace-mystack-1-jgnzeezgtmhotlp-res_state, myworkspace-mystack-1-last_changed, myworkspace-mystack-1-remove, myworkspace-mystack-1-title, myworkspace-mystack-2-last_changed, myworkspace-mystack-2-remove, myworkspace-mystack-2-title, myworkspace-mystack-2-wpmbppudytffvdu-dataset, myworkspace-mystack-2-wpmbppudytffvdu-remove, myworkspace-mystack-2-wpmbppudytffvdu-res_cell_clicked, myworkspace-mystack-2-wpmbppudytffvdu-res_cells_selected, myworkspace-mystack-2-wpmbppudytffvdu-res_columns_selected, myworkspace-mystack-2-wpmbppudytffvdu-res_rows_all, myworkspace-mystack-2-wpmbppudytffvdu-res_rows_current, myworkspace-mystack-2-wpmbppudytffvdu-res_rows_selected, myworkspace-mystack-2-wpmbppudytffvdu-res_search, myworkspace-mystack-2-wpmbppudytffvdu-res_state, myworkspace-mystack-3-last_changed, myworkspace-mystack-3-remove, myworkspace-mystack-3-title, myworkspace-mystack-3-uozygoqqaknabbd-dataset, myworkspace-mystack-3-uozygoqqaknabbd-remove, myworkspace-mystack-3-uozygoqqaknabbd-res_cell_clicked, myworkspace-mystack-3-uozygoqqaknabbd-res_cells_selected, myworkspace-mystack-3-uozygoqqaknabbd-res_columns_selected, myworkspace-mystack-3-uozygoqqaknabbd-res_rows_all, myworkspace-mystack-3-uozygoqqaknabbd-res_rows_current, myworkspace-mystack-3-uozygoqqaknabbd-res_rows_selected, myworkspace-mystack-3-uozygoqqaknabbd-res_search, myworkspace-mystack-3-uozygoqqaknabbd-res_state 
  Readonly:  TRUE

codecov · 2023-12-08T22:22:14Z

Codecov Report

Attention: 125 lines in your changes are missing coverage. Please review.

Comparison is base (f1dddf9) 64.90% compared to head (a1c4c93) 75.66%.

Files	Patch %	Lines
R/server.R	8.33%	66 Missing ⚠️
R/ui.R	10.60%	59 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #124       +/-   ##
===========================================
+ Coverage   64.90%   75.66%   +10.76%     
===========================================
  Files          16       16               
  Lines        1681     1796      +115     
===========================================
+ Hits         1091     1359      +268     
+ Misses        590      437      -153

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DivadNojnarg · 2023-12-08T22:23:03Z

R/workspace.R

@@ -12,7 +12,7 @@ set_workspace <- function(..., title = "", settings = NULL, force = FALSE) {

  set_workspace_title(title)
  set_workspace_settings(settings)
-  set_workpsace_stacks(list(...))
+  set_workspace_stacks(list(...))


minor typo fixed

DivadNojnarg · 2023-12-08T22:25:27Z

R/ui.R

+
+  ns <- NS(id)
+
+  stacks <- get_workspace_stacks()


this feels actually a bit weird to have to get the stacks this way since workspace is not a structure like new_stack or new_block. I can make it work but this does not feel super in line with the rest of our API.

DivadNojnarg · 2023-12-08T22:26:31Z

R/ui.R

+      div(
+        class = "d-flex justify-content-between stacks",
+        lapply(seq_along(stacks), \(i) {
+          generate_ui(stacks[[i]], id = ns(sprintf("mystack-%s", i)))


Same as above. I would have expected to loop over x (the workspace) and not over the result of get_workspace_stacks.

DivadNojnarg · 2023-12-11T08:32:56Z

Note: #119 is needed to cleanup module server after being removed (cc @nbenn)

DivadNojnarg · 2024-01-10T17:13:29Z

Still to do:

~~maybe add the "Add block button" within the stack card~~ DONE.
~~improve layout~~ DONE.

DivadNojnarg · 2024-01-10T22:58:40Z

Few notes for @nbenn:

We'll need to handle the server conflicts with the main branch: from what we discussed, I propose to accept both incoming and current to be able to use the old approach for generate_server.
Now that we have the workspace, stacks, blocks, ... a good time to rethink the validation process.
This is a basic/default workspace implementation. @JohnCoene has his own with the super framework.

nbenn

Lgtm. We should tweak the workspace API a bit. If in your scope you have the workspace available as x, it feels wrong to call e.g. get_workspace_stacks() instead of get_workspace_stacks(x). Happy to discuss your thoughts on whether the workspace object should remain implemented as environment or not.

nbenn · 2024-01-11T16:04:28Z

R/ui.R

+
+  ns <- NS(id)
+
+  stacks <- get_workspace_stacks()


This is more a note-to-self: my workspace API here is not good. If the workspace is passed as x you'd want to be able to call get_workspace_stacks() as get_workspace_stacks(x).

Yes this is weird. We can leave it as is if the time constaint is too high with other priorities. Anyway we still have #92 open so we can add a bullet point here.

nbenn · 2024-01-11T16:14:25Z

R/server.R

+  }, {
+    # We can't remove the data block if there are downstream consumers...
+    stacks <- get_workspace_stacks()
+    to_remove <- which(chr_ply(stacks, \(x) attr(x, "name")) == id)


This is not wrong. From a design-perspective, it might be worthwhile to consider either passing the index from the parent scope, where it should be more easily available, no? This is also in anticipation of maybe not implementing the workspace as environment.

nbenn · 2024-01-11T16:21:05Z

R/server.R

+  moduleServer(
+    id = id,
+    function(input, output, session) {
+      vals <- reactiveValues(stacks = list(), new_blocks = list())


More of a naming issue (probably my understanding here is incomplete), but why the plural? Does new_blocks actually hold on to multiple new blocks at the same time?

Yes that's TRUE. This is a wrong naming because to the best of my knowledge, we only add 1 block at a time. Can you confirm @JohnCoene? That should also apply to the stack server as well:

#' @param new_blocks For dynamically inserted blocks. #' @export generate_server.stack <- function(x, id = NULL, new_blocks = NULL, ...) { ... }

I can change it.

Yes, this is correct.

I cannot run generate_server on the workspace directly. I need to regenerate their UI on the correct tabs and their servers in their correct tabs/modules as well as pass the adequate new_block reactive to each server.

nbenn · 2024-01-11T16:28:49Z

R/ui.R

+      radioButtons(
+        ns("selected_block"),
+        "Choose a block",
+        choices = setNames(seq_len(length(list_blocks())), list_blocks()),


We have set_names() in utils.R, b/c I thought it was unnecessary to import an entire package for a 2-line function. This effort was thwarted by @christophsax in 3e1b971.

Another remark: after checking with @JohnCoene in another PR, I might be wrong using list_blocks() which is not exported. I should use available_blocks() instead.

first pass into workspace UI/server

d60cd40

DivadNojnarg commented Dec 8, 2023

View reviewed changes

DivadNojnarg mentioned this pull request Dec 11, 2023

Cleanup module: block, stack ... #119

Open

DivadNojnarg added 13 commits December 11, 2023 09:34

code reordering

e4b016d

Merge branch 'main' into 121-workspace-server

0de7fe7

Merge branch 'main' into 121-workspace-server

fb2ac6e

fix issue with looping

fb325b2

Merge branch 'main' into 121-workspace-server

a9fafa2

Merge branch 'main' into 121-workspace-server

b48a0c6

generalise inject_remove_button

a7c865b

more generics to generalise ...

39ed800

cleanup

3a31a1e

fix remove issue

5318bca

R CMD check

c2e106a

start handle dynamic block insertion

f2d0acb

fix 2 issues: re-adding existing blocks + insertion at wrong position

c768ebb

finalize

cfdb15c

DivadNojnarg marked this pull request as ready for review January 10, 2024 22:52

DivadNojnarg requested a review from nbenn as a code owner January 10, 2024 22:52

DivadNojnarg mentioned this pull request Jan 11, 2024

Workspace class #92

Closed

nbenn approved these changes Jan 11, 2024

View reviewed changes

DivadNojnarg added 3 commits January 12, 2024 11:41

correct naming, using exported registry

02cfe4f

Merge branch 'main' into 121-workspace-server

bde2c59

rox

a1c4c93

DivadNojnarg merged commit ff1d095 into main Jan 12, 2024
7 of 8 checks passed

DivadNojnarg deleted the 121-workspace-server branch January 12, 2024 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass into workspace UI/server #124

First pass into workspace UI/server #124

DivadNojnarg commented Dec 8, 2023 •

edited

Loading

codecov bot commented Dec 8, 2023 •

edited

Loading

DivadNojnarg Dec 8, 2023

DivadNojnarg Dec 8, 2023

DivadNojnarg Dec 8, 2023

DivadNojnarg commented Dec 11, 2023

DivadNojnarg commented Jan 10, 2024 •

edited

Loading

DivadNojnarg commented Jan 10, 2024

nbenn left a comment

nbenn Jan 11, 2024

DivadNojnarg Jan 11, 2024

nbenn Jan 11, 2024

nbenn Jan 11, 2024

DivadNojnarg Jan 11, 2024

JohnCoene Jan 11, 2024

nbenn Jan 11, 2024

DivadNojnarg Jan 11, 2024

First pass into workspace UI/server #124

First pass into workspace UI/server #124

Conversation

DivadNojnarg commented Dec 8, 2023 • edited Loading

codecov bot commented Dec 8, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DivadNojnarg commented Dec 11, 2023

DivadNojnarg commented Jan 10, 2024 • edited Loading

DivadNojnarg commented Jan 10, 2024

nbenn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DivadNojnarg commented Dec 8, 2023 •

edited

Loading

codecov bot commented Dec 8, 2023 •

edited

Loading

DivadNojnarg commented Jan 10, 2024 •

edited

Loading