From 698a6a06744034e65c5f08be5b299ad03836839c Mon Sep 17 00:00:00 2001 From: Ilya Bogdanov Date: Wed, 10 Aug 2022 02:03:54 +0300 Subject: [PATCH] Left align component browser to the edited node (#3636) [ci no changelog needed] [Task link](https://www.pivotaltracker.com/story/show/181870555) This PR changes the relative position of the edited node in such a way that it is left-aligned to the component browser window. This change reflects the most recent version of the [design doc](https://github.com/enso-org/design/blob/main/epics/component-browser/design.md#overview) Screenshot 2022-08-08 at 19 15 47 As an additional change, the FRP implementation of the `Camera2d` was extended with a new output (screen dimensions) and fixed. With the old implementation, there was a possibility of panic at runtime because of non-exclusive borrows of `RefCell`. The FRP event for camera position was emited inside the scope with a mutable `RefCell` borrow. Any attempt to borrow the camera one more time (e.g., by calling one of the getters, such as `zoom()`) caused panic at runtime. --- app/gui/view/src/component_browser.rs | 25 ++++++-- .../core/src/display/camera/camera2d.rs | 64 +++++++++++++++++-- 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/app/gui/view/src/component_browser.rs b/app/gui/view/src/component_browser.rs index 8188da6620..79a5c2039c 100644 --- a/app/gui/view/src/component_browser.rs +++ b/app/gui/view/src/component_browser.rs @@ -7,6 +7,7 @@ use crate::prelude::*; use crate::component_browser::component_group::prelude::StyleWatchFrp; use crate::documentation; +use crate::graph_editor::component::node::HEIGHT as NODE_HEIGHT; use enso_frp as frp; use ensogl::application::frp::API; @@ -51,6 +52,22 @@ impl component::Model for Model { } } +impl Model { + fn expression_input_position( + panel_size: &Vector2, + panel_x: &f32, + vertical_gap: &f32, + ) -> Vector2 { + let half_width = panel_size.x / 2.0; + let half_node_height = NODE_HEIGHT / 2.0; + let panel_left = panel_x - half_width; + let panel_bottom = -panel_size.y / 2.0; + let x = panel_left; + let y = panel_bottom - vertical_gap - half_node_height; + Vector2(x, y) + } +} + impl display::Object for Model { fn display_object(&self) -> &display::object::Instance { &self.display_object @@ -76,7 +93,6 @@ impl component::Frp for Frp { model: &Model, style: &StyleWatchFrp, ) { - use crate::graph_editor::component::node::HEIGHT as NODE_HEIGHT; let network = &frp_api.network; let input = &frp_api.input; let out = &frp_api.output; @@ -106,14 +122,11 @@ impl component::Frp for Frp { model.list.input.shown <+ any(&input.show, &init); out.is_visible <+ bool(&input.hide, &input.show); out.size <+ size; - out.expression_input_position <+ all_with4( + out.expression_input_position <+ all_with3( &list_panel.size, &list_position_x, - &size, &gap, - |list_size, list_x, size, gap| { - Vector2(*list_x - list_size.x / 6.0, - size.y / 2.0 - gap - NODE_HEIGHT / 2.0) - } + Model::expression_input_position ); } init.emit(()); diff --git a/lib/rust/ensogl/core/src/display/camera/camera2d.rs b/lib/rust/ensogl/core/src/display/camera/camera2d.rs index 299c53d479..850593454c 100644 --- a/lib/rust/ensogl/core/src/display/camera/camera2d.rs +++ b/lib/rust/ensogl/core/src/display/camera/camera2d.rs @@ -167,6 +167,8 @@ pub struct Frp { pub position: frp::Source>, /// Camera zoom factor. pub zoom: frp::Source, + /// Camera's frustum screen dimensions. + pub screen: frp::Source, } /// Function used to return the updated screen dimensions. @@ -213,8 +215,9 @@ impl Camera2dData { frp::extend! { network frp_position <- source(); frp_zoom <- source(); + frp_screen <- source(); } - let frp = Frp { network, position: frp_position, zoom: frp_zoom }; + let frp = Frp { network, position: frp_position, zoom: frp_zoom, screen: frp_screen }; Self { frp, display_object, @@ -293,8 +296,6 @@ impl Camera2dData { self.matrix.view_projection = self.matrix.projection * self.matrix.view; let zoom = self.zoom; self.zoom_update_registry.run_all(zoom); - self.frp.position.emit(self.display_object.position()); - self.frp.zoom.emit(zoom); } changed } @@ -415,7 +416,12 @@ impl Camera2d { impl Camera2d { /// Sets screen dimensions. pub fn set_screen(&self, width: f32, height: f32) { - self.data.borrow_mut().set_screen(width, height) + let (endpoint, screen) = { + let mut borrowed = self.data.borrow_mut(); + borrowed.set_screen(width, height); + (borrowed.frp.screen.clone_ref(), borrowed.screen) + }; + endpoint.emit(screen); } /// Resets the zoom of the camera to the 1.0 value. @@ -425,7 +431,18 @@ impl Camera2d { /// Update all dirty camera parameters and compute updated view-projection matrix. pub fn update(&self, scene: &Scene) -> bool { - self.data.borrow_mut().update(scene) + let (is_updated, frp, zoom) = { + let mut borrowed = self.data.borrow_mut(); + let is_updated = borrowed.update(scene); + let frp = borrowed.frp.clone_ref(); + let zoom = borrowed.zoom; + (is_updated, frp, zoom) + }; + if is_updated { + frp.position.emit(self.display_object.position()); + frp.zoom.emit(zoom); + } + is_updated } // FIXME: This can fail, for example, when during calling the callback another callback is @@ -525,3 +542,40 @@ impl display::Object for Camera2d { &self.display_object } } + + + +// ============= +// === Tests === +// ============= + +#[cfg(test)] +mod tests { + use super::*; + + /// A regression test checks whether handling the camera's FRP events does not cause panics + /// at runtime. + /// + /// If the events are emitted with the camera's internal [`RefCell`] lock held, the usage of + /// camera API methods in event handlers can cause a panic. We need to check methods that use + /// both immutable and mutable borrows because, depending on the implementation of + /// the camera, it can hold either lock variant while emitting the event. + #[test] + fn test_frp_endpoints_are_not_causing_refcell_locks() { + let app = crate::application::Application::new("root"); + let camera = app.display.default_scene.camera(); + let frp = camera.frp(); + let network = frp::Network::new("TestCamera2d"); + frp::extend! { network + // `zoom()` method uses immutable borrow under the hood. + dummy <- frp.position.map(f_!(camera.zoom())); + // `set_position` method uses mutable borrow. + eval_ dummy(camera.set_position(default())); + // `screen` output is fired from the method that uses mutable borrow. + eval_ frp.screen(camera.zoom()); + eval_ frp.screen(camera.set_position(default())); + } + camera.set_position(Vector3(1.0, 2.0, 3.0)); + camera.update(&app.display.default_scene); + } +}