Component browser breadcrumbs #3686

vitvakatu · 2022-09-06T08:45:48Z

Pull Request Description

[ci no changelog needed]

This PR implements a Breadcrumbs panel for the new component browser.
The Breadcrumbs is a horizontal list of text labels separated by a special icon and has an optional ellipsis icon at the end.
It is implemented using the new GridView component.

Video:

Demo of adding new breadcrumbs, scrolling behavior, and selecting breadcrumbs with the mouse.

2022-09-08.21-53-56.mp4

Demo of selecting breadcrumbs with keyboard shortcuts:

2022-09-08.21-54-47.mp4

Important Notes

This PR implements an old interaction of the design of the component browser. The new design of the breadcrumbs can not be easily integrated into the current look of the component browser, so we would need to update styles later. It should be a relatively simple task. The implementation uses color from the new design though. (but not fonts and sizes)
I found a bug in the grid view implementation that causes panics at runtime in some conditions. The reason is triggering FRP endpoints while constructing new entries. This issue is fixed in the PR.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

…bs-182560265 # Conflicts: # app/gui/view/debug_scene/component-list-panel-view/src/lib.rs

This reverts commit f014f4b.

wdanilo

I really love the quality of the code here, amazing job. The final effect looks amazing as well! I am not approving this PR ONLY because I don't know this space so well as Adam, so he should review it as well and test it.

wdanilo · 2022-09-09T06:49:03Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+            let left = Circle(radius.clone()).fill(circles_color.clone());
+            let center = Circle(radius.clone()).fill(circles_color.clone());
+            let right = Circle(radius).fill(circles_color);


Instead of creating 3 circles, use the repeat function (our drawing API). It is WAY faster. Repeat adds 1 opcode to GPU code to repeat the same element infinite amount of times.

I like the idea and tried to implement it but faced an issue with the intersection method on shapes similar to https://www.pivotaltracker.com/story/show/182593513
I left a comment in the code to address it in the future.

This does not explain anything. You told that you faced issue "similar to", whcih doesnt describe what issue you were facing. The comment in the code only tells "its broken", not explaining what is happening there. We are using intersections in several places, it's worth understanding what happens here. Please describe it properly here + attach screenshots.

After your message, I checked again, with a slightly different code - and it worked. Now I can explain what happens. This code works:

let circles = Circle(...).repeat(...); let mask = Rect(...); let circles = circles.intersection(mask).fill(circles_color);

This one does not:

let circles = Circle(...).repeat(...).fill(circles_color); let mask = Rect(...); let circles = circles.intersection(mask);

The slight difference in placement of the fill breaks our renderer:

very interesting - please create an issue with these screenshots on pivotal tracker in the graph editor epic and assing to me.

app/gui/view/component-browser/breadcrumbs/src/entry.rs

wdanilo · 2022-09-09T06:51:17Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+    fn set_separator(&self) {
+        if self.state.get() != State::Separator {
+            self.remove_current();
+            self.display_object.add_child(&self.separator);
+            self.state.set(State::Separator);
+        }
+    }
+
+    fn set_ellipsis(&self) {
+        if self.state.get() != State::Ellipsis {


these names are not explanatory. What does Entry::set_separator mean? It sounds like I will be setting a separator in an entry. I mean, I know wht it means, but I feel that a better name can be found here (I dont know yet what name, but I'd be thankful if you think about it).

Renamed to switch_to_....

app/gui/view/component-browser/breadcrumbs/src/entry.rs

wdanilo · 2022-09-09T06:53:02Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+            eval text_size((s) data.set_default_text_size(*s));
+            is_disabled <- input.set_model.map(|m| matches!(m, Model::Separator | Model::Ellipsis));
+            width <- map2(&input.set_model, &text_offset,
+                f!([data](model, text_offset) {


you dont need [data] here. If the code starts with something. then something is automatically put there in f! macro.

wdanilo · 2022-09-09T06:54:11Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+/// If the selected entry is to the left of this fraction of the viewport, we scroll the breadcrumbs
+/// to keep the entry visible and easily reachable.
+const SCROLLING_THRESHOLD: f32 = 0.5;


I dont understand what it does - neither docs nor name really explains what is this number.

Expanded the documentation around it. Not sure if it is clear enough now.

wdanilo · 2022-09-09T06:54:40Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+/// A rectangular mask used to crop the breadcrumbs' content when it doesn't fit in the designated
+/// space.


tell me more! What is the "designated space"? Where this mask is really used?

Expanded documentation. However, I don't quite follow the where this mask is used part. It is used as stated - to crop the breadcrumb's contents.

wdanilo · 2022-09-09T06:55:45Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+        // Additional padding is added to avoid rare glitches when the last entry is
+        // cropped because it is placed right on the border of the viewport. Even 1px seems
+        // enough, but we add a bit more to be sure.
+        let padding = 10.0;


what is this padding for? The docs dont explain it - it is padding between what and what?

Adjusted comment. It is padding between the content and the right border of the viewport.

wdanilo · 2022-09-09T06:56:25Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+        } else if let Some(entry) = entries.borrow().get(col / 2) {
+            entry::Model::Text(entry.0.clone_ref())
+        } else {
+            tracing::error!("Requested entry is missing in the breadcrumbs ({col})");


prelude exports error! now. Don't use tracing::.

The prelude exports error! from our crate that accepts logger as the first argument. I believe I should use tracing instead - and I found multiple usages in other crates.

Sorry, I forgot that my PR intorduces it and I havent merged it yet :P

wdanilo · 2022-09-09T06:57:15Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+            eval selected_grid_col([model]((_row, col)) {
+                model.grey_out(Some(col + 1));
+            });


you dont need neither [model] here nor curly braces. Please clean similar places in other FRP places.

This is an artifact of the older code changes. In some cases, our macro does require both [model] and curly braces.

It should not. Could you please explain why it is needed here?

It is not needed here, but it was required in my previous versions of the code. I changed these lines.

app/gui/controller/engine-protocol/src/language_server/tests.rs

farmaazon · 2022-09-12T13:16:51Z

app/gui/view/component-browser/breadcrumbs/Cargo.toml

@@ -0,0 +1,12 @@
+[package]
+name = "ensogl-breadcrumbs"


The name suggests that this is a crate with general-purpose breadcrumbs component, but the location suggests that it is dedicated for IDE application.

app/gui/view/component-browser/breadcrumbs/src/entry.rs

app/gui/view/component-browser/breadcrumbs/src/lib.rs

lib/rust/ensogl/component/grid-view/src/entry/visible.rs

wdanilo · 2022-09-13T10:46:12Z

app/gui/controller/engine-protocol/src/language_server/tests.rs

+use crate::language_server::CapabilityRegistration;
+use crate::language_server::Client;
+use crate::language_server::ContentRoot;
+use crate::language_server::Event;
+use crate::language_server::ExpressionUpdatePayload;
+use crate::language_server::FileAttributes;
+use crate::language_server::FileEdit;
+use crate::language_server::FileEvent;
+use crate::language_server::FileEventKind;
+use crate::language_server::FileSystemObject;
+use crate::language_server::LocalCall;
+use crate::language_server::MethodPointer;
+use crate::language_server::Notification;
+use crate::language_server::Path;
+use crate::language_server::Position;
+use crate::language_server::RegisterOptions;
+use crate::language_server::Result;
+use crate::language_server::Sha3_224;
+use crate::language_server::StackItem;
+use crate::language_server::TextEdit;
+use crate::language_server::TextRange;
+use crate::language_server::Uuid;
+use crate::language_server::VisualisationConfiguration;
+use crate::language_server::API;


do we need to import so many things? We prefer qualified names, so maybe it would be better to use language_server::FileEdit directly instead of importing it? @farmaazon what do u think?

Discussed briefly with @wdanilo: let's make a mass import use::crate::language_server::*

@vitvakatu we agreed on star import ONLY because these are tests. Do not use star import in other, non-test modules please. Just FYI.

wdanilo · 2022-09-13T10:48:52Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+            let left = Circle(radius.clone()).fill(circles_color.clone());
+            let center = Circle(radius.clone()).fill(circles_color.clone());
+            let right = Circle(radius).fill(circles_color);


This does not explain anything. You told that you faced issue "similar to", whcih doesnt describe what issue you were facing. The comment in the code only tells "its broken", not explaining what is happening there. We are using intersections in several places, it's worth understanding what happens here. Please describe it properly here + attach screenshots.

wdanilo · 2022-09-13T10:51:30Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+
+// === EntryData ===
+
+/// An internal structure of [`Entry`], which may be passed to FRP network.


Does this doc tells that this structure may be passed to FRP, or that Entry can be passed to FRP? Please make sentences in docs more obvious (here in other places - please check where doc sentences can be shorter and make sure that the subject is not ambiguous).

If this tells that this object can be passed to FRP, the clone here is pretty costly - it has 6 or 7 Rcs to be cloned. In such a case, this sohudl be wrapped in one Rc.

Rephrased the documentation completely.

It is wrapped in a single Rc (see Entry struct implementation)

wdanilo · 2022-09-13T10:54:28Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+// === Params ===
+
+/// The parameters of Breadcrumbs' entries.
+#[allow(missing_docs)]


It states that docs are not needed, but in reality they are (or better field names):

I dont understand what is "margin".

What is greyed_out_start ? What is the usize there?

What is text_offset - offset to what?

What is font? Is it real font data or is it just font name? We are using "font" var name in many places to always indicate font, not font name.

Added docs

Added docs, replaced usize with Col (nice catch!)

text_padding_left

We use types, for this reason, don't we? For example, the DEFAULT_FONT constant is named as it is, but it should be named DEFAULT_FONT_NAME then. Anyway, I renamed it to font_name.

yup, its ould be DEFAULT_FONT_NAME. I believe I changed it in my newest PR (not merged yet).

wdanilo · 2022-09-13T10:55:16Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+
+// === Params ===
+
+/// The parameters of Breadcrumbs' entries.


I dont understand these docs -what are parameters of breadcrumbs?

Rephrased a little

wdanilo · 2022-09-13T10:57:35Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+        network: &frp::Network,
+        init: frp::Source<()>,
+    ) -> frp::Sampler<Self> {
+        frp::extend! { network
+            let margin = style.get_number(theme::entry::margin);
+            let hover_color = style.get_color(theme::entry::hover_color);
+            let font = style.get_text(theme::entry::font);
+            let text_offset = style.get_number(theme::entry::text_offset);
+            let text_size = style.get_number(theme::entry::text_size);
+            let selected_color = style.get_color(theme::entry::selected_color);
+            let highlight_corners_radius = style.get_number(theme::entry::highlight_corners_radius);
+            let greyed_out_color = style.get_color(theme::entry::greyed_out_color);
+            greyed_out_start <- init.constant(None);
+            text_params <- all4(&init, &text_offset,&text_size,&font);
+            colors <- all4(&init, &hover_color,&selected_color,&greyed_out_color);
+            params <- all_with6(&init,&margin,&text_params,&colors,&highlight_corners_radius,
+                &greyed_out_start,
+                |_,&margin,text_params,colors,&highlight_corners_radius,&greyed_out_start| {
+                    let (_, text_offset,text_size,font) = text_params;
+                    let (_, hover_color,selected_color,greyed_out_color) = colors;
+                    Params {
+                        margin,
+                        text_offset: *text_offset,
+                        text_size: text::Size::from(*text_size),
+                        hover_color:*hover_color,
+                        font: ImString::new(font),
+                        selected_color: *selected_color,
+                        highlight_corners_radius,


This code extends frp network not belonging to thsi struct. Unfortunately, due to lack of proper docs, I dont understand what this structure is for. Anyway, what network does it extend? Please remember that after you extend FRP network, you CAN'T remove this structure leving the FRP network - otherwise you will cause memory leak. Are you 100% sure that when this struct is dropped the network is dropped as well? If you are not sure, you can't do it and you'd need to use FRP Bridge Network (used for example in the nodes implementation), which will be automatically dropped OR create a local network in this struct. Anyway, this is not documented. If you are 100% sure that network will be droped with thsi struct, this needs to be extremely well documented and explained why it happens this way.

There should be no mistake here. To avoid confusion, I moved the function closer to its usage (to the root of the crate) and added docs. Keep in mind that the return value is Sampler<Self>, not Self, and the Sampler is later used in the network we extended here.

Good to know that. Still, it sohuld always be well documented because it's dangereous. In new FRP implementation we will disallow extend function completely. Thanks for moving it closer to the usage point and the docs.

wdanilo · 2022-09-13T10:58:25Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+            color <- all_with3(&transparent_color, &target_color, &appear_anim.value, mix);
+
+            contour <- all_with(&size, &margin, |size, margin| Contour {
+                size: *size - Vector2(*margin, *margin) * 2.0,


magic number 2.0 - what it is?

It means twice. The margins are placed on all sides of the entry - left, right, top and bottom. So we need to multiply the margin by 2 to reduce the size correctly. I don't believe such numbers can be considered magic.

wdanilo · 2022-09-13T10:59:00Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+            size <- input.set_size.on_change();
+            margin <- input.set_params.map(|p| p.margin).on_change();
+            hover_color <- input.set_params.map(|p| p.hover_color).on_change();
+            font <- input.set_params.map(|p| p.font.clone_ref()).on_change();
+            text_offset <- input.set_params.map(|p| p.text_offset).on_change();
+            text_color <- input.set_params.map(|p| p.selected_color).on_change();
+            text_size <- input.set_params.map(|p| p.text_size).on_change();
+            greyed_out_color <- input.set_params.map(|p| p.greyed_out_color).on_change();
+            greyed_out_from <- input.set_params.map(|p| p.greyed_out_start).on_change();
+            highlight_corners_radius <- input.set_params.map(|p| p.highlight_corners_radius).on_change();
+            transparent_color <- init.constant(color::Rgba::transparent());
+
+            col <- input.set_location._1();
+            should_grey_out <- all_with(&col, &greyed_out_from,
+                |col, from| from.map_or(false, |from| *col >= from)
+            );
+            color_anim.target <+ should_grey_out.map(|should| if *should { 1.0 } else { 0.0 });
+            target_color <- all_with3(&text_color, &greyed_out_color, &color_anim.value, mix);
+            appear_anim.target <+ init.constant(1.0);


very nice FRP names here - as I mentioned before, I like the FRP look in this PR!

wdanilo · 2022-09-13T11:05:44Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+use ensogl_core::Animation;
+use ensogl_grid_view as grid_view;
+use ensogl_grid_view::Viewport;
+use ensogl_hardcoded_theme::application::component_browser::searcher::list_panel::breadcrumbs as theme;


100 chars - this code is not linted. Please check why linter did not catch it and write here if it was a bad configuration or someting else, please.

I believe this is the intended behavior of the rustfmt. There is no clear way to reformat this code to fit into the max_width limit. See rust-lang/rustfmt#3863 and rust-lang/rustfmt#4746

Can you split it into two uses instead?

wdanilo · 2022-09-13T11:07:23Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+// =================
+
+/// We virtually divide the breadcrumbs into two regions – left and right. This constant
+/// determines the fraction of the left one.


The word "fraction" is not explained. What does fraction in this context mean?

I rephrased a little by using the word proportion instead.

wdanilo · 2022-09-13T11:09:03Z

app/gui/view/component-browser/breadcrumbs/src/lib.rs

+        if self.entries.borrow().is_empty() {
+            None
+        } else if self.show_ellipsis.get() {
+            Some(self.grid_columns().saturating_sub(3))


magic number

Fixed by using a variable

wdanilo

Very nice implementation, well done!

wdanilo · 2022-09-14T14:14:37Z

app/gui/view/component-browser/breadcrumbs/src/entry.rs

+        Self { display_object, state, text, ellipsis, separator }
+    }
+
+    fn remove_current(&self) {


what is "current"? Please use better name - is it currently selected one?

…bs-182560265

vitvakatu added 9 commits September 6, 2022 12:44

Implement bredcrumbs panel in the component browser

dce4f4c

Refactoring

43dc8e2

Formatting

2a8a7c2

Enso-formatter

0bb80e6

Docs & formatting

88269aa

Fix after merge

4304e03

Remove hover highlight

f0e7fd2

Grammar & content width fixes

85b73eb

vitvakatu self-assigned this Sep 6, 2022

Fix crates hierarchy

42e7ad3

vitvakatu marked this pull request as ready for review September 6, 2022 12:18

vitvakatu requested review from MichaelMauderer, wdanilo and farmaazon as code owners September 6, 2022 12:18

vitvakatu marked this pull request as draft September 6, 2022 12:55

vitvakatu added 10 commits September 7, 2022 12:40

Refactor entry to simplify code

dbfb2d7

Padding and color animation

70a715b

Animation for entries appearance

3962954

Smooth scrolling + removing unselected entries when pushing

ec947f5

Scrolling & colors adjustments

d1d5a30

Cleanup

347ed6e

Enso formatter

f014f4b

Revert "Enso formatter"

b0650a4

This reverts commit f014f4b.

Fix formatting

74fdefa

Enso formatter

0df958a

vitvakatu marked this pull request as ready for review September 8, 2022 18:36

vitvakatu requested review from mwu-tow, kazcw and JaroslavTulach as code owners September 8, 2022 18:36

wdanilo reviewed Sep 9, 2022

View reviewed changes

vitvakatu added 2 commits September 12, 2022 16:15

Addressing review comments

cce1726

Addressing review comments

9774074

farmaazon approved these changes Sep 12, 2022

View reviewed changes

vitvakatu added 3 commits September 13, 2022 13:09

Addressing review comments [wip]

c9d1acd

Addressing review comments

ec19ae0

Enso formatter pass

0b192fe

wdanilo requested changes Sep 13, 2022

View reviewed changes

vitvakatu added 3 commits September 13, 2022 22:02

Addressing review comments

a7cdde7

Addressing review comments

1a6bb95

Addressing review comments

a31c715

vitvakatu requested a review from wdanilo September 13, 2022 19:04

wdanilo approved these changes Sep 14, 2022

View reviewed changes

Addressing review comments

4832bd5

vitvakatu added the CI: Ready to merge label Sep 15, 2022

mergify bot and others added 9 commits September 15, 2022 08:46

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

3263c26

…bs-182560265

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

127163b

…bs-182560265

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

b357d65

…bs-182560265

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

e0aa703

…bs-182560265

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

0231309

…bs-182560265

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

388cb3a

…bs-182560265

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

f6bbe41

…bs-182560265

Merge branch 'develop' into wip/vitvakatu/component-browser-breadcrum…

f240767

…bs-182560265

Bump wasm size limit

dfa0ab5

mergify bot merged commit a771e40 into develop Sep 19, 2022

mergify bot deleted the wip/vitvakatu/component-browser-breadcrumbs-182560265 branch September 19, 2022 09:21

wdanilo mentioned this pull request Feb 6, 2023

Intersection of shapes doesn't work with shadows #5454

Closed

		/// A rectangular mask used to crop the breadcrumbs' content when it doesn't fit in the designated
		/// space.


		// === EntryData ===

		/// An internal structure of [`Entry`], which may be passed to FRP network.


		// === Params ===

		/// The parameters of Breadcrumbs' entries.

Component browser breadcrumbs #3686

Component browser breadcrumbs #3686

Conversation

vitvakatu commented Sep 6, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

wdanilo left a 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

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

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

wdanilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu commented Sep 6, 2022 •

edited

Loading