Create new project action in searcher. #1566

farmaazon · 2021-05-12T16:02:56Z

Pull Request Description

This PR implements a new action in searcher for creating new project; the project will be opened. It has a lot of refactoring of our controllers to support closing and opening new projects

Checklist

Please include the following checklist in your PR:

The CHANGELOG.md was updated with the changes introduced in this PR.
The documentation has been updated if necessary.
All code conforms to the Rust style guide.
All code has automatic tests where possible.
All code has been profiled where possible.
All code has been manually tested in the IDE.
~~All code has been manually tested in the "debug/interface" scene.~~
All code has been manually tested by the PR owner against our test scenarios.
All code has been manually tested by at least one reviewer against our test scenarios.

…project2

mwu-tow · 2021-05-12T19:08:41Z

src/rust/ide/src/controller/ide.rs

+// === Status Notifications ===
+// ============================
+
+pub type ProcessHandle = usize;


I don't like the name Process here, initially I thought about different kind of process.
Perhaps something more along lines of "Job", "Task", "BackgroundAction", etc?

mwu-tow · 2021-05-12T21:06:33Z

src/rust/ide/src/controller/ide.rs

+impl StatusNotifications {
+    pub fn new() -> Self { default() }
+
+    pub fn publish_event(&self, label:impl Into<String>) {


I'd bikeshed a few more names here.

I dont understand this comment

mwu-tow · 2021-05-12T21:09:14Z

src/rust/ide/src/controller/ide/cloud.rs

+        // TODO[ao]: we should think how to handle engine's versions in cloud.
+        //     https://github.com/enso-org/ide/issues/1195
+        let version              = semver::Version::parse(controller::project::ENGINE_VERSION_FOR_NEW_PROJECTS)?;
+        let project_id           = default();


I don't think that project_id should ever be default.

Add TODO and issue.

src/rust/ide/src/controller/ide/desktop.rs

src/rust/ide/src/controller/project.rs

mwu-tow · 2021-05-17T12:16:50Z

src/rust/ide/src/controller/searcher/action.rs

@@ -41,6 +43,7 @@ impl Action {
                completion.name.clone()
            }
            Self::Example(example) => format!("Example: {}", example.name),
+            Self::CreateNewProject => "Create New Project".to_owned(),


Suggested change

Self::CreateNewProject => "Create New Project".to_owned(),

Self::CreateNewProject => "Create a New Project".to_owned(),

but then it looks like PoKeMoN script.
Perhaps just New Project?

mwu-tow · 2021-05-17T12:23:47Z

src/rust/ide/src/model/project/synchronized.rs

@@ -17,7 +17,8 @@ use enso_protocol::language_server::MethodPointer;
 use enso_protocol::project_manager;
 use flo_stream::Subscriber;
 use parser::Parser;
-
+use crate::transport::web::WebSocket;


unordered use decls

mwu-tow · 2021-05-17T12:25:14Z

src/rust/ide/src/model/project/synchronized.rs

 #[derive(Debug,Clone)]
-pub struct Data {
+pub struct Properties {


Perhaps its Flowbox legacy, but seeing properties used caused me think that this is something totally different.

mwu-tow · 2021-05-17T12:26:43Z

src/rust/ide/src/controller/ide.rs

+    // Automock macro does not work without explicit lifetimes here.
+    #[allow(clippy::needless_lifetimes)]
+    fn manage_projects<'a>(&'a self) -> Option<&'a dyn ManagingProjectAPI>;
+}


Perhaps IDE should also provide parser?

mwu-tow · 2021-05-17T12:28:52Z

src/rust/ide/src/controller/ide.rs

+    /// The model of currently opened project.
+    fn current_project(&self) -> model::Project;


So we still assume that IDE has exactly one project opened. I wonder if this should be documented in the trait.

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

…project2

wdanilo · 2021-05-17T22:39:30Z

CHANGELOG.md

@@ -42,13 +70,24 @@ you can find their release notes
 #### Visual Environment

 - [Window management buttons.][1511]. The IDE now has components for
+<<<<<<< HEAD


Thisi s a commited file with git conflicts!

wdanilo · 2021-05-17T22:40:13Z

src/rust/ide/src/controller/ide.rs

+impl StatusNotifications {
+    pub fn new() -> Self { default() }
+
+    pub fn publish_event(&self, label:impl Into<String>) {


I dont understand this comment

wdanilo · 2021-05-17T22:40:37Z

src/rust/ide/src/controller/ide.rs

+}
+
+/// A polymorphic handle of IDE controller.
+pub type Ide  = Rc<dyn API>;


wdanilo · 2021-05-17T22:42:27Z

src/rust/ide/src/controller/ide/plain.rs

@@ -0,0 +1,83 @@
+//! The Plain IDE Controller


missing dot.

wdanilo · 2021-05-17T22:43:42Z

src/rust/ide/src/controller/ide/plain.rs

+        //TODO [ao]: this should be not the default; instead project model should not need the id.
+        //    See also https://github.com/enso-org/ide/issues/1572


Missing link to an issue. I understand that the first line describes an issue (missing link there) and the second line refers to yet another issue (because to the wording "see also") - is that correct?

Ok, I remove the also.

wdanilo · 2021-05-17T22:45:32Z

src/rust/ide/src/controller/searcher.rs

+                            // We checked that manage_projects returns Some just a moment ago, so
+                            // unwrapping is safe.


the "Some" is not correctly formatted

What does it mean "just a moment ago"?

The safety is not clearly seen in the code, imo this should be refactored.

It's a comment, not documentation, so I don't see any need for formatting. But it should be Ok after the refactoring

You're in match checking if it returned Ok.

It's quite hard to refactor; already discussed with @mwu-tow : it would make the manage_project method return Rc, not a reference, and thus we couldn't return self what is quite handy here.

wdanilo · 2021-05-17T22:46:35Z

src/rust/ide/src/ide/initializer.rs

            let application   = Application::new(&web::get_html_element_by_id("root").unwrap());
            let view          = application.new_view::<ide_view::project::View>();
            let status_bar    = view.status_bar().clone_ref();
+            // We know the name of new project before it actually loads. We set it right now to


the word "actually" is not needed here.

…project2

Original commit: enso-org/ide@5adc952

farmaazon added 5 commits April 27, 2021 16:30

WIP

074aefe

WIP

ca89039

Refactored project setup

c44ad20

Create new project action

ec93597

Fixes

1be198c

farmaazon requested a review from mwu-tow May 12, 2021 16:03

farmaazon added 2 commits May 13, 2021 15:46

Code cleanup

1b11b70

Self-review

6df60f5

farmaazon marked this pull request as ready for review May 13, 2021 14:07

farmaazon requested a review from wdanilo as a code owner May 13, 2021 14:07

farmaazon self-assigned this May 13, 2021

Merge remote-tracking branch 'origin/develop' into wip/ao/create-new-…

99f32f1

…project2

farmaazon force-pushed the wip/ao/create-new-project2 branch from 3463a1e to 99f32f1 Compare May 13, 2021 14:26

Fix compilation

9a65231

mwu-tow approved these changes May 17, 2021

View reviewed changes

farmaazon and others added 10 commits May 17, 2021 14:47

Update src/rust/ide/src/controller/ide/desktop.rs

679d654

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

Update src/rust/ide/src/controller/ide/desktop.rs

a0ee9e5

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

Update src/rust/ide/src/controller/ide/desktop.rs

589a403

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

Update src/rust/ide/src/controller/project.rs

5148154

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

Update src/rust/ide/src/controller/ide/plain.rs

58b10ec

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

Update src/rust/ide/src/controller/project.rs

b929f64

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

Update src/rust/ide/src/controller/project.rs

75d5366

Co-authored-by: Michał Wawrzyniec Urbańczyk <mwu-tow@gazeta.pl>

Fixes

384b9a0

Applying @mwu-two review

b675ee4

Merge remote-tracking branch 'origin/develop' into wip/ao/create-new-…

ab849cb

…project2

wdanilo approved these changes May 17, 2021

View reviewed changes

farmaazon and others added 6 commits May 18, 2021 11:28

Applying @wdanilo review

245c21f

Merge remote-tracking branch 'origin/develop' into wip/ao/create-new-…

84921d7

…project2

Linter fixes

a58d8bc

Merge branch 'develop' into wip/ao/create-new-project2

9b9c14d

prettify CHANGELOG

ccda769

Fix tests

c264a2c

farmaazon merged commit 5adc952 into develop May 21, 2021

farmaazon deleted the wip/ao/create-new-project2 branch May 21, 2021 13:24

mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021

Create new project action in searcher. (enso-org/ide#1566)

4b14e16

Original commit: enso-org/ide@5adc952

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new project action in searcher. #1566

Create new project action in searcher. #1566

farmaazon commented May 12, 2021 •

edited

mwu-tow May 12, 2021

mwu-tow May 12, 2021

wdanilo May 17, 2021

mwu-tow May 12, 2021

farmaazon May 17, 2021

mwu-tow May 17, 2021

mwu-tow May 17, 2021

mwu-tow May 17, 2021

mwu-tow May 17, 2021

mwu-tow May 17, 2021

wdanilo May 17, 2021

wdanilo May 17, 2021

wdanilo May 17, 2021

wdanilo May 17, 2021

wdanilo May 17, 2021

farmaazon May 18, 2021

wdanilo May 17, 2021

farmaazon May 18, 2021

wdanilo May 17, 2021

	Self::CreateNewProject => "Create New Project".to_owned(),
	Self::CreateNewProject => "Create a New Project".to_owned(),

		/// The model of currently opened project.
		fn current_project(&self) -> model::Project;

		//TODO [ao]: this should be not the default; instead project model should not need the id.
		// See also https://github.com/enso-org/ide/issues/1572

		// We checked that manage_projects returns Some just a moment ago, so
		// unwrapping is safe.

Create new project action in searcher. #1566

Create new project action in searcher. #1566

Conversation

farmaazon commented May 12, 2021 • edited

Pull Request Description

Checklist

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

farmaazon commented May 12, 2021 •

edited