Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Node Model #268

Merged
merged 124 commits into from
Apr 3, 2020
Merged

Node Model #268

merged 124 commits into from
Apr 3, 2020

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented Mar 17, 2020

Pull Request Description

  • Implements Nodes graph IDE model.
  • Implements Shapes animations in GUI. In order to implement it a big refactoring to our animation system was done.
  • Partially implements Node selection. It allows for selecting nodes by clicking, it displays a rectangular area for group node selection, but the rectangular selection does not work yet. Its implementation is complex (it was described in the task as research task) and it will be delivered in some other PR in the future when our FRP model will settle down more.
  • Contains a lot of refactoring mostly needed for the tasks above and connected with improper, too wide RefCell usage. Now our codebase has much less RefCells! ❤️

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Rust, Scala, Java or Haskell style guides as appropriate.
  • All code has been tested where possible.

@wdanilo wdanilo changed the title Node Selection Node Model Mar 18, 2020
@@ -29,7 +30,7 @@ members = [
]

[profile.dev]
opt-level = 0
opt-level = 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check how it works with tests and CI caching.

@@ -194,7 +195,7 @@ commands.watch = command(`Start a file-watch utility and run interactive mode`)
commands.watch.parallel = true
commands.watch.rust = async function() {
let target = '"' + `node ${paths.script.main} build --no-js --dev -- ` + subProcessArgs.join(" ") + '"'
let args = ['watch','--watch','lib','-s',`${target}`]
let args = ['watch','-s',`${target}`]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to add --npm flag

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I did something simpler (as discussed via phone). All flags after -- are passed to cargo only (not to npm anymore).

impl Default for SimulationThresholds {
fn default() -> Self {
Self::new(0.1,0.1)
impl<T:Position,Cb> CloneRef for Simulator<T,Cb> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derive(CloneRef) - checke every impl CloneRef in your PR if it may be replaced with derive

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not everywhere as CloneRef deriving does not work yet for structs with bounds, but done where possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is ready and blocked by your review since Tuesday.
#323

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwu-tow oh, I was not aware! Or I was but forgot. sorry for that!


/// Registry gathering callbacks. Each registered callback is assigned with a handle. Callback and
/// handle lifetimes are strictly connected. As soon a handle is dropped, the callback is removed
/// as well.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc is the same as for Registry, there should be information about difference between them

pub on_updated : RefCell<Option<Box<dyn Fn(&NodeData)>>>,
pub on_show : RefCell<Option<Box<dyn Fn()>>>,
pub on_hide : RefCell<Option<Box<dyn Fn()>>>,
pub on_show_with : RefCell<Option<Box<dyn Fn(&Scene)>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Describe when exactly the "on_show_with" and "on_hinde_with" is called.

// _ => Err(BadVariant)
// }
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented code

use ensogl::display::layout::alignment;
use ensogl::system::web;
use ensogl::control::callback;
use ensogl::gui::component::animation;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix import order

}

pub fn for_each_taken<F:Fn(Node)>(&self,f:F) {
self.take().into_iter().for_each(|(_,node)| { node.upgrade().for_each(|n| f(n)) })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may use WeakValueHashMap from weak_table (imported in prelude)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, not in this PR, but Ill take a look at it, thanks!

'WheelEvent',
'DomRect',
'AddEventListenerOptions'
]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline

Comment on lines 14 to 16
paste = "0.1"
derivative = "1.0.3"
shrinkwraprs = "0.3.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
paste = "0.1"
derivative = "1.0.3"
shrinkwraprs = "0.3.0"
paste = { version = "0.1" }
derivative = { version = "1.0.3" }
shrinkwraprs = { version = "0.3.0" }

and align closing }

@farmaazon farmaazon self-requested a review April 1, 2020 13:18
@wdanilo wdanilo marked this pull request as ready for review April 3, 2020 06:15
Comment on lines +135 to +136
/// Specialized version of `Registry` for arguments implementing `Copy`. Passing copy-able elements
/// as values is more performant than by reference.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you benchmarked it? I would say, that rust optimizer will handle such case anyway (because it know, that is "readonly refrence" and cannot be changed by other process, so it can replace reference with copy!

Comment on lines +26 to +27
pub use super::Object as TRAIT_Object;
pub use super::ObjectOps as TRAIT_ObjectOps;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment explaining why we're doing this should be rather here, not by the ObjectOps definition

@wdanilo wdanilo merged commit 169b3d1 into master Apr 3, 2020
@wdanilo wdanilo deleted the wip/wd/dev branch April 15, 2020 21:59
mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants