Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Deselect visualization when its node gets removed #1556

Merged
merged 8 commits into from
May 13, 2021

Conversation

s9ferech
Copy link
Contributor

@s9ferech s9ferech commented May 7, 2021

Pull Request Description

This fixes issue #1352. The graph editor will register visualizations as deselected when their node is removed.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The CHANGELOG.md was updated with the changes introduced in this PR.
  • The documentation has been updated if necessary.
  • All code conforms to the Rust style guide.
  • All code has automatic tests where possible.
  • All code has been profiled where possible.
  • All code has been manually tested in the IDE.
  • All code has been manually tested in the "debug/interface" scene.
  • All code has been manually tested by the PR owner against our test scenarios.
  • All code has been manually tested by at least one reviewer against our test scenarios.

@s9ferech s9ferech requested a review from wdanilo as a code owner May 7, 2021 12:22
@s9ferech s9ferech added Category: GUI The Graphical User Interface Priority: High Should be scheduled as soon as possible Type: Bug A bug in Enso IDE labels May 7, 2021
@s9ferech s9ferech self-assigned this May 7, 2021
@s9ferech s9ferech changed the title Deselect visualization Deselect visualization when its node gets removed May 7, 2021
To make sure that the node id is still valid
@@ -1403,6 +1403,7 @@ impl GraphEditorModel {
/// implementation.
fn remove_node(&self, node_id:impl Into<NodeId>) {
let node_id = node_id.into();
self.frp.source.on_visualization_select.emit(Switch::Off(node_id));
Copy link
Member

Choose a reason for hiding this comment

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

Emiting frp events from model functions is rather a bad pattern, as it is really hard to understand where the events come from. Instead, check where this model function is called in the FRP network and handle it directly there.

This should be handled next to the lines:

output.source.on_visualization_select <+ selected.constant(Switch::On(node_id));
output.source.on_visualization_select <+ deselected.constant(Switch::Off(node_id));

so it would be easy to track when visualization is deselectected when reading the code (all deselecting will be coded in one place then)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this, but it cannot be handled next to the lines that you suggested, because we need only one connection per graph editor, but those lines are run for every node.

CHANGELOG.md Outdated Show resolved Hide resolved
@s9ferech s9ferech requested a review from wdanilo May 10, 2021 22:12
@s9ferech s9ferech merged commit f9778d6 into develop May 13, 2021
@s9ferech s9ferech deleted the deselect-visualization branch May 13, 2021 07:50
mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Category: GUI The Graphical User Interface Priority: High Should be scheduled as soon as possible Type: Bug A bug in Enso IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants