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

Opening Component Browser by clicking on the output port #3346

Merged
merged 38 commits into from
Mar 21, 2022

Conversation

akavel
Copy link
Contributor

@akavel akavel commented Mar 17, 2022

Pull Request Description

Double-clicking a node's output port or clicking the port with a right mouse button (RMB) creates a new node aligned to the clicked node.

Visuals

The screencast below demonstrates the following features:

  • double-clicking the left mouse button on a node's output port;
  • clicking the right mouse button on a node's output port;
  • alignment of the nodes created as a result of the actions described above;
  • corner case: double-clicking (and RMB-clicking) on output ports of a "collapsed" ("enterable") node;
  • double-clicking on a "collapsed" ("enterable") node still allows entering the node when done over an area of the node that is not the node's output port;
  • basic support for nodes with multiple output ports (shown on the interface demo scene).
Screen.Recording.2022-03-17.at.17.02.58.mov

The supplementary screencast below demonstrates that double-clicking or RMB-clicking a node's output port cancels the action of dragging a new connection from a node.

Screen.Recording.2022-03-18.at.12.48.38.mov

https://www.pivotaltracker.com/story/show/181076145

Important Notes

  • The "double-clicking a node" shortcut was previously used to allow entering a "collapsed" node (for example, a node created by pressing the cmd+g keyboard shortcut after selecting a group of nodes). This PR keeps that functionality when the user double-clicks on a node, as long as the mouse is not positioned over the node's output ports.
  • The support for nodes with multiple output ports is currently very basic. The information about a port (Crumb) is passed into the create_node function, but it is not passed further to NodeSource. The Node Searcher currently does not support passing port information through NodeSource.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

@akavel akavel marked this pull request as ready for review March 18, 2022 11:55
Comment on lines -1383 to +1386
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to make this struct so heavyweight? Why do you need to keep EdgeEndpoint instead of NodeId, just like DroppingEdge does?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was by my request. If we want to support multiple output ports (and we want eventually), then the future implementor would already know where to start.

Copy link
Member

Choose a reason for hiding this comment

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

I am OK with adding the information. My question was why instead of adding simply NodeId we are adding the whole structure there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it makes sense in this structure. The node was created by clicking on the port - the fact that we don't make use of this information right now is an implementation detail. And I would not be worried with weight of crumbs: those Vecs are empty in most cases, and usually have no more than two elements otherwise.

@@ -2831,7 +2848,7 @@ fn new_graph_editor(app: &Application) -> GraphEditor {
input_press : &node_input_touch.down,
output : &out,
};
model.create_node(&ctx, *way, *mouse_pos)
model.create_node(&ctx, way.clone(), *mouse_pos)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps create_node could take way by a reference?

Comment on lines 2833 to 2838
input_start_node_creation_from_port <- inputs.hover_node_output.sample(&inputs.start_node_creation_from_port);
start_node_creation_from_port <- input_start_node_creation_from_port.filter_map(|v| v.clone());
start_creation_from_port_way <- start_node_creation_from_port.map(
|endpoint| WayOfCreatingNode::StartCreationFromPortEvent{endpoint: endpoint.clone()});
removed_edges_on_node_creation_from_port <= start_creation_from_port_way.map(f_!(model.model.clear_all_detached_edges()));
out.source.on_edge_drop <+ removed_edges_on_node_creation_from_port;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Some of those lines are over 100 characters.
  2. In this section, FRP nodes are for just creating the way structure. Please move nodes specific to creating node from clicking output to a separate block.

Comment on lines +492 to +493
/// Start creation of a new Node connected to the port that is currently under the cursor.
/// If the cursor is currently not over any node's port, this event will have no effect.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would repeat or refer to information in start_node_creation that this is meant to be user interaction.

@akavel akavel requested a review from wdanilo March 18, 2022 14:39
@akavel
Copy link
Contributor Author

akavel commented Mar 18, 2022

(Process note: the code-review is now Approved, therefore the PR is waiting for an Acceptance/QA Review on Task https://www.pivotaltracker.com/story/show/181076145 before merge.)

Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

Accepted

@akavel akavel added the CI: Ready to merge This PR is eligible for automatic merge label Mar 21, 2022
@mergify mergify bot merged commit b117a7d into develop Mar 21, 2022
@mergify mergify bot deleted the wip/akavel/cb-placement-by-dblclick-181076145 branch March 21, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants