Fix FRP events that deactivate visualizations. #6638
Conversation
MichaelMauderer
added
the
CI: No changelog needed
MichaelMauderer
requested review from
wdanilo,
farmaazon and
kazcw
as code owners
MichaelMauderer
changed the title
|
@wdanilo @kazcw @farmaazon please review the PR |
| vis_input_type <- frp.set_vis_input_type.on_change(); | ||
| vis_input_type <- vis_input_type.gate(&visualisation_uninitialised).unwrap(); |
There was a problem hiding this comment.
Why this on_change is needed? Wouldn't it be stopped on the on_change in visualization_path later anyway?
There was a problem hiding this comment.
The problem is, that it updates the visualization chooser, which in turn sends an update that nothing was selected, causing the visualization to be set to None.
There was a problem hiding this comment.
The problem is, that it updates the visualization chooser, which in turn sends an update that nothing was selected, causing the visualization to be set to
None.
Please add this useful info as a comment here.
| menu_appears <- menu.menu_visible.gate(&menu.menu_visible).constant(()); | ||
|
|
||
| // We want to update entries according to the input type, but only when it changed and | ||
| // menu is visible. | ||
| input_type_when_visible <- frp.set_vis_input_type.gate(&menu.menu_visible); | ||
| input_type_when_appeared <- frp.set_vis_input_type.sample(&menu_appears); | ||
| input_type <- any(input_type_when_visible,input_type_when_appeared); | ||
| input_type_changed <- input_type.on_change(); | ||
|
|
There was a problem hiding this comment.
Why this is deleted?
I'm afraid that on scenes with really many nodes we cannot afford to go through every visualization and update all chooser which are invisible anyway.
There was a problem hiding this comment.
Fair enough, but this code was essentially wrong. What happened here was, that the list of entries was only ever updated when the visualization chooser had an open selection menu. That means that the preview was never updated unless the menu was opened, and the internal state was somewhat inconsistent.
If we want the performance benefit of not updating the menu unless shown, we need to do it in the action bar, where we have the actual information about whether the visualization chooser is visible.
There was a problem hiding this comment.
Let's go with it for now, but I'm very confused here. I wouldn't expect that updating or not the list in the view would have an effect on how the visualization updates work. Certainly we need to do some refactoring here.
| vis_input_type <- frp.set_vis_input_type.on_change(); | ||
| vis_input_type <- vis_input_type.gate(&visualisation_uninitialised).unwrap(); |
There was a problem hiding this comment.
The problem is, that it updates the visualization chooser, which in turn sends an update that nothing was selected, causing the visualization to be set to
None.
Please add this useful info as a comment here.
| menu_appears <- menu.menu_visible.gate(&menu.menu_visible).constant(()); | ||
|
|
||
| // We want to update entries according to the input type, but only when it changed and | ||
| // menu is visible. | ||
| input_type_when_visible <- frp.set_vis_input_type.gate(&menu.menu_visible); | ||
| input_type_when_appeared <- frp.set_vis_input_type.sample(&menu_appears); | ||
| input_type <- any(input_type_when_visible,input_type_when_appeared); | ||
| input_type_changed <- input_type.on_change(); | ||
|
|
There was a problem hiding this comment.
Let's go with it for now, but I'm very confused here. I wouldn't expect that updating or not the list in the view would have an effect on how the visualization updates work. Certainly we need to do some refactoring here.
QA report: 🍏No issue spotted which is not on develop. |
MichaelMauderer
added
the
CI: Ready to merge
mergify
bot
deleted the
visualizations_updates_are_not_stable_and_leaving_visualizations_blank
branch
Re-introduce a feature that was removed with #6638: only initialize visualization choosers when they are visible. This avoids initializing lots of invisible UI elements at the same time when opening a project.
Pull Request Description
Addresses the issue described here: #6561 (comment) .
This was caused by FRP events that would re-set the visualization path when ending the node editing. This would eventually clear the visualization before setting it again, losing the already received data.
Peek.2023-05-10.15-57.mp4
also addresses the issue described here #6561 (comment)
Peek.2023-05-11.15-35.mp4
Note that now the default visualization is already shown on the first hover of the action bar, where before it was empty. This was caused by a faulty initialization.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.