Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Widgets integrated with graph nodes #6347

Merged
merged 47 commits into from
Apr 26, 2023
Merged

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Apr 18, 2023

Pull Request Description

Rewrites node input component. Now the input is composed of multiple widget components arranged in a tree of views with automatic layout. That allows creating complex UI elements on top of the node itself, and further widget positions will be automatically adapted to that. The tree roughly follow the span tree, as it is built by consuming its nodes and eagerly creating widgets from them. The tree is rebuilt every time the expression changes, but that rebuild process reuses as much previously created widgets as possible, and only updates their configuration as needed. Each widget type can have its own configuration options that can be passed to it from the parent, or assigned based on configuration received from the language server.

image

Important Notes

For now, all span-tree updates are sent over to the shared Frp endpoint of the whole tree, so there is no mechanism for intercepting them by the parent widgets. One idea would be to use existing bubbling/capturing events on widget display objects for that purpose, but I think existing implementation is simpler and more convenient, and we can always easily change that if we have a use for it.

There are some issues with performance due to much more display objects being created on the graph. Expect it to be a little worse, especially at initialization time.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Frizi Frizi force-pushed the wip/frizi/widgets-on-node branch 3 times, most recently from c3aa3f2 to dc4fa59 Compare April 20, 2023 17:34
@Frizi Frizi marked this pull request as ready for review April 20, 2023 18:08
app/gui/language/span-tree/src/builder.rs Outdated Show resolved Hide resolved
@@ -93,14 +93,17 @@ impl<T: Payload> SpanTreeGenerator<T> for String {
#[derivative(Default(bound = ""))]
struct ChildGenerator<T> {
current_offset: ByteDiff,
sibling_offset: ByteDiff,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call note: to be probably deleted as, current_offset probably should be offset to sibling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that there is a good reason for storing offset to parent. The child method allows random access to any child by index, and it requires parent offset to easily calculate the child span's start. It is used everywhere, as it is the base of indexing the tree by crumbs. So we have to it. I will rename the fields and fix some inaccurate comments to make it a little better.

app/gui/language/span-tree/src/lib.rs Show resolved Hide resolved
app/gui/src/controller/graph/widget/metadata.rs Outdated Show resolved Hide resolved
app/gui/src/controller/graph/widget/metadata.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/instance.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/instance.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/object/instance.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/display/scene.rs Show resolved Hide resolved
lib/rust/ensogl/core/src/display/scene.rs Show resolved Hide resolved
@Frizi
Copy link
Contributor Author

Frizi commented Apr 21, 2023

Found regressions:

  • when creating new node, the node background is invisible until the user starts typing
  • when creating new node by dragging an edge, the connected edge is not aligned properly to the node center
  • when dragging an edge grabbed on target side, the cursor style is not updated

@Frizi Frizi force-pushed the wip/frizi/widgets-on-node branch from 4cd8266 to 35a4a3f Compare April 21, 2023 18:25
@vitvakatu
Copy link
Contributor

QA Report 🟢

I didn't spot any serious issues.

  1. Ridiculous issue I spotted when mindlessly moving connections.
2023-04-26.15-39-15.mp4
  1. Ctrl-clicking on nodes usually places the mouse cursor in a completely random place, I didn't spot any pattern there. It can go to the beginning of the code, to the end, a few characters to the side, or exactly to the place you clicked.
  2. Sometimes, it's impossible to connect to the specific element of the array. Simpler cases work fine, though.
2023-04-26.15-27-58.mp4

@MichaelMauderer
Copy link
Contributor

QA Report 🟢

No issue spotted that originated in this PR. #6221 still seems to be present.

@Frizi Frizi added CI: Ready to merge This PR is eligible for automatic merge CI: Keep up to date Automatically update this PR to the latest develop. labels Apr 26, 2023
@mergify mergify bot merged commit a00efb2 into develop Apr 26, 2023
@mergify mergify bot deleted the wip/frizi/widgets-on-node branch April 26, 2023 19:37
Procrat added a commit that referenced this pull request Apr 27, 2023
* develop:
  Passing events to sub-widgets in List Editor and refactoring of the slider component. (#6433)
  Revert "Cloud/desktop mode switcher (#6308)" (#6444)
  Widgets integrated with graph nodes (#6347)
  Table Visualization and display text changes. (#6382)
  Skip redundant compilations (#6436)
  Add parse extensions to Text type. #6330 (#6404)
  Cloud/desktop mode switcher (#6308)
  Replace Table should_equal with should_equal_verbose (#6405)
  Rollback event handling changes for the mouse (#6396)
  Dashboard directory interactivity (#6279)
  Ability to change the execution environment between design and live. (#6341)
  Implementation of elements adding to List Editor and a lot of internal API (#6390)
  Drop method exported from WASM + removing leaks. (#6365)
  Turn null into UnexpectedExpression when Union type is incomplete (#6415)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Keep up to date Automatically update this PR to the latest develop. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants