Grid-view based dropdown component #3985

Frizi · 2022-12-15T01:06:56Z

Pull Request Description

Implements https://www.pivotaltracker.com/n/projects/2539304/stories/184023380

Dropdown component. Planned to be used in nodes as a single and multiple selection widget, both for static and dynamically loaded values. Initial support is focused on static data, with limited support for dynamic sources. Notably, loading states are not supported yet. Full support for that is planned to be added later with widget lazy-loading.

Supports single and multiple selections.
Dedicated API for providing a static list of all entries.
Range-based query API for dynamically loading data as it is scrolled (only basic support - will need more work for proper async lazy-loading).
Internal entry cache and query batching to avoid querying data one by one (the batching for now is very basic, will have to be improved for proper lazy-loading).
Automatic dropdown width adjustment based on the entry label lengths, up to a set max allowed value.
Open and close animation.
Keyboard support for focusing and selecting entries.

Important Notes

Implementing the dropdown on top of grid-view have uncovered some assumptions around grid-view layers. It was assumed to always be a part of the component browser. Removing that assumption required a mechanism for propagating camera update information through layer tree. This is now implemented using a camera_parent layer field. Ideally each layer should simply have at most a single parent, and camera inheritance would follow that. That refactor turned out to be quite involved, so right now the simpler temporary solution is introduced in order to not delay this PR further.

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.

wdanilo

Really nice code, beautiful description and clear video! Thanks for it :)

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

lib/rust/ensogl/component/drop-down/src/entry.rs

wdanilo · 2022-12-16T07:53:40Z

lib/rust/ensogl/component/drop-down/src/entry.rs

+            data.label_thin.set_font <+ font;
+            data.label_bold.set_font <+ font;
+
+            natural_entry_width <- data.label_bold.width.map2(&text_offset, |w, offset| w + offset);


what is "natural" width?

Natural here refers to the width that the entry would like to take by itself, if no external limits were in place. It is based on text label width, with paddings applied.

I'm not sure what could be a more descriptive name, without being too verbose. Maybe desired_entry_width, but I'm not sure if that's better/easier to understand.

In such a case, just add a comment there. And yeah, desiredentry_width sounds better.

lib/rust/ensogl/component/drop-down/src/lib.rs

wdanilo · 2022-12-16T07:56:05Z

lib/rust/ensogl/component/drop-down/src/lib.rs

+            // === Keyboard navigation ===
+            model.grid.accept_selected_entry <+ input.toggle_focused_entry;
+            model.grid.move_selection_up <+ input.focus_previous_entry;
+            model.grid.move_selection_down <+ input.focus_next_entry;
+            model.grid.select_entry <+ model.grid.entry_hovered;
+
+            has_focused_entry <- model.grid.entry_selected.map(|entry| entry.is_some());
+            model.grid.select_entry <+ input.focus_previous_entry.gate_not(&has_focused_entry)
+                .map2(&visible_range, |_, range| Some(((range.end - 1).max(range.start), 0)));
+            model.grid.select_entry <+ input.focus_next_entry.gate_not(&has_focused_entry)
+                .map2(&visible_range, |_, range| Some((range.start, 0)));
+
+
+            // === Initialization ===


Please refactor these FRP inits to several init functions.

I tried to do that now, but it turns out to be massively verbose. Because the component::Frp trait doesn't use self value for initialization, each init function needs to receive network, private frp Api and model as separate arguments. Then, some sections use streams defined in previous sections (on purpose). Spitting it into multiple methods also means that I need to weave those streams through returns and extra params.

I think leaving it as a single FRP initialization block is much easier to follow and it is definitely less verbose.

I think that it's a mistake that component::Frp trait doesnt use self for initialization. We should fix it. I understand that we might not do it during this task, but on the other hand, this will become a big chunk of coede no one will ever touch again. I'm very afraid of such code chunks, we already have several of them. That's why I'd prefer to hve this refactored. we can do it in another PR, but let's at least agree that before drop-down widgets are finished, this will be refactored, ok?

I'm fine to change that. I'll need to understand the reasons behind original design of it, but in general I agree that it should change to allow easier code splitting. Let's do that as part of future widget PRs.

lib/rust/ensogl/component/drop-down/src/model.rs

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

app/gui/view/graph-editor/src/component/node/input/port.rs

wdanilo · 2022-12-20T11:53:08Z

lib/rust/ensogl/component/drop-down/src/entry.rs

+            data.label_thin.set_font <+ font;
+            data.label_bold.set_font <+ font;
+
+            natural_entry_width <- data.label_bold.width.map2(&text_offset, |w, offset| w + offset);


In such a case, just add a comment there. And yeah, desiredentry_width sounds better.

wdanilo · 2022-12-20T11:56:19Z

lib/rust/ensogl/component/drop-down/src/lib.rs

+            // === Keyboard navigation ===
+            model.grid.accept_selected_entry <+ input.toggle_focused_entry;
+            model.grid.move_selection_up <+ input.focus_previous_entry;
+            model.grid.move_selection_down <+ input.focus_next_entry;
+            model.grid.select_entry <+ model.grid.entry_hovered;
+
+            has_focused_entry <- model.grid.entry_selected.map(|entry| entry.is_some());
+            model.grid.select_entry <+ input.focus_previous_entry.gate_not(&has_focused_entry)
+                .map2(&visible_range, |_, range| Some(((range.end - 1).max(range.start), 0)));
+            model.grid.select_entry <+ input.focus_next_entry.gate_not(&has_focused_entry)
+                .map2(&visible_range, |_, range| Some((range.start, 0)));
+
+
+            // === Initialization ===


I think that it's a mistake that component::Frp trait doesnt use self for initialization. We should fix it. I understand that we might not do it during this task, but on the other hand, this will become a big chunk of coede no one will ever touch again. I'm very afraid of such code chunks, we already have several of them. That's why I'd prefer to hve this refactored. we can do it in another PR, but let's at least agree that before drop-down widgets are finished, this will be refactored, ok?

wdanilo · 2022-12-20T12:00:05Z

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

+        let garbage = &mut *self.garbage.borrow_mut();
+        garbage.before_pixel_update.append(&mut garbage.before_pixel_sync);
    }

    /// Pixel value retrieved (the point 3 in [`Collector`] docs).
    #[profile(Debug)]
    pub fn pixel_updated(&self) {
-        let mut garbage = self.garbage.borrow_mut();
-        let objects_being_moved = std::mem::take(&mut garbage.before_pixel_update);
-        garbage.before_mouse_events.extend(objects_being_moved);
+        let garbage = &mut *self.garbage.borrow_mut();
+        garbage.before_mouse_events.append(&mut garbage.before_pixel_update);
    }


Why these changes? The previous code was a little bit more verbose but also more explanatory. I'm OK with the shorter version, although I like the earlier one a little bit more. Before making this change, talk with the code author aobut it, with Adam.

There is a logic change. The old code was removing garbage.before_pixel_update, the new code is not. Why? There is no explanation in this PR why such change was introduced and what happened here.

Vec::append also does remove the items from source vector. This is how it is a move, not a clone. That way it is not a logic change.

The actual significant change here is that the vector is not reallocated every time a garbage collector is run. Instead, the vector capacity remains allocated and is ready to be reused for future garbage.

@farmaazon could you take a look at this part to verify that the changes are correct?

I'm ok with this change. I just wasn't aware of append method.

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

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

…les to demo scene

wdanilo · 2022-12-21T10:55:01Z

app/gui/view/component-browser/component-list-panel/grid/src/lib.rs

@@ -265,7 +265,7 @@ impl component::Model for Model {
        let enterable_elements = default();
        let colors = default();
        let requested_section_info = default();
-        let base_layer = &app.display.default_scene.layers.node_searcher_text;


Why this name change? Did the logic of this layer change?

This looked like a mistake. Using text layer for whole grid is surprising and different from what all demo scenes do. Individual sublayers created here are guaranteeing correct draw order anyway, including the dedicated text sublayer created in the grid-view.

With this change, node_searcher_text is only used by the old searcher, so it could be removed with it.

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

Frizi force-pushed the wip/frizi/dropdown-component-184023380 branch from 1563ca8 to 8c6e04d Compare December 15, 2022 13:11

Frizi marked this pull request as ready for review December 15, 2022 15:23

Frizi requested review from MichaelMauderer, wdanilo, farmaazon, mwu-tow and kazcw as code owners December 15, 2022 15:23

Frizi force-pushed the wip/frizi/dropdown-component-184023380 branch from 3648923 to 571b66a Compare December 15, 2022 19:48

wdanilo approved these changes Dec 16, 2022

View reviewed changes

Frizi force-pushed the wip/frizi/dropdown-component-184023380 branch 3 times, most recently from f8c255e to 04392c3 Compare December 20, 2022 11:13

wdanilo requested changes Dec 20, 2022

View reviewed changes

Frizi force-pushed the wip/frizi/dropdown-component-184023380 branch from 320d927 to 1d3029d Compare December 20, 2022 17:01

Frizi requested a review from wdanilo December 21, 2022 10:45

Frizi added 15 commits December 21, 2022 11:51

dropdown with correct scroll layers behaviour

c0475c3

internal dropdown entry cache and initial multiselect

f8d0e2a

remove bad import

d3590a4

New dropdown FRP api and static list support

93d33ba

adjust dropdown style to better match design

d69e822

fix deselection of dropdown values and add more cross-connected examp…

eb4ff64

…les to demo scene

add tag_values to suggestion entry and propagate it to node widgets

596c8e8

implement batch node and cleanup FRP network of dropdown

796ad19

isolate standalone dropdown component changeset

dc6bf70

automatic dropdown width change

4e009ca

fix format issues

b165a0e

fix clippy issues

67792fa

always calculate dropdown width based on bold text labels

855f214

remove leftover debug code

77d7eae

add changelog entry

a866c6d

Frizi added 15 commits December 21, 2022 11:51

handle keyboard input on empty selection

b55e9b2

adjust wasm size limit

29293d2

adjust wasm size limit

6112ca8

add missing docs and restrict unnecessarily public apis

7738433

remove unused type and fix cache size check

0a55998

remove unused "mask" hardcoded layer

4cf4b92

fix grid view masked layer visibility

b1e33fd

remove magic numbers and fix spacing

7b99768

only allow single layer parent and refactor camera inheritance

8934290

fix set_size after rebase

1e76c5e

limit code line width in dropdown frp init

9f917b7

fix dropdown min and max size after rebase, add more cases to demo scene

d961721

update according to review comments

389fc98

keep using for_each_sublayer, remove Group from docs.

cef13a0

adjust wasm size limit

4a670e8

Frizi force-pushed the wip/frizi/dropdown-component-184023380 branch from 3d84ef1 to 4a670e8 Compare December 21, 2022 10:52

wdanilo approved these changes Dec 21, 2022

View reviewed changes

Frizi added 4 commits December 21, 2022 12:56

correctly attach mask layer in complex-shape-system demo scene

427a89c

move LayerFlags definition before Layer

ecd1fc6

add scissor test back to complex-shape-system example

a2ba44f

fix rendering of shape instance children in focus_management scene

5c56247

Frizi added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Ready to merge This PR is eligible for automatic merge labels Dec 21, 2022

Update sprite parents in ShapeInstance::swap

8178808

Frizi added the CI: Ready to merge This PR is eligible for automatic merge label Dec 22, 2022

Merge branch 'develop' into wip/frizi/dropdown-component-184023380

7ace2dc

Frizi added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Dec 22, 2022

mergify bot merged commit 4042b5b into develop Dec 22, 2022

mergify bot deleted the wip/frizi/dropdown-component-184023380 branch December 22, 2022 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grid-view based dropdown component #3985

Grid-view based dropdown component #3985

Frizi commented Dec 15, 2022 •

edited

Loading

wdanilo left a comment

wdanilo Dec 16, 2022

Frizi Dec 19, 2022

wdanilo Dec 20, 2022

wdanilo Dec 16, 2022

Frizi Dec 19, 2022

wdanilo Dec 20, 2022

Frizi Dec 21, 2022

wdanilo Dec 20, 2022

wdanilo Dec 20, 2022

wdanilo Dec 20, 2022

Frizi Dec 20, 2022

Frizi Dec 20, 2022

farmaazon Dec 21, 2022

wdanilo Dec 21, 2022

Frizi Dec 21, 2022

Grid-view based dropdown component #3985

Grid-view based dropdown component #3985

Conversation

Frizi commented Dec 15, 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

Frizi commented Dec 15, 2022 •

edited

Loading