A drop-down that allows changing the execution mode. #6130

MichaelMauderer · 2023-03-29T12:17:13Z

Pull Request Description

Implements #5931.

Peek.2023-03-29.13-15.mp4

Important Notes

Not functional yet, as it needs integration with the engine.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

farmaazon

In the video you posted, the entry labels in drop-down are not centered vertically in selection:

Please change (or remove) the offset I suspect we're adding there.

app/gui/view/examples/icons/src/lib.rs

app/gui/view/execution-mode-selector/Cargo.toml

app/gui/view/execution-mode-selector/src/lib.rs

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

wdanilo · 2023-03-31T07:25:05Z

In the video you posted, the entry labels in drop-down are not centered vertically in selection:

Please change (or remove) the offset I suspect we're adding there.

I just did the same screenshot :D

wdanilo · 2023-03-31T07:25:47Z

app/gui/view/examples/execution-mode-dropdown/src/lib.rs

+use ide_view_execution_mode_selector as execution_mode_selector;
+
+
+


wdanilo · 2023-03-31T07:26:39Z

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

@@ -254,6 +254,13 @@ fn init(app: &Application) {
    graph_editor.set_node_profiling_status(node3_id, node3_status);


+    // === Execution Modes ===
+
+    graph_editor.set_execution_modes(vec!["development".to_string(), "production".to_string()]);


These strings ar defiend in another file as well

These are dummy strings anyway. The ones in the demo scene will only be used there, while the other ones will be removed during integration.

app/gui/view/execution-mode-selector/src/lib.rs

wdanilo · 2023-03-31T07:28:46Z

app/gui/view/execution-mode-selector/src/lib.rs

+        set_execution_modes      (ExecutionModes),
+    }
+    Output {
+        selected_ecxecution_mode (ExecutionMode),


wdanilo · 2023-03-31T07:29:11Z

app/gui/view/execution-mode-selector/src/lib.rs

+
+ensogl::define_endpoints_2! {
+    Input {
+        set_execution_modes      (ExecutionModes),


This sounds like selecting execution modes. Maybe it should be named set_available_execution_modes instead?

wdanilo · 2023-03-31T07:30:04Z

app/gui/view/execution-mode-selector/src/lib.rs

+            });
+
+            // Inputs
+


why this nl? Code above and below does not use nl after such comments.

Added the space in the above place for consistency.

But literally 7 lines below you have the exact same problem now. I think this is a good place to use sub-sections, as their layout rules are unified across files and they are very close to what you have here.

I do not know what to write here. I mean, this discussion should already be resolved, but .... it is still inconsistent ...

Doing review of the same code so many times is super time consuming :(

wdanilo · 2023-03-31T07:30:17Z

app/gui/view/execution-mode-selector/src/lib.rs

+            eval_ update_position ([model, camera] {
+                model.update_position(&camera);
+            });


you don't need curly braces here and this can be one-line expressions.

Technically, there is a difference, because here we discard potential return value from update_position.

But more important is, that rustfmt formats too long lambdas this way, and we are meant to apply same formatting rules to macros.

Regarding rustfmt, without curly braces I believe rustfmt would format that one-line.

Oh, you are right - it adds braces only when extended 100 characters limit.

wdanilo · 2023-03-31T07:31:01Z

app/gui/view/execution-mode-selector/src/lib.rs

+            let camera_changed = scene.frp.camera_changed.clone_ref();
+            update_position <- any(init, camera_changed);


Why are we making position update on camera change? Instead, this should be added to al ayer with camera that does not move with navigator.

This update is not about camera movement, but the viewport size change. As we want this to be aligned with the top of the window. This seems to be what all the components placed this way are using at the moment.

The alternative would be a new layer with a fixed origin at the top left window corner, but that would also not work for things placed at the bottom. We could fix the coordinates of that layer to be 0..1, but then finding the correct position for an object would be an issue as it would depend on the scaling. Or am I missing something?

Hmm, we shouldd probably have a layer aligned to top left corner and place such items there. Items aligned to bottom left one would use another layer then. I mean, it should not be the responsibility of elements to place themselves. Nevertheless, this is a broader discussion and im ok with not fixing it here.

wdanilo · 2023-03-31T07:31:44Z

app/gui/view/graph-editor/src/component/visualization/container/visualization_chooser.rs

+//! values. It should also be refactored to use `drop_down_menu` from `ensogl_components` instead
+//! of the old list view.


Why are we not using the new drop-down in the first place here?

The "new drop-down" only implements the part that opens and closes, while this component has the "clickable selected item". Ideally, they should be refactored to be merged, but for this use cases, the "old one" is more appropriate (and the additional refactoring not required at this time).

wdanilo · 2023-03-31T07:33:14Z

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

+        let traffic_light_width = traffic_lights_gap_width();
+
+        self.add_child(&self.execution_mode_selector);
+        self.execution_mode_selector
+            .set_x(traffic_light_width + execution_mode_selector::OVERALL_WIDTH / 2.0);
+        self.execution_mode_selector.set_y(y_offset + execution_mode_selector::HEIGHT / 2.0);


The position of the execution mode selector should be reactive. It should probably be computed with FRP based on the width of traffic light. Even better, you can use our auto-layout to place execution mode next to traffic light. The description of how to use auto-layout is in display::object module.

It actually is reactive based on the FRP updates below. This is just the default initialization. The reason I did not go with the layout is, that the traffic lights are: (a) not currently part of the Graph Editor and (b) as far as I am aware, are not always present, as on macOS there are the native controls placed there and this offset is applied without a corresponding EnsoGL component.

(also some changes happening due to use of the theme here soon to be visible)

Regarding auto-layouts - they work great when some items are not always present. Just FYI - you can create auto grid layout and when item is missing, other items will be layout correctly.

…r/Execution_Context_Dropdown_Menu # Conflicts: # CHANGELOG.md

MichaelMauderer · 2023-04-03T19:33:48Z

Updated video.

Peek.2023-04-03.14-04.mp4

wdanilo · 2023-04-04T07:58:46Z

app/gui/view/execution-mode-selector/src/lib.rs

+
+impl Model {
+    fn update_dropdown_style(&self, overall_width: f32, divider_offset: f32) {
+        self.dropdown.set_menu_offset_y(20.0);


hardcoded literal - could you please extract it to a well named const? :)

wdanilo · 2023-04-04T07:59:25Z

app/gui/view/execution-mode-selector/src/lib.rs

+    }
+
+    fn update_play_icon_style(&self, max_width: f32, play_button_offset: f32) {
+        self.play_icon.set_size(Vector2::new(10.0, 11.0));


Hardcoded literals, also, the size of 10x11 looks strange - could you provide some explanation next to the definition of that it is, please?

wdanilo · 2023-04-04T08:01:23Z

app/gui/view/execution-mode-selector/src/lib.rs

+            });
+
+            // Inputs
+


But literally 7 lines below you have the exact same problem now. I think this is a good place to use sub-sections, as their layout rules are unified across files and they are very close to what you have here.

wdanilo · 2023-04-04T08:02:21Z

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

+        // breadcrumb_gap_update <- all(inputs.space_for_window_buttons,size_update);
+        // eval breadcrumb_gap_update([model]((gap_size, execution_mode_selector_size)) {
+        //     let traffic_light_width = traffic_lights_gap_width();
+        //     let breadcrumb_gap_width = traffic_light_width + execution_mode_selector_size.x + TOP_BAR_ITEM_MARGIN;
+        //
+        //     let execution_mode_selector_x = traffic_light_width + execution_mode_selector_size.x / 2.0;
+        //     model.execution_mode_selector.set_x(gap_size.x + execution_mode_selector_x);
+        //     model.breadcrumbs.gap_width(gap_size.x + breadcrumb_gap_width);
+        // });


commented code

wdanilo · 2023-04-04T08:02:43Z

lib/rust/ensogl/app/theme/hardcoded/src/lib.rs

+                background = Rgb::from_base_255(100.0, 181.0, 38.0) , Rgb::from_base_255(100.0, 181.0, 38.0);
+                divider = Rgba::black_with_alpha(0.12) , Rgba::black_with_alpha(0.12);
+                triangle = Rgba::white_with_alpha(0.75) , Rgba::white_with_alpha(0.75);
+                play_button_size = 10.0 , 10.0;
+                play_button_offset = 15.0 , 15.0;
+                play_button_padding = 10.0 , 10.0;
+                divider_offset = 32.5 , 32.5;
+                divider_padding = 10.0 , 10.0;
+                dropdown_width = 95.0 , 95.0;
+                height = 24.0 , 24.0;


very big indentation in the code

wdanilo · 2023-04-04T10:31:34Z

app/gui/view/execution-mode-selector/src/lib.rs

+            });
+
+            // Inputs
+


I do not know what to write here. I mean, this discussion should already be resolved, but .... it is still inconsistent ...

Doing review of the same code so many times is super time consuming :(

MichaelMauderer · 2023-04-06T09:04:52Z

@wdanilo @farmaazon This awaits your re-review.

…pdown_Menu

vitvakatu · 2023-04-07T09:07:09Z

app/gui/view/graph-editor/src/component/visualization/instance.rs

+
+            eval send_data ([](data) error!("Received data: {:?}",data));
+            eval deactivate ([](_) error!("Deactivated"));


I think there are leftover debug logs here.

vitvakatu · 2023-04-07T10:02:31Z

QA report 🟡

In general, I didn't spot any issues on my machine. But:

The play button shape was too small

Only the triangle could capture mouse events, so it was quite hard to press it. I changed the code slightly to fix that and pushed the commit f39f74d

🔴 @Procrat reported that dropdown does not work on his machine

The list does not react on clicking. I didn't spot any obvious issues with layers, and it is not reproducible on my machine.

The video by @Procrat:

videos.zip

MichaelMauderer · 2023-04-11T07:03:36Z

Only the triangle could capture mouse events, so it was quite hard to press it. I changed the code slightly to fix that and pushed the commit f39f74d

Thanks!

red_circle @Procrat reported that dropdown does not work on his machine

The list does not react on clicking. I didn't spot any obvious issues with layers, and it is not reproducible on my machine.

The video by @Procrat:

videos.zip

That is very concerning! I'll have a closer look at the videos in a bit.
@Procrat are you able to select visualizations from the visualization dropdown menu on this branch and on develop? (That menu uses the same component).

…pdown_Menu

vitvakatu

QA: passed

…pdown_Menu

kazcw · 2023-04-17T15:54:39Z

Bug for the WASM test failure: #6315

…pdown_Menu

enso-5931 A drop-down that allows changing the execution mode.

11945cd

MichaelMauderer self-assigned this Mar 29, 2023

MichaelMauderer added d-intermediate Difficulty: some prior knowledge required p-high Should be completed in the next sprint -gui labels Mar 29, 2023

MichaelMauderer added 2 commits March 29, 2023 13:20

Changelog update.

a47b113

Revert opt level change.

739e2f9

MichaelMauderer marked this pull request as ready for review March 29, 2023 12:48

MichaelMauderer requested review from wdanilo, farmaazon, kazcw and mwu-tow as code owners March 29, 2023 12:48

farmaazon requested changes Mar 30, 2023

View reviewed changes

Reduce unused crate dependencies.

2abc196

wdanilo requested changes Mar 31, 2023

View reviewed changes

MichaelMauderer added 3 commits March 31, 2023 11:45

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

6426666

…r/Execution_Context_Dropdown_Menu # Conflicts: # CHANGELOG.md

Implement review feedback.

d811fd2

Add docs.

883cb43

MichaelMauderer requested review from wdanilo and farmaazon April 3, 2023 18:56

wdanilo approved these changes Apr 4, 2023

View reviewed changes

Review feedback.

37ec08f

wdanilo requested changes Apr 4, 2023

View reviewed changes

Layout.

6632a6f

MichaelMauderer requested a review from wdanilo April 4, 2023 13:00

MichaelMauderer added 2 commits April 6, 2023 10:32

Merge branch 'develop' into wip/MichaelMauderer/Execution_Context_Dro…

2031123

…pdown_Menu

Fix shape offset.

ed2e4f5

Make the shape of the Play button bigger

f39f74d

vitvakatu reviewed Apr 7, 2023

View reviewed changes

MichaelMauderer added 5 commits April 11, 2023 09:21

Remove logging.

b4f06ca

Merge branch 'develop' into wip/MichaelMauderer/Execution_Context_Dro…

70a12ce

…pdown_Menu

Fix click overlay.

39ce6a2

Add docs for workaround.

9fda421

Fix default layer label.

9d38b2b

enso-bot bot mentioned this pull request Apr 11, 2023

Workflow-level Play button business-logic #6179

Closed

vitvakatu approved these changes Apr 12, 2023

View reviewed changes

farmaazon approved these changes Apr 12, 2023

View reviewed changes

wdanilo approved these changes Apr 14, 2023

View reviewed changes

farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Apr 14, 2023

MichaelMauderer added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Apr 16, 2023

Merge branch 'develop' into wip/MichaelMauderer/Execution_Context_Dro…

2781f0a

…pdown_Menu

MichaelMauderer force-pushed the wip/MichaelMauderer/Execution_Context_Dropdown_Menu branch from d8b6994 to 2781f0a Compare April 16, 2023 17:09

MichaelMauderer mentioned this pull request Apr 17, 2023

Error: failed to deserialize wasm module rustwasm/wasm-pack#1273

Open

Merge branch 'develop' into wip/MichaelMauderer/Execution_Context_Dro…

71f3bf0

…pdown_Menu

Procrat mentioned this pull request Apr 17, 2023

Make play button clickable and hide it when irrelevant, take #2 #6302

Merged

Merge branch 'develop' into wip/MichaelMauderer/Execution_Context_Dro…

1748b1f

…pdown_Menu

MichaelMauderer added the CI: Keep up to date Automatically update this PR to the latest develop. label Apr 18, 2023

Merge branch 'develop' into wip/MichaelMauderer/Execution_Context_Dro…

a004adb

…pdown_Menu

mergify bot merged commit 6404332 into develop Apr 18, 2023

mergify bot deleted the wip/MichaelMauderer/Execution_Context_Dropdown_Menu branch April 18, 2023 10:26

vitvakatu mentioned this pull request Apr 19, 2023

A drop-down that allows changing the execution mode. #5931

Closed

		use ide_view_execution_mode_selector as execution_mode_selector;

		let camera_changed = scene.frp.camera_changed.clone_ref();
		update_position <- any(init, camera_changed);

		//! values. It should also be refactored to use `drop_down_menu` from `ensogl_components` instead
		//! of the old list view.


		eval send_data ([](data) error!("Received data: {:?}",data));
		eval deactivate ([](_) error!("Deactivated"));

A drop-down that allows changing the execution mode. #6130

A drop-down that allows changing the execution mode. #6130

Conversation

MichaelMauderer commented Mar 29, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

farmaazon left a comment

Choose a reason for hiding this comment

wdanilo commented Mar 31, 2023

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

wdanilo Mar 31, 2023 • edited by farmaazon Loading

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

MichaelMauderer commented Apr 3, 2023 • edited Loading

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

MichaelMauderer commented Apr 6, 2023

Choose a reason for hiding this comment

vitvakatu commented Apr 7, 2023

QA report 🟡

The play button shape was too small

🔴 @Procrat reported that dropdown does not work on his machine

MichaelMauderer commented Apr 11, 2023

red_circle @Procrat reported that dropdown does not work on his machine

vitvakatu left a comment

Choose a reason for hiding this comment

kazcw commented Apr 17, 2023

MichaelMauderer commented Mar 29, 2023 •

edited

Loading

wdanilo Mar 31, 2023 •

edited by farmaazon

Loading

MichaelMauderer commented Apr 3, 2023 •

edited

Loading