Add node button to enable/disable output context #6205
Conversation
Procrat
added
the
CI: No changelog needed
Procrat
requested review from
MichaelMauderer,
wdanilo,
farmaazon,
kazcw and
mwu-tow
as code owners
app/gui/src/presenter/graph.rs
Outdated
| } else { | ||
| None | ||
| }; | ||
| self.log_action( | ||
| || { | ||
| let ast_id = | ||
| self.state.update_from_view().set_node_context_switch(id, expr.clone())?; | ||
| let ast_id = self.state.ast_node_id_of_view(id)?; |
There was a problem hiding this comment.
This is the part that I changed to get the expression updates to reach the view. Based on my understanding, what used to happen is this:
- In this function (which triggers when the button is clicked), both the presenter state and the controller get updated.
- When we get an update back from the controller, we trigger this FRP link:
expression_update <= update_data.map(|update| update.set_node_expressions());. - In
set_node_expressions, the nodes are checked for a mismatch between the controller update and the presenter state. However, since the state was already updated, no mismatch was found and therefore no update is triggered.
By removing the explicit view update here, we create that mismatch and the update fires correctly. I'm not sure if this was the correct approach. In any case, I think the skip and freeze changes have the same problem. Should I do the same thing there?
There was a problem hiding this comment.
I'm not sure. How to reproduce this issue in a previous version of the code? I don't understand why we need to update the view if it was already updated.
app/gui/src/presenter/graph.rs
Outdated
| } else { | ||
| None | ||
| }; | ||
| self.log_action( | ||
| || { | ||
| let ast_id = | ||
| self.state.update_from_view().set_node_context_switch(id, expr.clone())?; | ||
| let ast_id = self.state.ast_node_id_of_view(id)?; |
There was a problem hiding this comment.
I'm not sure. How to reproduce this issue in a previous version of the code? I don't understand why we need to update the view if it was already updated.
There was a problem hiding this comment.
Summary after internal call:
After reviewing the code, I found an issue with its functionality. When I tested the feature by opening a new project, clicking the button, and switching the global environment with a shortcut, I observed that the added expression disappeared from the node. This behavior is incorrect and needs to be addressed.
Based on my analysis, I hypothesize that the highlight may be coupled to the state of the ToggleButton, causing an additional "click" event that removes the expression. I suggest that we decouple the highlight from the state of the ToggleButton or find an alternative solution.
Regarding the updates, I believe that node_action_context_switch is correct and does not require any changes. However, I suggest modifying the FRP logic. Currently, output_context_updated only stores the state of the last updated node, resulting in only a single node being updated when the global context changes. To address this issue, I suggest we loop over every node, similar to set_node_expressions.
…ode which had it's context switch expression updated last.
|
@vitvakatu If you're having another look, it might be worth going through commit by commit to understand which parts belong together and why that change was made. |
Procrat
force-pushed
the
wip/procrat/toggle-reevaluation-button-5929
branch
from
92e0c57 to
449dcb9
Compare
|
I think I addressed everything except the tooltip tabel question. I also found another edge case that should be solved now. My apologies if the force push is inconvenient. I had to roll back the merge with the latest version of |
| @@ -31,9 +33,11 @@ const BUTTON_OFFSET: f32 = 0.5; | |||
| /// Grow the hover area in x direction by this amount. Used to close the gap between action | |||
| /// icons and node. | |||
| const HOVER_EXTENSION_X: f32 = 15.0; | |||
| const VISIBILITY_TOOLTIP_LABEL: &str = "Show preview"; | |||
There was a problem hiding this comment.
It seems the old labels were not sentences. But if these are, should they be followed by a period?
There was a problem hiding this comment.
Hmm, I don't think single-sentence UI components generally use periods?
Pull Request Description
Closes #5929: Adding a node button to enable and disable the output context for that particular node.
I also added a temporary shortcut (cmd-shift-c) to switch the execution environment so we can properly test it.
2023-04-05.11-02-59.online-video-cutter.com.mp4
Important Notes
While we're waiting to integrate it with the language server, the execution environment is temporarily stored in the presenter. (Otherwise we'd have to define it in multiple places and the behaviour would look rather weird.)
I also fixed a bug where the view didn't get any updates when the context switch expression changed. I'll make a comment on the relevant part. I think the SKIP and FREEZE macros might have the same issue.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
./run ide build.