Ability to change the execution environment between design and live. #6341

MichaelMauderer · 2023-04-18T14:24:26Z

Pull Request Description

Integrate the UI for electing the Execution Environment with the Language Server and unify existing uses. Implements #5930 + actual integration instead of just mocking it.

Peek.2023-04-18.23-32.mp4

Important Notes

The console output is only emitted as part of the INFO level. A better check would be to look at the messages sent to the backend in the developer console.

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.

enso-bot · 2023-04-18T22:44:09Z

Michael Mauderer reports a new STANDUP for today (2023-04-18):

Progress: Updated the PR for this task: integrated with the now existing backend API and merged with already implemented functionality on develop. It should be finished by 2023-04-19.

Next Day: Next day I will be working on the #6179 task. Do the same for 6179

wdanilo · 2023-04-19T02:04:09Z

app/gui/controller/engine-protocol/src/language_server/types.rs

+#[derive(Hash, Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Display)]
+pub enum ExecutionEnvironment {
+    /// Allows editing the graph, but the `Output` context is disabled, so it prevents accidental
+    /// changes.
+    Design,
+    /// Unrestricted, live editing of data.
+    Live,


I believe we have the strings "design" and "live" hardcoded somewhere in the previous PR.

I don't think that hardcoding that as enum is a good option, as these 2 options are temporary and we will allow users defining custom modes. So we should keep the current mode in string-like struct instead.

This will also make all of helper functions simpler, such as list_all_imstrings.

Function list_all is similar to a function in previous PR, which was already listing all available execution environments, we should unify these codes.

Here they need to be hard-codes as enum, as this is the current LS protocol. The UI elements can handle arbitrary labels. So, would suggest this to be refactored appropriately once custom modes are implemented.

wdanilo · 2023-04-19T02:04:27Z

app/gui/controller/engine-protocol/src/language_server/types.rs

+    fn try_from(value: &str) -> core::result::Result<Self, Self::Error> {
+        match value {
+            "design" | "Design" => Ok(ExecutionEnvironment::Design),
+            "live" | "Live" => Ok(ExecutionEnvironment::Live),
+            _ => Err(()),
+        }


If we want to be error prone, why don't we just do value.to_lowercase() instead?

wdanilo · 2023-04-19T02:05:26Z

app/gui/src/model/execution_context/synchronized.rs

@@ -308,6 +313,16 @@ impl model::execution_context::API for ExecutionContext {
    fn rename_method_pointers(&self, old_project_name: String, new_project_name: String) {
        self.model.rename_method_pointers(old_project_name, new_project_name);
    }
+
+    fn set_mode(&self, mode: ExecutionEnvironment) -> BoxFuture<FallibleResult> {


sometimes the var with exec env is called environment, sometimes mode. This is a minor inconsistency.

I hope I spotted the last ones I missed. Will double-check again.

wdanilo · 2023-04-19T02:05:52Z

app/gui/src/presenter/project.rs

+                }
+            });
+        } else {
+            error!("Invalid execution mode: {mode:?}");


naming inconsistency - execution mode is not a valid name anymore.

wdanilo · 2023-04-19T02:06:40Z

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

-fn make_entries() -> execution_mode_selector::ExecutionModes {
-    Rc::new(vec!["development".to_string(), "production".to_string()])
+fn make_entries() -> ExecutionEnvironments {
+    Rc::new(vec!["Design".to_string().into(), "Live".to_string().into()])


another hardcoded place with the modes. I would like to ask that all mode names will be defined ONLY in one place in the code.

We don't want all of them in one place only because of crate dependencies: we have a simple demo scene for a chooser that can take arbitrary labels. This element and its demo scene do not need to know about the language server protocol that contains our current ExecutionEnvironment and its valid values. So, it makes sense to just use fixed dummy values there. We can unify those dummy values between the two demo scenes, but I would argue against introducing a dependency on the LS protocol.

wdanilo · 2023-04-19T02:07:22Z

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

+        "Live".to_string().into(),
+        "Design".to_string().into(),


yet another place with mode names. I know this is an example, but as we are keeping it sync'ed with the names, it should probably use the same names defined in one place.

(see above)

wdanilo · 2023-04-19T02:07:41Z

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

+        execution_environment_state <- all(out.execution_environment,frp.set_available_execution_environments);
+
+        execution_environment_toggled <- execution_environment_state.sample(&frp.toggle_execution_environment);
+        toggled_execution_environment <- execution_environment_toggled.map(|(mode,available)| get_next_execution_environment(mode,available)).unwrap();
+
+        external_execution_environment_update <- any(selected_execution_environment,toggled_execution_environment);
+        execution_environment_selector.set_execution_environment <+ external_execution_environment_update;
+


wdanilo · 2023-04-19T02:09:07Z

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

+        init <- source::<()>();
+        size_update <- all(init,execution_environment_selector.size,inputs.space_for_window_buttons);
+        eval size_update ([model]((_,size,gap_size)) {
+            let y_offset = MACOS_TRAFFIC_LIGHTS_VERTICAL_CENTER;
+            let traffic_light_width = traffic_lights_gap_width();
+
+            let execution_environment_selector_x = gap_size.x + traffic_light_width;
+            model.execution_environment_selector.set_x(execution_environment_selector_x);
+            let breadcrumb_gap_width = execution_environment_selector_x + size.x + TOP_BAR_ITEM_MARGIN;
+            model.breadcrumbs.gap_width(breadcrumb_gap_width);
+
+            model.execution_environment_selector.set_y(y_offset + size.y / 2.0);
+            model.breadcrumbs.set_y(y_offset + component::breadcrumbs::HEIGHT / 2.0);


Did you try to use auto-layout instead? I believe the auto-layout would make it a lot simpler. Or is it impossible to be used because of some reasons?

Auto layout would require some larger refactoring we want to avoid doing right now. Some of these components are part of the application and some are part of the graph editor. We should at some point unify them, use layouts and get rid of the constants. Please also note that this is old code that just got moved.

wdanilo · 2023-04-19T02:09:41Z

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

+use ensogl::application::shortcut::ActionType::*;
+
+
+


no sections

…_execution_environment_between_design_and_live_on_shortcut # Conflicts: # app/gui/src/presenter/project.rs # app/gui/view/examples/execution-environment-dropdown/src/lib.rs # app/gui/view/examples/interface/src/lib.rs # app/gui/view/execution-environment-selector/src/lib.rs # app/gui/view/execution-environment-selector/src/play_button.rs

farmaazon · 2023-04-24T08:21:47Z

CHANGELOG.md

+- - [The IDE UI element for selecting the execution mode of the
+    project is now sending messages to the backend.][6341].


farmaazon · 2023-04-24T08:26:08Z

app/gui/src/model/execution_context/plain.rs

+    /// Execution environment of the context.
+    pub execution_environment: Rc<Cell<ExecutionEnvironment>>,


Does it need to be in Rc? This model does not implement CloneRef, and other fields are just RefCells without being wrapped in Rc.

Good catch.

farmaazon · 2023-04-24T08:34:27Z

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

@@ -174,12 +181,12 @@ impl display::Object for Model {


 // =============================
-// === ExecutionModeDropdown ===
+// === ExecutionEnvironmentDropdown ===


Adjust //================= length above and below.

farmaazon · 2023-04-24T08:35:37Z

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

            eval selected_entry ([model] (execution_mode) {
-                // TODO(#5930): Revisit when connecting with externally set execution mode
-                let play_button_visibility = match execution_mode.as_str() {
-                    "design" => true,
-                    "live" => false,
-                    _ => {
-                        error!("Play button: invalid execution mode");
-                        false
-                    }
-                };
+                let play_button_visibility = matches!(execution_mode.to_lowercase().as_str(), "design");


Why TODO was removed? The logic hasn't changed, has it?

This is implementing the referenced issue (5930) and it appears there is nothing else to do, so no point in keeping the reference to a son to be closed task. This probably should be revisited when we get the custom modes that Wojciech mentioned din one of his other comments.

I guess if we really want to be prepared for it, we could already an API that allows us to set which execution environments should show the play button, but for the current spec that is not needed.

farmaazon · 2023-04-24T08:37:55Z

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

+    profiling_button: component::profiling::Button,
+    styles_frp: StyleWatchFrp,
+    selection_controller: selection::Controller,
+    execution_environment_selector: execution_environment_selector::ExecutionEnvironmentSelector,


I would import ExecutionEnvironmentSelector, as the module name is just repeated information.

…_execution_environment_between_design_and_live_on_shortcut # Conflicts: # app/gui/src/presenter/project.rs # app/gui/view/graph-editor/src/lib.rs

vitvakatu

QA Review: 🔴

Read-only mode is broken. See the comment below.
Changing the execution environment doesn't update the icons on nodes. It might be solved later in Finish execution contexts integration in IDE #6353, so not blocking the merge. I will try to find the needed code change for that later.
(minor side issue) The dropdown is shifted to the side, I believe it should be displayed more to the left, probably align with the green box. I will create a separate issue for that.

At the project's opening, we send a setExecutionEnvironment message (sometimes even two of them). They fail, probably because the engine is not yet ready. I think we shouldn't send them, the engine assumes the project is in a design environment by default.

vitvakatu · 2023-04-24T12:34:06Z

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

        // TODO(#5930): Temporary shortcut for testing different execution environments
        toggle_execution_environment(),
+        /// Set the execution environmenta available to the graph.
+        set_available_execution_environments          (Rc<Vec<execution_environment_selector::ExecutionEnvironment>>),


Suggested change

set_available_execution_environments (Rc<Vec<execution_environment_selector::ExecutionEnvironment>>),

set_available_execution_environments (Rc<Vec<ExecutionEnvironment>>),

vitvakatu · 2023-04-24T12:34:49Z

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

+    // === Profiling Mode ===
+    (Press, "", "cmd p", "toggle_profiling_mode"),
+    // === Execution Mode ===
+    (Press, "", "shift ctrl e", "toggle_execution_environment"),


Let's add this new shortcut to the documentation (shortcuts.md)

(as a debug shortcut)

vitvakatu · 2023-04-24T12:40:28Z

app/gui/src/presenter/project.rs

@@ -385,22 +404,26 @@ impl Project {
            eval_ view.execution_context_restart(model.execution_context_restart());

            view.set_read_only <+ view.toggle_read_only.map(f_!(model.toggle_read_only()));
+            eval graph_view.execution_environment((mode) model.execution_environment_changed(mode));
+
+            eval_ view.toggle_read_only(model.toggle_read_only());


Why this line is needed? We already have

view.set_read_only <+ view.toggle_read_only.map(f_!(model.toggle_read_only()));

slightly above. I see that read-only mode is broken in a weird way (after toggling it twice removing nodes works, but dragging edges doesn't)

I think this is a merge error and the line is removed now.

…_execution_environment_between_design_and_live_on_shortcut

vitvakatu · 2023-04-25T19:58:42Z

QA Report 🟢

We are merging it since we need this functionality as soon as possible. However, a few issues must be addressed in the next PR. @MichaelMauderer

I still see the Request timeout error message when opening the project. Probably, we send the request when initializing the FRP of the dropdown. If it is not easy to fix, please create a separate task.
Context switch buttons on nodes need to be updated. I fixed it here: c87ab85. The relevant line is here: c87ab85#diff-55dbc90f2ab0a6203fb1c2a1d80abf6ff798b92b7ec80a9c27a8c8cc7cf0c912R1733. While doing so, I noticed a few things:
a. set_execution_context input of the graph editor is not used anywhere except for one place. We can probably remove it.
b. I understand the idea, but the fact that the current dropdown API operates ImString instead of an enum makes implementation harder and requires parsing. See c87ab85#diff-48811d492864133eabe64a151ba79a4d0ebf0e8b8314dfa12d00bc5e94d1b181R51 and how the implementation got simpler in presenter/project.rs
c. We probably don't need the toggle_execution_environment shortcut anymore. Now we can always use the dropdown itself. Removing it will simplify implementation in some places.
There are three places marked with TODO(#5930) in the code. It must be addressed.

…6341) Integrate the UI for electing the Execution Environment with the Language Server and unify existing uses. Implements #5930 + actual integration instead of just mocking it. https://user-images.githubusercontent.com/1428930/232919438-6e1e295a-34fe-4756-86a4-5f5d8f718fa0.mp4 # Important Notes The console output is only emitted as part of the `INFO` level. A better check would be to look at the messages sent to the backend in the developer console.

* develop: Passing events to sub-widgets in List Editor and refactoring of the slider component. (#6433) Revert "Cloud/desktop mode switcher (#6308)" (#6444) Widgets integrated with graph nodes (#6347) Table Visualization and display text changes. (#6382) Skip redundant compilations (#6436) Add parse extensions to Text type. #6330 (#6404) Cloud/desktop mode switcher (#6308) Replace Table should_equal with should_equal_verbose (#6405) Rollback event handling changes for the mouse (#6396) Dashboard directory interactivity (#6279) Ability to change the execution environment between design and live. (#6341) Implementation of elements adding to List Editor and a lot of internal API (#6390) Drop method exported from WASM + removing leaks. (#6365) Turn null into UnexpectedExpression when Union type is incomplete (#6415)

Follow up to #6341 (comment) . Contains some refactoring and solves some left over to-dos in the code.

MichaelMauderer added 6 commits April 18, 2023 12:53

Immplement shortcut handling.

f49457a

Implement controller handling.

877d2a9

./run fmt, ./run lint

5001a5b

Fix clickable area size.

527ed4d

Rebase to develop and merge changes.

348c7a6

Integrate with new existing functionality.

cbc3d1a

MichaelMauderer self-assigned this Apr 18, 2023

MichaelMauderer added 4 commits April 18, 2023 15:39

Refactoring & code style.

f00c012

Fix naming of parameters.

bbb4352

Revert unintended changes.

d473bd1

Changelog.

11e293c

MichaelMauderer marked this pull request as ready for review April 18, 2023 22:40

MichaelMauderer requested review from wdanilo, farmaazon, kazcw and mwu-tow as code owners April 18, 2023 22:40

wdanilo requested changes Apr 19, 2023

View reviewed changes

MichaelMauderer added 3 commits April 21, 2023 10:33

Review feedback.

40f2bb7

Removed another instance of "mode"

74a692b

Unify dummy data use.

bdf8a5a

MichaelMauderer requested a review from wdanilo April 21, 2023 09:59

MichaelMauderer added 4 commits April 21, 2023 11:01

Fix merge issues.

6bb1dfd

Remove whitespace.

468d25b

Restore debug functionality.

c1ffc83

farmaazon approved these changes Apr 24, 2023

View reviewed changes

MichaelMauderer added 3 commits April 24, 2023 09:43

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

4faf47f

…_execution_environment_between_design_and_live_on_shortcut # Conflicts: # app/gui/src/presenter/project.rs # app/gui/view/graph-editor/src/lib.rs

Resolve merge conflicts.

bad2e6e

Review feedback.

4588e57

wdanilo approved these changes Apr 24, 2023

View reviewed changes

vitvakatu requested changes Apr 24, 2023

View reviewed changes

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

When picking a Boolean value via the dropdown doesn't recalculate correctly. #6221

Closed

MichaelMauderer and others added 7 commits April 25, 2023 14:26

QA feedback.

7ca79ad

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

b4b2c98

…_execution_environment_between_design_and_live_on_shortcut

Revert debug logging.

21397f5

Fix shortcuts. Revert read only refactoring.

ee8fec7

Remove TRACE_ALL

ed4af57

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

8376deb

…_execution_environment_between_design_and_live_on_shortcut

Fix compilation and formatting

a5cafba

vitvakatu approved these changes Apr 25, 2023

View reviewed changes

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

mergify bot merged commit 0d84a60 into develop Apr 25, 2023

mergify bot deleted the wip/MichaelMauderer/Ability_to_change_the_execution_environment_between_design_and_live_on_shortcut branch April 25, 2023 20:28

Procrat mentioned this pull request Apr 26, 2023

Can not click on a dropdown to change execution mode if the mouse pointer is above the text #6371

Closed

2 tasks

MichaelMauderer mentioned this pull request Apr 26, 2023

Execution environment integration fixes. #6434

Merged

5 tasks

mergify bot pushed a commit that referenced this pull request Apr 27, 2023

Execution environment integration fixes. (#6434)

233bf86

Follow up to #6341 (comment) . Contains some refactoring and solves some left over to-dos in the code.

vitvakatu mentioned this pull request Apr 27, 2023

Ability to change the execution environment between design and live on shortcut. #5930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to change the execution environment between design and live. #6341

Ability to change the execution environment between design and live. #6341

MichaelMauderer commented Apr 18, 2023 •

edited

Loading

enso-bot bot commented Apr 18, 2023

wdanilo Apr 19, 2023

MichaelMauderer Apr 21, 2023

wdanilo Apr 19, 2023

wdanilo Apr 19, 2023

MichaelMauderer Apr 21, 2023

wdanilo Apr 19, 2023

wdanilo Apr 19, 2023

MichaelMauderer Apr 21, 2023

wdanilo Apr 19, 2023

MichaelMauderer Apr 21, 2023

wdanilo Apr 19, 2023

wdanilo Apr 19, 2023

MichaelMauderer Apr 21, 2023

wdanilo Apr 19, 2023

farmaazon Apr 24, 2023

farmaazon Apr 24, 2023

MichaelMauderer Apr 24, 2023

farmaazon Apr 24, 2023

farmaazon Apr 24, 2023

MichaelMauderer Apr 24, 2023

farmaazon Apr 24, 2023

vitvakatu left a comment

vitvakatu Apr 24, 2023

vitvakatu Apr 24, 2023

vitvakatu Apr 24, 2023

MichaelMauderer Apr 25, 2023

vitvakatu commented Apr 25, 2023

		- - [The IDE UI element for selecting the execution mode of the
		project is now sending messages to the backend.][6341].

		/// Execution environment of the context.
		pub execution_environment: Rc<Cell<ExecutionEnvironment>>,

	set_available_execution_environments (Rc<Vec<execution_environment_selector::ExecutionEnvironment>>),
	set_available_execution_environments (Rc<Vec<ExecutionEnvironment>>),

Ability to change the execution environment between design and live. #6341

Ability to change the execution environment between design and live. #6341

Conversation

MichaelMauderer commented Apr 18, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

enso-bot bot commented Apr 18, 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

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 left a comment

Choose a reason for hiding this comment

QA Review: 🔴

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 Apr 25, 2023

QA Report 🟢

MichaelMauderer commented Apr 18, 2023 •

edited

Loading