Implement creating nodes by dropping a connection #3231

vitvakatu · 2022-01-20T14:17:39Z

Pull Request Description

Creating nodes by pressing Tab works as before, without changes.

Creating nodes by dragging edges and dropping them on the scene:

Creating.nodes.by.dragging.edges.mp4

It doesn't matter if you drag the connection by pressing LMB then releasing it or if you continuously hold LMB while dragging.
Searcher filters suggestions based on the type of node from which the connection was dragged.

Disconnecting existing connections works depending on which endpoint you drag (which half of the connection you start to drag): dropping the target endpoint creates a new node, dropping the source endpoint does not.

Screen.Recording.2022-01-24.at.12.39.23.mov

This PR does not cover nodes alignment after creation - they are created always directly below the cursor (you can't really mess up with other nodes though, as dropping connection on top of other node won't work or will just connect these nodes).

Changes in this PR:

controller::searcher::Mode::NewNode node holds an optional node id of the "source node". The source node is either the node selected while creating a new node or the node from which the connection was dragged out.
controller::searcher::ThisNode constructor changed so now it accepts a single node id argument instead of a list of nodes. Previously it accepted a list of selected nodes, but it doesn't make sense now, as we should make a decision on what node to use as a "source node" earlier.
WayOfOpeningSearcher enum added.
searcher_input output of the Project View is removed, its usages replaced with searcher_opened output (it now contains WayOfOpeningSearcher rather than NodeId.
GraphEditor FRP refactored so clicking on the background to abort node editing doesn't conflict with dropping connection.
Pressing the Enter key while dragging the connection now causes a connection drop. It is useful to drop connections without opening the searcher.

Important Notes

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 documentation and configuration conforms to the markdown and YAML style guides.
All code has been tested where possible.

vitvakatu

Some implementation-related explanations.

vitvakatu · 2022-01-21T12:13:12Z

app/gui/view/src/project.rs

+
+/// The information needed to setup Searcher Controller.
+#[derive(Clone, CloneRef, Copy, Debug, PartialEq)]
+pub enum SearcherInput {


I can't think of a better name and/or location for this enum. It is used both for created nodes and for edited nodes (all cases when the searcher is opened), and it can't be placed in controllers, as it must be used in the View's FRP.

I think the placement is ok, if we change the meaning of this structure a little: not "The information needed to set up the Searcher Controller." but rather "An enum describing how the searcher was opened".

And adjust the name to that: this is not actually the searcher input, because the edge is not a part of edge input (and is a part of one variant of this enum). WayOfOpeningSearcher is better, or something more compact if you find.

vitvakatu · 2022-01-21T12:14:03Z

app/gui/view/graph-editor/src/lib.rs

@@ -2426,14 +2426,49 @@ fn new_graph_editor(app: &Application) -> GraphEditor {
    }


+    // === Add Node ===
+
+    frp::extend! { network


This FRP piece is just moved to make has_detached_edge being available for Node Editing section.

Thank you for this comment, very helpful during the PR review ❤️

vitvakatu · 2022-01-21T12:16:20Z

app/gui/src/controller/searcher.rs

-        graph: &controller::Graph,
-    ) -> Option<Self> {
-        let id = *nodes.first()?;
+    pub fn new(id: double_representation::node::Id, graph: &controller::Graph) -> Option<Self> {


Passing selected_nodes everywhere just to take the first of it doesn't make sense to me. So now it receives just one node, either selected one or the source node of the connection (depending on Mode)

I think there was an idea long ago, that the new node in searcher shall take not only "this"/"self" argument of selected node, but also arguments from other selected nodes. But for now, we can leave your fix, according to the YAGNI rule.

vitvakatu · 2022-01-21T12:17:14Z

app/gui/view/src/project.rs

        adding_new_node                     (bool),
-        searcher_input                      (Option<NodeId>),


searcher_opened wasn't used before at all, so I decided to replace usages of searcher_input with it.

wdanilo

Few questions / requests:

Please show all corner cases - press-drag-drop is one way of creating the connection. What with click (press and release) , move mouse, click again? What with connections from input ports? What when you are disconnecting existing connections and dropping them? Please show ALL possible corner cases on the video.
The design doc describes the desired behavior of this feature, including alignment of the newly created node. Is the Magnet Alignment algorithm used here as described in the doc? If no, is it planned as next PR? I did not find any information about it in the PR description. Please remember, that you should always carefully read the design doc (or at least a part of it) when implementing a feature, and there the alignment algorithm is described.

vitvakatu · 2022-01-21T18:15:41Z

@wdanilo

Please show all corner cases - press-drag-drop is one way of creating the connection. What with click (press and release) , move mouse, click again? What with connections from input ports? What when you are disconnecting existing connections and dropping them? Please show ALL possible corner cases on the video.

Sure, I will record additional footage to cover that. How do you distinguish between click-move-click and press-drag-drop on a video? I actually used click-move-click in the posted recording. Also, what is connection from input port? I don't know how to create a connection from the input port. (perhaps I'm missing something)

The design doc describes the desired behavior of this feature, including alignment of the newly created node. Is the Magnet Alignment algorithm used here as described in the doc? If no, is it planned as next PR? I did not find any information about it in the PR description. Please remember, that you should always carefully read the design doc (or at least a part of it) when implementing a feature, and there the alignment algorithm is described.

This is a PR for the task #180873545, which includes only creating new nodes. The alignment will be covered in the task #180887079, though it might be modified after the Design Doc will be finally ready. Should I describe future plans for things that are not included in the PR?

wdanilo · 2022-01-21T19:13:06Z

@vitvakatu

Sure, I will record additional footage to cover that.

Please do! It's important to cover in the video all possible corner cases (as described in our process doc) 🙂

How do you distinguish between click-move-click and press-drag-drop on a video?

I was sure that the mouse cursor gets bigger when you stop pressing it, but I was wrong - it is small even if you don't press it (and that's correct behavior!).

I actually used click-move-click in the posted recording.

Wow, it was so smooth and fast that I was sure it was press-drag-release! TBH I. was never able to use this method so smoothly! 😄

Also, what is connection from input port?

You can disconnect node in such a way that the connection is still connected to input port, but not to the output port.

I don't know how to create a connection from the input port. (perhaps I'm missing something)

Regarding corner cases, double check with the task designer if you covered all the corner cases before requesting the review, please. This will drastically shorten the review process – otherwise, the PR needs to be rejected before the code is even checked, the explanation needs to be described, you have to fix it, then it needs to be re-reviewed – this just should not be needed. There are sections about it in the design doc (I know it was being slightly modified for the last few days, but these things were already described there), like this one:

This is a PR for the task #180873545, which includes only creating new nodes. The alignment will be covered in the task #180887079,

Amazing, thanks!

though it might be modified after the Design Doc will be finally ready.

No need for that. And I dont think anything will change. The design doc is pretty stable since Monday – we had a few calls about look and feel of some advanced cases, but no deep changes were done there during the last 2 days. I will update some old screenshots and we are ready to merge it already. So you should be able to get all the needed info from the current stage.

Should I describe future plans for things that are not included in the PR?

If you are implementing a feature that is tightly connected to another one, it is cool if you mention that "this doesn't cover the node alignment, which will be implemented in another PR" – this makes everyone sure that this is by design and not accidental. Of course you don't have to – in such a case, me or someone else would just sometimes ask additional questions about it :)

vitvakatu · 2022-01-24T09:54:00Z

@wdanilo

You can disconnect node in such a way that the connection is still connected to input port, but not to the output port.

Cool, I was a bit confused that you separated nodes' disconnection and dropping connection from input ports in your first reply, so I was afraid that I missed this opportunity. It works as the design doc states: dropping the source endpoint does nothing, dropping the target one creates a new node. I attached an additional screen recording and expanded PR description, hope it's better now.

If you are implementing a feature that is tightly connected to another one, it is cool if you mention that "this doesn't cover the node alignment, which will be implemented in another PR" – this makes everyone sure that this is by design and not accidental. Of course, you don't have to – in such a case, I or someone else would just sometimes ask additional questions about it :)

Yeah, it was an oversight from my side, I was expecting that Task Reviewer will verify if the task description matches implemented behavior and he will be in a full context of the task. I should've simplified his work anyway.

farmaazon

Minor comments. I'll still need to go through the test scenario.

farmaazon · 2022-01-24T14:02:21Z

app/gui/src/presenter/project.rs

@@ -9,6 +9,7 @@ use crate::executor::global::spawn_stream_handler;
 use crate::presenter::graph::ViewNodeId;
 use enso_frp as frp;
 use ide_view as view;
+use view::project::WayOfOpeningSearcher;


Please fix import ordering.

Ordering is actually matching our style guide, I added a few new lines here and there

farmaazon · 2022-01-24T14:10:41Z

app/gui/view/graph-editor/src/lib.rs

    // === Node Editing ===

    frp::extend! { network
+        // Clicking on background either drops dragged edge or aborts node editing.
+        let background_selected = touch.background.selected.clone_ref();
+        edge_being_dragged  <- has_detached_edge.sample(&background_selected);


I think this name does not quite tell what the FRP node is. It suggests something like "a bool property telling if I'm during edge dragging". Instead, it should be something like "was_edge_dragged_when_background_selected". Or "was_edge_dragged" is sufficient, as we use this name only locally.

Renamed. As it is used only locally, it's okay to use quite a long name I think, so I stick to it.

farmaazon

Regressions found during tests:

I cannot select an entry with tab key (it should put the suggestion in the searcher input and continue editing). Instead, the new searcher is opened. I think that because the output used to block shortcut was renamed or removed, I missed it during my first review.
For similar reason, quitting searcher with Escape does not work as well.

also fix review suggestions

wdanilo · 2022-01-24T16:07:55Z

The new video looks amazing! I did not check the code yet, however, let's first fix the regressions found by Adam + could you also add a functionality that pressing enter while dragging the connection removes it? I just realised that right now it is impossible to drop a connection when dragging it without opening the searcher, which is bad.

vitvakatu · 2022-01-24T17:19:32Z

The new video looks amazing! I did not check the code yet, however, let's first fix the regressions found by Adam + could you also add a functionality that pressing enter while dragging the connection removes it? I just realised that right now it is impossible to drop a connection when dragging it without opening the searcher, which is bad.

@wdanilo I fixed the regression and implemented this. I'd appreciate FRP code review if you have a free time.

farmaazon · 2022-01-25T09:31:57Z

app/gui/CHANGELOG.md

@@ -2,6 +2,10 @@

 #### Visual Environment

+- [New nodes can be created by dragging and dropping a connection on the
+  scene.][3231]
+- [Node connections can be dropped by pressing the Enter key while dragging


For me, it would be much more natural to press Esc instead.

Also, update the shortcuts list in the docs

Of course this should be escape, I've written enter completely by accident. Thanks for catching it!

Fixed it :)

wdanilo · 2022-01-25T10:45:51Z

app/gui/src/presenter/searcher.rs

@@ -201,4 +202,23 @@ impl Searcher {
        let entry = controller.actions().list().and_then(|l| l.get_cloned(entry));
        entry.map_or(false, |e| matches!(e.action, Example(_)))
    }
+
+    /// Select source node for node creation. It would be either:
+    /// 1. The source node of the dragged connection, dropping which leads to the node creation.


I don't think this sentence is logically correct or just iff it is correct English sentence. What does mean "dropping which leads to the node creation"? Could you rephrase it, please?

wdanilo · 2022-01-25T10:47:40Z

app/gui/src/presenter/searcher.rs

+            let id = edge_source.map(|source| source.node_id);
+            id.map(|id| graph_presenter.ast_node_of_view(id)).flatten()


Make var names a little bit better. I understand what edge is, I understand edge_source, but then we have id - this is ID of what? the var name should explain that better imo.

wdanilo · 2022-01-25T10:48:46Z

app/gui/src/presenter/searcher.rs

+    fn source_node(
+        view: &view::project::View,
+        graph_presenter: &presenter::Graph,
+        way_of_opening_searcher: &WayOfOpeningSearcher,
+    ) -> Option<Uuid> {


This function name does not reflect what it does. The name source_node suggests that it returns a source node, but it does not. It also does not return the node's id. It returns the AST node id, is that correct? This should be well explained in the doc.

wdanilo · 2022-01-25T10:51:07Z

app/gui/view/graph-editor/src/lib.rs

@@ -2426,14 +2426,49 @@ fn new_graph_editor(app: &Application) -> GraphEditor {
    }


+    // === Add Node ===
+
+    frp::extend! { network


Thank you for this comment, very helpful during the PR review ❤️

wdanilo · 2022-01-25T10:51:57Z

app/gui/view/graph-editor/src/lib.rs

    // === Node Editing ===

    frp::extend! { network
+        // Clicking on background either drops dragged edge or aborts node editing.
+        let background_selected = touch.background.selected.clone_ref();


why do you need clone of it if you are only using it as reference and then dropping it?

wdanilo · 2022-01-25T11:00:48Z

app/gui/view/graph-editor/src/lib.rs

+    drop_edges     <- any (drop_on_bg_up,click_to_drop_edge);
+
+    edge_dropped_to_create_node <= drop_edges.map(f_!(model.edges_with_detached_targets()));
+    out.source.on_edge_drop_to_create_node <+ edge_dropped_to_create_node;


Do you need to connect it here to out....? We try to connect to out minimal amounts of things, and there was already a logic that was opening the searcher – are you not able to plug into this existing logic here? Maybe not, but it's a little bit hard for me to understand what this code does because its hard to understand the var names now.

The searcher is opened when the node enters being_edited state - but the Project View is creating new nodes and is starting editing them, not the Graph View. So there is no way to open searcher from the Graph View at the moment, I need an additional output to signal Project View.
Perhaps there is a way to refactor the code in such a way that Graph View would be responsible for node creation, but it feels like quite a big change.

wdanilo · 2022-01-25T11:03:05Z

app/gui/view/graph-editor/src/lib.rs

+    edge_dropped_to_create_node <= drop_edges.map(f_!(model.edges_with_detached_targets()));
+    out.source.on_edge_drop_to_create_node <+ edge_dropped_to_create_node;
+
+    remove_all_detached_edges <- any (drop_edges, inputs.drop_dragged_edge);


Is this an event name? I mean, is this a name that means "this happens on this event"? If so, please rename it to a name starting with "on_" - this is a new naming convention that we are using in FRP, but it was not used yet in all places in this FRP

What identifier are you referring to?

wdanilo · 2022-01-25T11:04:01Z

app/gui/view/src/project.rs

+#[derive(Clone, CloneRef, Copy, Debug, PartialEq)]
+pub enum WayOfOpeningSearcher {
+    /// New node was created by opening the searcher or the node is being edited.
+    NodeEdited(NodeId),


Node Edited sounds like the node was edited. I would never understand that as node being edited. So this name is misleading,

Would NodeEditing be better?

wdanilo · 2022-01-25T11:05:28Z

app/gui/view/src/project.rs

+
+/// An enum describing how the searcher was opened.
+#[derive(Clone, CloneRef, Copy, Debug, PartialEq)]
+pub enum WayOfOpeningSearcher {


I'd rename it to ComponentBrowserOpenReason. The "way" word is broader than "reason" IMO

Btw, we should lowly rename "searcher" to component browser in the future. I think that new apis can already use the new name.

I think Cause is even better.

wdanilo · 2022-01-25T11:06:27Z

app/gui/view/src/project.rs

+}
+
+impl WayOfOpeningSearcher {
+    /// NodeId of the created/edited node.


If you are referring to a name from documentation, please use Rust syntax for referencing existing names (I dont remember it right now, but its something with [ and ])

feat: implement creating nodes by dropping a connection

7808f9b

vitvakatu self-assigned this Jan 20, 2022

vitvakatu added 7 commits January 20, 2022 17:33

update searcher input on node editing event

0a125f3

get rid of searcher_input output endpoint

914e392

rework project FRP

14a8f0e

massive refactoring

3c0f863

documentation and codestyle fixes

5bf4faa

Merge branch 'develop' into wip/vitvakatu/dropping-edges-180873545

6c562c5

remove debug message

f2e2f2a

vitvakatu commented Jan 21, 2022

View reviewed changes

vitvakatu added 4 commits January 21, 2022 17:21

fix docs of ThisNode

0a0f6c2

SearcherInput -> WayOfOpeningSearcher

c2e36df

update changelog

f3bf3be

apply prettier for changelog

8f8d829

vitvakatu marked this pull request as ready for review January 21, 2022 15:26

vitvakatu requested review from MichaelMauderer, mwu-tow and wdanilo as code owners January 21, 2022 15:26

vitvakatu requested a review from akavel January 21, 2022 15:26

wdanilo requested changes Jan 21, 2022

View reviewed changes

vitvakatu requested review from wdanilo and farmaazon January 24, 2022 09:54

farmaazon reviewed Jan 24, 2022

View reviewed changes

farmaazon requested changes Jan 24, 2022

View reviewed changes

fix is_searcher_opened endpoint bug

7402ecc

also fix review suggestions

implement dropping edge on pressing Enter

e770759

update changelog

9c3b657

vitvakatu requested a review from farmaazon January 24, 2022 17:17

farmaazon approved these changes Jan 25, 2022

View reviewed changes

vitvakatu added 2 commits January 25, 2022 13:40

drop connections on esc, not enter

55717a4

Merge branch 'develop' into wip/vitvakatu/dropping-edges-180873545

b594b2c

wdanilo requested changes Jan 25, 2022

View reviewed changes

wdanilo approved these changes Jan 25, 2022

View reviewed changes

vitvakatu added 2 commits January 25, 2022 15:31

Refactor names & fix docs

c56453a

Merge branch 'develop' into wip/vitvakatu/dropping-edges-180873545

8f6478e

4e6 merged commit 3905698 into develop Jan 26, 2022

4e6 deleted the wip/vitvakatu/dropping-edges-180873545 branch January 26, 2022 11:23

		let id = edge_source.map(\|source\| source.node_id);
		id.map(\|id\| graph_presenter.ast_node_of_view(id)).flatten()

Implement creating nodes by dropping a connection #3231

Implement creating nodes by dropping a connection #3231

Conversation

vitvakatu commented Jan 20, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

vitvakatu 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

wdanilo left a comment

Choose a reason for hiding this comment

vitvakatu commented Jan 21, 2022

wdanilo commented Jan 21, 2022

vitvakatu commented Jan 24, 2022

farmaazon 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

farmaazon left a comment

Choose a reason for hiding this comment

wdanilo commented Jan 24, 2022

vitvakatu commented Jan 24, 2022

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

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

vitvakatu commented Jan 20, 2022 •

edited

Loading