-
Notifications
You must be signed in to change notification settings - Fork 34
Conversation
src/rust/ide/view/src/project.rs
Outdated
/// Ads a new node below `parent` and returns its ID. If there is not enough space right below | ||
/// `parent` then the new node is moved to the right to first gap that is large enough. | ||
pub fn add_node_below(&self, parent:NodeId) -> NodeId { | ||
let parent_pos = self.graph_editor.model.get_node_position(parent).unwrap_or_default(); | ||
let styles = StyleWatch::new(&self.app.display.scene().style_sheet); | ||
use ensogl_theme::project as theme; | ||
let x_gap = styles.get_number(theme::default_x_gap_between_nodes); | ||
let y_gap = styles.get_number(theme::default_y_gap_between_nodes); | ||
let y_offset = y_gap + node::HEIGHT; | ||
let mut x = parent_pos.x; | ||
let y = parent_pos.y - y_offset; | ||
// Push x to the right until we find a position where we have enough space for the new | ||
// node, including a margin of size `x_gap`/`y_gap` on all sides. | ||
{ | ||
let nodes = self.graph_editor.model.nodes.all.raw.borrow(); | ||
// We subtract `f32::EPSILON` to be sure that we do not include the selected node. | ||
let maybe_overlapping = nodes.values().filter(|node| | ||
(node.position().y - y).abs() < y_offset - f32::EPSILON); | ||
let maybe_overlapping = maybe_overlapping.sorted_by_key(|n| | ||
OrderedFloat(n.position().x)); | ||
// This is how much horizontal space we are looking for. | ||
let assumed_width = styles.get_number(theme::assumed_node_width); | ||
for node in maybe_overlapping { | ||
let node_left = node.position().x - x_gap; | ||
let node_right = node.position().x + node.view.model.width() + x_gap; | ||
if x + assumed_width > node_left { | ||
x = x.max(node_right); | ||
} else { | ||
// Since `maybe_overlapping` is sorted, we know that the if-condition will | ||
// be false for all following `node`s as well. Therefore, we can skip the | ||
// remaining iterations. | ||
break; | ||
} | ||
} | ||
} | ||
let pos = Vector2(x,y); | ||
self.graph_editor.add_node.emit(()); | ||
let node_id = self.graph_editor.frp.output.node_added.value(); | ||
self.graph_editor.set_node_position((node_id,pos)); | ||
node_id | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is not a function of graph editor? Graph editor is responsible for creating new nodes, it should be its responsibility to find the place as well IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left it here because the position was also computed here before (It was much simpler at that time). I suppose it could easily be defined as a method on GraphEditor
. I will try that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that default_y_gap_between_nodes
is defined in the project
part of the theme as well. Should that also be moved to the graph-editor part then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it seems so
src/rust/ide/view/src/project.rs
Outdated
@@ -325,8 +361,8 @@ mod js { | |||
#[allow(missing_docs)] | |||
#[derive(Clone,CloneRef,Debug)] | |||
pub struct View { | |||
model : Model, | |||
pub frp : Frp, | |||
pub model : Model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making model pub is rather a bad practice. We should rather not do it. Why is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_node_below
needs to be defined as a method on Model
, such that we can easily use it from the FRP network. But we also need the method to demonstrate the auto-layout in the demo scene. The model needs to be public for that.
src/rust/ide/view/src/project.rs
Outdated
pub fn add_node_below(&self, parent:NodeId) -> NodeId { | ||
let parent_pos = self.graph_editor.model.get_node_position(parent).unwrap_or_default(); | ||
let styles = StyleWatch::new(&self.app.display.scene().style_sheet); | ||
use ensogl_theme::project as theme; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line inbetween var defs looks strange
src/rust/ide/view/src/project.rs
Outdated
// node, including a margin of size `x_gap`/`y_gap` on all sides. | ||
{ | ||
let nodes = self.graph_editor.model.nodes.all.raw.borrow(); | ||
// We subtract `f32::EPSILON` to be sure that we do not include the selected node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand neither this comment nor this line of code. What would happen if we do not subtract it? Please describe it more clear in the docs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What could happen is that rounding errors lead us to consider the parent root as overlapping, because lies exactly on the threshold where we should not count it.
src/rust/ide/view/src/project.rs
Outdated
let maybe_overlapping = maybe_overlapping.sorted_by_key(|n| | ||
OrderedFloat(n.position().x)); | ||
// This is how much horizontal space we are looking for. | ||
let assumed_width = styles.get_number(theme::assumed_node_width); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that theme:: assumed_node_width
is confusing. The name should be more explicit, like "minimal_x_spacing_for_a_new_node" or something like that
src/rust/ide/view/src/project.rs
Outdated
for node in maybe_overlapping { | ||
let node_left = node.position().x - x_gap; | ||
let node_right = node.position().x + node.view.model.width() + x_gap; | ||
if x + assumed_width > node_left { | ||
x = x.max(node_right); | ||
} else { | ||
// Since `maybe_overlapping` is sorted, we know that the if-condition will | ||
// be false for all following `node`s as well. Therefore, we can skip the | ||
// remaining iterations. | ||
break; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rework the loop to use e.g. while loop and not use break
? We try to avoid break
always when possible to make the function/block exit in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would prefer to just remove the break
if that is fine. It has a minimal performance impact, but keeps the code more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, keeping performance optimal is alwasy better. So its better to keep it, but its even more nicer to use while
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you have a concrete suggestion to implement it with a while loop, I will leave it like this, because I don't see how I could do it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are almost there! Let's fix these small perf things, and lets merge it :)
pub fn add_node_below(&self, parent:NodeId) -> NodeId { | ||
use ensogl_theme::graph_editor as theme; | ||
let parent_pos = self.model.get_node_position(parent).unwrap_or_default(); | ||
let styles = StyleWatch::new(&self.model.app.display.scene().style_sheet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slow. Style watch should be created once and remembered instead.
let x_gap = styles.get_number(theme::default_x_gap_between_nodes); | ||
let y_gap = styles.get_number(theme::default_y_gap_between_nodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slow as well - this should be read in FRP by using StyleWatchFRP instead. Now, we are using a lot of string queries in hashmaps everytime we create a new node. When using FRP, this will be done only when theme changes.
// `y_offset` is exactly the distance between `parent` and the new node. At this | ||
// distance, `parent` should not count as overlapping with the new node. But we might | ||
// get this wrong in the presence of rounding errors. To avoid this, we use | ||
// `f32::EPSILON` as an error margin. | ||
let maybe_overlapping = nodes.values().filter(|node| | ||
(node.position().y - y).abs() < y_offset - f32::EPSILON); | ||
let maybe_overlapping = maybe_overlapping.sorted_by_key(|n| | ||
OrderedFloat(n.position().x)); | ||
// This is how much horizontal space we are looking for. | ||
let min_spacing = styles.get_number(theme::minimal_x_spacing_for_new_nodes); | ||
for node in maybe_overlapping { | ||
let node_left = node.position().x - x_gap; | ||
let node_right = node.position().x + node.view.model.width() + x_gap; | ||
if x + min_spacing > node_left { | ||
x = x.max(node_right); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing job with the comments!!! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with the code if you apply the @wdanilo comments.
|
||
/// Ads a new node below `parent` and returns its ID. If there is not enough space right below | ||
/// `parent` then the new node is moved to the right to first gap that is large enough. | ||
pub fn add_node_below(&self, parent:NodeId) -> NodeId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename parent
to something else, maybe above
? Because parent in this place may suggest rather some display objects hierarchy.
@s9ferech What is the status of this? |
Waiting for re-review by Wojciech. |
Pull Request Description
This PR implements the auto-layout described in issue #1550.
New nodes are pushed to the right into the first gap that is large enough:
If there isn't enough spacing then it will be pushed further:
The "interface" debug scene was updated to contain a "foo", "bar" and "baz" node that demonstrate the new auto-layout:
Important Notes
Checklist
Please include the following checklist in your PR:
CHANGELOG.md
was updated with the changes introduced in this PR.