Fullscreen Visualization #1355

farmaazon · 2021-03-22T11:12:50Z

Pull Request Description

This PR enables again the shortcut for FS visualization. It also contains a lot of fixes in visualizations

Important Notes

Scatterplot and histogram are redrawn after each resize.
Most visualizations don't keep zoom and pan after going fullscreen. Fixing that is planned for https://github.com/enso-org/ide/issues/1354

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.

wdanilo · 2021-03-23T08:57:08Z

src/js/lib/content/src/style.css

 .visualization {
    z-index: 2;
    border-radius: 14px;
+    /* visualizations are put into the fullscreen-vis layer which has pointer-events set to none, so we need to
+       override it */
+    pointer-events: auto;
 }


is the visualization class attached to fullscreen visualizations only? If so, it has a bad name.

no, but is attached to the div element we move to the fullscreen-vis. I will change the comment to "may be put"

wdanilo · 2021-03-23T08:59:40Z

src/rust/ensogl/lib/core/src/display/object/class.rs

+                        if !child.has_visible_parent() {
                            child.set_vis_false(host);


Wouldnt this change call "set_vis_false" on a child that was already hidden sometimes? For example, if a hidden child was moved to a visible parent, which just got hidden the next frame? If this could happen, this is blocker from merging. I'd love to chat why it never happens (if it never happens). We should add tests about it.

Yes, it may happen. But if the visible parent got hidden, then it will call set_vis_false, and thus the moved child will be also hidden.

So this is bad. set_vis_false sohuld be guaranteed to be called only once and never be called on a thing already hidden. Otherwise it can break some rendering assumptions. The same with showing things.

but set_vis_false checks if the instance is visible. I will add a test to make sure that on_hide callback is not called redundantly.

wdanilo · 2021-03-24T08:26:53Z

src/rust/ensogl/lib/core/src/display/scene.rs

+    /// Back DOM with fullscreen visualization. Kept separately from `back`, because use different
+    /// Camera


this has several problems:

It's not gramatically correct ('use')

does not have dot at the end.

States that this is a "back DOM", which is not specific enough, as this is a back DOM scene.

Does not explain what a "different camera" is and why do we use a different camera here. It should be explained here soduring future refactoring it would be clear what it is about.

wdanilo · 2021-03-24T08:28:44Z

src/rust/ensogl/lib/core/src/display/scene.rs

        canvas.set_style_or_warn("pointer-events", "none"    , &logger);
        front.dom.set_class_name("front");
        front.dom.set_style_or_warn("z-index", "1", &logger);
        back.dom.set_class_name("back");
        back.dom.set_style_or_warn("pointer-events", "auto", &logger);
        back.dom.set_style_or_warn("z-index"       , "0"   , &logger);
+        fullscreen_vis.dom.set_class_name("fullscreen_vis");
+        // fullscreen_vis.dom.set_style_or_warn("pointer-events", "auto", &logger);


commented code

wdanilo · 2021-03-24T08:29:16Z

src/rust/ensogl/lib/core/src/display/scene.rs

@@ -614,7 +622,7 @@ pub struct HardcodedLayers {

    pub viz_fullscreen : Layer,
    pub breadcrumbs    : Layer,
-    layers             : Layers,
+    layers             : Layers,    


You left spaces on the end of the line, which is strange, as IDEs should trim it on save

wdanilo · 2021-03-24T08:34:45Z

src/rust/ide/view/graph-editor/src/component/visualization/container/fullscreen.rs

+    pub fn new(logger:&Logger, scene:&Scene) -> Self {
+        let logger         = Logger::sub(logger,"fullscreen_view");
+        let display_object = display::object::Instance::new(&logger);
+        // scene.layers.viz_fullscreen.add_exclusive(&display_object);


commented code

wdanilo · 2021-03-24T08:35:30Z

src/rust/ide/view/graph-editor/src/component/visualization/container/fullscreen.rs

+/// View of the visualization container meant to be used in fullscreen mode. Its components are
+/// rendered on top-level layers of the stage.


I dont think this is strictly true. Components of full screen visualization panel, like HTML visualizations will not be rendered on top-level layers of the stage, will they be?

wdanilo · 2021-03-24T08:35:49Z

src/rust/ide/view/graph-editor/src/component/visualization/container/fullscreen.rs

+        let styles   = StyleWatch::new(&scene.style_sheet);
+        let bg_color = styles.get_color(ensogl_theme::graph_editor::visualization::background);
+        let bg_color = color::Rgba::from(bg_color);
+        let bg_hex   = format!("rgba({},{},{},{})",bg_color.red*255.0,bg_color.green*255.0,bg_color.blue*255.0,bg_color.alpha);


too long line

wdanilo · 2021-03-24T08:36:10Z

src/rust/ide/view/graph-editor/src/component/visualization/container/fullscreen.rs

+    fn display_object(&self) -> &display::object::Instance {
+        &self.display_object
+    }
+}


wdanilo · 2021-03-24T08:39:49Z

src/rust/ide/view/src/project.rs

+    fn show_fullscreen_visualization(&self, node_id:NodeId) {
+        let node = self.graph_editor.model.model.nodes.all.get_cloned_ref(&node_id);
+        if let Some(node) = node {
+            let visualization = node.view.model.visualization.fullscreen_visualization().clone_ref();
+            self.display_object.remove_child(&self.graph_editor);
+            self.display_object.add_child(&visualization);
+            *self.fullscreen_vis.borrow_mut() = Some(visualization);
+        }
+    }
+
+    fn hide_fullscreen_visualization(&self) {
+        if let Some(visualization) = std::mem::take(&mut *self.fullscreen_vis.borrow_mut()) {
+            self.display_object.remove_child(&visualization);
+            self.display_object.add_child(&self.graph_editor);
+        }
+    }


these should have docs of what hacks they are doing and why do we do the hacks with a link to issue which will provide correct solution (the scene cutting one)

Also, does this work in debug-scene? It is implemented in such a module that I'm not sure it works there.

It works in interface.

…vis2

Original commit: enso-org/ide@ebf5c4f

farmaazon added 8 commits March 22, 2021 09:46

Going fullscreen works

3c0cb5d

Hiding visualization

1d2f84f

WIP

42f6b60

WIP

f616362

Fixing visualizations WIP

f254ffe

Fixes

876e13c

Remove all traces

562ef28

Remove debug shortcut

6fbeae8

farmaazon requested a review from wdanilo March 22, 2021 11:12

farmaazon self-assigned this Mar 22, 2021

Fix linter + changelog

f049c43

wdanilo suggested changes Mar 24, 2021

View reviewed changes

farmaazon added 2 commits March 24, 2021 11:11

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

6df2c3f

…vis2

Apply @wdanilo review

5e3211f

farmaazon requested a review from wdanilo March 24, 2021 10:59

prettify CHANGELOG

5765adb

wdanilo approved these changes Mar 25, 2021

View reviewed changes

farmaazon added 5 commits March 30, 2021 09:13

Add tests for callback calls.

9ac575d

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

3c2139d

…vis2

Fix linter

1bd1b62

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

600cfc8

…vis2

Fixes after merge

92c69ba

farmaazon merged commit ebf5c4f into develop Mar 30, 2021

farmaazon deleted the wip/ao/fullscreen-vis2 branch March 30, 2021 10:43

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

Fullscreen Visualization (enso-org/ide#1355)

3934cca

Original commit: enso-org/ide@ebf5c4f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fullscreen Visualization #1355

Fullscreen Visualization #1355

farmaazon commented Mar 22, 2021 •

edited

wdanilo Mar 23, 2021

farmaazon Mar 24, 2021

wdanilo Mar 23, 2021

farmaazon Mar 24, 2021

wdanilo Mar 25, 2021

farmaazon Mar 25, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

farmaazon Mar 24, 2021 •

edited

		/// Back DOM with fullscreen visualization. Kept separately from `back`, because use different
		/// Camera

		/// View of the visualization container meant to be used in fullscreen mode. Its components are
		/// rendered on top-level layers of the stage.

Fullscreen Visualization #1355

Fullscreen Visualization #1355

Conversation

farmaazon commented Mar 22, 2021 • edited

Pull Request Description

Important Notes

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

farmaazon Mar 24, 2021 • edited

Choose a reason for hiding this comment

farmaazon commented Mar 22, 2021 •

edited

farmaazon Mar 24, 2021 •

edited