Creating a new node with the (+) button #3278

farmaazon · 2022-02-15T15:54:13Z

Pull Request Description

A new (+) button on the left-bottom corner appeared. It may be clicked to open searcher in the middle of the scene, as an alternative to tab key.

add-button-.5.mp4

Important Notes

The window_control_buttons::common was extracted to separate crate ensogl-component-button almost without change.
This includes a severe refactoring of adding nodes in general in the Graph Editor. The whole responsibility of adding new nodes (and starting their editing) was moved to Graph Editor - the Project View only reacts for GE events to show searcher properly.
The status bar was moved from the bottom-left corner to the middle-top of the scene. It does not collide with (+) button, and plays "notification" role anyway.
The interface debug scene was buggy. The problem was with one expression's span-tree. When I replaced it, the scene works.
I've removed "new searcher" API, as it is completely outdated.
I've changed code owners of integration tests to GUI team, as it is the team writing mostly the integration tests (int rust)

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.

…node-button-180887253

wdanilo

There is a LOT of FRP refactoring here. I'm OK with it, but I have a serious request to the tester – please test all corner cases very carefully. Go trough testing scenario – talk with Sylwia about where to find the testing scenario. You can also do the testing together with Sylwia, so she will show you how she does it (she is the most precise tester of Enso ever). Such a big refactoring of FRP requires serious testing.

app/gui/view/graph-editor/src/component/add_node_button.rs

wdanilo · 2022-02-17T15:27:59Z

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

+
+// ====================
+// === OccupiedArea ===
+// ===================


missing =

wdanilo · 2022-02-17T15:28:41Z

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

+
+/// The structure describing an occupied area.
+///
+/// All such areas are rectangles described by x and y ranges.


x1 is start of the range and x2 is end of it? If so, add short info about it pls.

EDIT: after seeing implementation, it seems that x1 and x2 are not sorted. This needs short doc :)

wdanilo · 2022-02-17T15:31:57Z

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

+    }
+
+    /// Return the x position of the left or right boundary, depending on whether the direction
+    /// points leftwards of rightwards respectively. Returns `None` if direction does not point


In Rust the doc syntax for mentioning things is [`None`], not `None` (unfortunately). Please apply in other comments too.

wdanilo · 2022-02-17T15:44:03Z

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

+
+impl GraphEditorModelWithNetwork {
+    pub fn new(app: &Application, cursor: cursor::Cursor, frp: &Frp) -> Self {
+        let network = frp.network.clone_ref(); // FIXME make weak


This is part of moved code, not written by me.

It seems to be non-trivial to change, so I would leave the "fixme".

wdanilo · 2022-02-17T15:50:13Z

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

+        let y_gap = self.frp.default_y_gap_between_nodes.value();
+        let y_offset = y_gap + node::HEIGHT;
+        let starting_point = above_pos - Vector2(0.0, y_offset);
+        let direction = Vector2(1.0, 0.0);


I think it should mean "left", but (1,0) means right – are we not searching place on the left side of currently occupied place?

I just wanted to keep it as it was. But of course I can change to left.

wdanilo · 2022-02-17T15:50:40Z

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

+        // - area taken by node view (obviously)
+        // - the minimum gap between nodes in all directions, so the new node won't be "glued" to
+        //   another


use either sentences or semicolons at the end of points, please.

wdanilo · 2022-02-17T15:51:20Z

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

@@ -2445,7 +2547,7 @@ fn new_graph_editor(app: &Application) -> GraphEditor {
    }


-    // === Add Node ===
+    // === Nodes Utilities ===


"Utilities" is the worst word IMO, as everything can be an "utility". This section will probably contain everything in the future. Do you see any better name maybe?

The problem is, that I don't really get what is going on in the block below. Seems like preparing many FRP nodes, without clear theme. But surely it wasn't adding nodes.

Ok, I read it more carefully, and it looks like things related to mouse cursor and clicks. So I rename it to "Mouse Interactions"

lib/rust/ensogl/component/button/src/lib.rs

akavel · 2022-02-18T15:33:31Z

When working with the branch, I noticed a gray vertical bar near the top of the screen, where the "status bar" later shows up - should it not be there maybe? It looks a bit like it was a shadow of an empty status bar. It's visible in the screencast attached to the PR description - look in the area highlighted with a yellow circle in the image below:

vitvakatu

A few remarks

vitvakatu · 2022-02-21T09:17:51Z

app/gui/view/debug_scene/interface/src/lib.rs

@@ -338,6 +338,7 @@ pub fn expression_mock() -> Expression {
    Expression { pattern, code, whole_expression_id, input_span_tree, output_span_tree }
 }

+// TODO[ao] This expression mocks results in panic. If you want to use it, please fix it first.


I don't quite understand this sentence, and also I'm not sure if we should leave in code TODOs not attached to a created issue in pivotal.

vitvakatu · 2022-02-21T09:46:11Z

app/gui/view/src/project.rs

+
+            committed_in_searcher <-
+                searcher.editing_committed.map2(&last_searcher, |&entry, &s| (s.input, entry));
+            trace committed_in_searcher;


Probably should be removed (one more slightly below)

vitvakatu · 2022-02-21T09:50:27Z

app/gui/view/src/project.rs

+            // frp.source.old_expression_of_edited_node <+ existing_node_edited.map(f!((node_id)
+            //     model.graph_editor.model.nodes
+            // ));


Should be removed? The output seems to be unused in the code.

vitvakatu · 2022-02-21T11:03:59Z

Manual testing showed two regressions:

Shadow of the empty status bar is visible, as @akavel pointed out.
The cursor changes its shape to a narrow vertical bar, but doesn't change it back when the searcher is closed. To reproduce (it is not reproducible on develop):

Without any node selected, press Tab key to create a new one
Press Esc immediately to abort node creation
The cursor will not return its round shape.

See attached video for the second issue:

2022-02-21.14-00-37.mp4

vitvakatu · 2022-02-21T14:10:20Z

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

+    pub fn start_editing_new_node(&self, node_id: NodeId) {
+        self.frp.set_node_expression.emit(&(node_id, node::Expression::default()));
+        self.frp.edit_node.emit(&node_id);
+    }


This function seems to be unused. Is it deliberate?

No, it is not. Thanks for catching!

wdanilo · 2022-02-23T02:19:33Z

@akavel @vitvakatu absolutely stunning work with the testing!!! Keep it this way ❤️

vitvakatu

Regressions were fixed 👍🏻

[The Task](https://www.pivotaltracker.com/story/show/180887253) A new (+) button on the left-bottom corner appeared. It may be clicked to open searcher in the middle of the scene, as an alternative to tab key. https://user-images.githubusercontent.com/3919101/154514279-7972ed6a-0203-47cb-9a09-82dba948cf2f.mp4 * The window_control_buttons::common was extracted to separate crate `ensogl-component-button` almost without change. * This includes a severe refactoring of adding nodes in general in the Graph Editor. The whole responsibility of adding new nodes (and starting their editing) was moved to Graph Editor - the Project View only reacts for GE events to show searcher properly. * The status bar was moved from the bottom-left corner to the middle-top of the scene. It does not collide with (+) button, and plays "notification" role anyway. * The `interface` debug scene was buggy. The problem was with one expression's span-tree. When I replaced it, the scene works. * I've removed "new searcher" API, as it is completely outdated. * I've changed code owners of integration tests to GUI team, as it is the team writing mostly the integration tests (int rust)

* Creating a new node with the (+) button (#3278) [The Task](https://www.pivotaltracker.com/story/show/180887253) A new (+) button on the left-bottom corner appeared. It may be clicked to open searcher in the middle of the scene, as an alternative to tab key. https://user-images.githubusercontent.com/3919101/154514279-7972ed6a-0203-47cb-9a09-82dba948cf2f.mp4 * The window_control_buttons::common was extracted to separate crate `ensogl-component-button` almost without change. * This includes a severe refactoring of adding nodes in general in the Graph Editor. The whole responsibility of adding new nodes (and starting their editing) was moved to Graph Editor - the Project View only reacts for GE events to show searcher properly. * The status bar was moved from the bottom-left corner to the middle-top of the scene. It does not collide with (+) button, and plays "notification" role anyway. * The `interface` debug scene was buggy. The problem was with one expression's span-tree. When I replaced it, the scene works. * I've removed "new searcher" API, as it is completely outdated. * I've changed code owners of integration tests to GUI team, as it is the team writing mostly the integration tests (int rust) * Fix regression #181528359 * Add docs & remove unused function * Fix & enable native Rust tests * Fix formatting Co-authored-by: Adam Obuchowicz <adam.obuchowicz@enso.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

[The Task](https://www.pivotaltracker.com/story/show/180887253) A new (+) button on the left-bottom corner appeared. It may be clicked to open searcher in the middle of the scene, as an alternative to tab key. https://user-images.githubusercontent.com/3919101/154514279-7972ed6a-0203-47cb-9a09-82dba948cf2f.mp4 * The window_control_buttons::common was extracted to separate crate `ensogl-component-button` almost without change. * This includes a severe refactoring of adding nodes in general in the Graph Editor. The whole responsibility of adding new nodes (and starting their editing) was moved to Graph Editor - the Project View only reacts for GE events to show searcher properly. * The status bar was moved from the bottom-left corner to the middle-top of the scene. It does not collide with (+) button, and plays "notification" role anyway. * The `interface` debug scene was buggy. The problem was with one expression's span-tree. When I replaced it, the scene works. * I've removed "new searcher" API, as it is completely outdated. * I've changed code owners of integration tests to GUI team, as it is the team writing mostly the integration tests (int rust)

In this branch: * The workaround for cursor-not-being-updated-after-closing-searcher bug (discovered while testing #3278) is reverted. * The proper fix was introduced: created an abstraction for EnsoGL component, which, when dropping, will not immediately drop the FRP network and model, but instead put it into the Garbage Collector. The Collector ensures, that all "component hiding" effects and events will be handled, and drops FRP network and model only after that. * I run clippy for wasm32 target out of curiosity. There was one warning, and I fixed it on this branch.

vitvakatu and others added 8 commits February 9, 2022 13:19

node creation refactoring

15b5a51

opening searcher (doesn't really work)

1b85682

Seems like refactored

a7e132b

WIP

4f94e96

WIP

514e4b1

WIP

13f3aa4

Button is displayed, but with the wrong shadow

e08436b

The Shadow displayed correctly, and the button actually works

1159a0f

farmaazon self-assigned this Feb 15, 2022

farmaazon added 9 commits February 16, 2022 16:32

Fixing node placement + bug

b2f7949

Code cleanup part 1

63ef571

Re-use free place finder in find_new_node_position

6ca7c3f

Move status bar to the top

fd70dc6

Merge remote-tracking branch 'origin/develop' into wip/farmaazon/add-…

8bfe041

…node-button-180887253

Fix linter issues

7d1f6cc

Self-review

7ba9752

Merge remote-tracking branch 'origin/develop' into wip/farmaazon/add-…

bb6acce

…node-button-180887253

Restore old limit

3c24031

farmaazon marked this pull request as ready for review February 17, 2022 15:08

farmaazon requested review from MichaelMauderer, wdanilo, mwu-tow, 4e6, PabloBuchu and jdunkerley as code owners February 17, 2022 15:08

Change owners of integration tests

c1ed6c2

wdanilo approved these changes Feb 17, 2022

View reviewed changes

farmaazon added 2 commits February 17, 2022 17:08

Merge branch 'develop' into wip/farmaazon/add-node-button-180887253

0d9e7f3

Applying @wdanilo review

84113f1

MichaelMauderer approved these changes Feb 18, 2022

View reviewed changes

Merge branch 'develop' into wip/farmaazon/add-node-button-180887253

133d36b

farmaazon assigned vitvakatu Feb 18, 2022

Plus button add nodes on the center of screen

0908481

vitvakatu reviewed Feb 21, 2022

View reviewed changes

Applying @vitvakatu comments and bugfixes

4d548a0

vitvakatu reviewed Feb 21, 2022

View reviewed changes

vitvakatu and others added 4 commits February 23, 2022 19:00

Merge branch 'develop' into wip/farmaazon/add-node-button-180887253

f4700ca

Add integration test for add node button

daa6530

Fix comment

d6dc807

Fix cursor style after removing node

e30dd3e

vitvakatu approved these changes Feb 24, 2022

View reviewed changes

farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Feb 24, 2022

farmaazon and others added 2 commits February 24, 2022 13:21

Merge branch 'develop' into wip/farmaazon/add-node-button-180887253

96d063f

Merge branch 'develop' into wip/farmaazon/add-node-button-180887253

9609991

mergify bot merged commit 0836ce7 into develop Feb 24, 2022

mergify bot deleted the wip/farmaazon/add-node-button-180887253 branch February 24, 2022 16:01

farmaazon mentioned this pull request Mar 7, 2022

The EnsoGL Component abstraction with special dropping behavior #3322

Merged

4 tasks

vitvakatu mentioned this pull request Mar 14, 2022

Return creating node with (+) button & fix a regression #3338

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating a new node with the (+) button #3278

Creating a new node with the (+) button #3278

farmaazon commented Feb 15, 2022 •

edited

Loading

wdanilo left a comment

wdanilo Feb 17, 2022

wdanilo Feb 17, 2022

wdanilo Feb 17, 2022

wdanilo Feb 17, 2022

farmaazon Feb 17, 2022

farmaazon Feb 17, 2022

wdanilo Feb 17, 2022

farmaazon Feb 17, 2022

wdanilo Feb 17, 2022

wdanilo Feb 17, 2022

farmaazon Feb 17, 2022

farmaazon Feb 17, 2022

akavel commented Feb 18, 2022 •

edited

Loading

vitvakatu left a comment

vitvakatu Feb 21, 2022

vitvakatu Feb 21, 2022

vitvakatu Feb 21, 2022

vitvakatu commented Feb 21, 2022

vitvakatu Feb 21, 2022

farmaazon Feb 22, 2022

wdanilo commented Feb 23, 2022

vitvakatu left a comment

Creating a new node with the (+) button #3278

Creating a new node with the (+) button #3278

Conversation

farmaazon commented Feb 15, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

wdanilo 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

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

akavel commented Feb 18, 2022 • edited Loading

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

vitvakatu commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdanilo commented Feb 23, 2022

vitvakatu left a comment

Choose a reason for hiding this comment

farmaazon commented Feb 15, 2022 •

edited

Loading

akavel commented Feb 18, 2022 •

edited

Loading