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

New node design #7311

Merged
merged 55 commits into from Jul 27, 2023
Merged

New node design #7311

merged 55 commits into from Jul 27, 2023

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Jul 17, 2023

Pull Request Description

Fixes #6552
Fixes #6910
Fixes #6872

Implementation of new node design. Includes many changes related to stylesheet update handling and per-style FRP construction, as well as refactoring of scene layers used by graph editor. Some additional components were migrated to use Rectangle shape and new mouse handling events. Fixed text rendering, where random thin lines appeared at the borders of glyph sprites. Refined edge layout to match new node sizes and not leave any visible gaps between line segments.

The node colors are currently randomly selected from predefined list. Later this will be improved to use group information from the suggestion database, once that is fully migrated to use the documentation tags, thus removing the dependency on the execution context.

new-node-design-with-args.mp4
image

Important Notes

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 added 27 commits June 22, 2023 12:54
- refactor more widgets to use theme instead of hardcoded constants
- add proper FromTheme trait and refactor derive
- add generic FromTheme style cache for widgets
- add layer blend modes
- remove special error indicator shape, temporarily use simple border around the node
- simplify FromTheme to only return the style sampler, since we no longer need to manually init it
- fix wrong usage of separator widget in infix expressions
- fix memory leaks on blank widget
- properly handle cursor position at transition from host layer state
- refactor scrollbars to use rectangle shapes
- move widget styling
@Frizi Frizi linked an issue Jul 19, 2023 that may be closed by this pull request
2 tasks
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

First round of comments

app/gui/language/ast/impl/src/lib.rs Show resolved Hide resolved
app/gui/language/span-tree/src/node/kind.rs Outdated Show resolved Hide resolved
Comment on lines +705 to +706
let group_colors_theme = GroupColorsTheme::from_theme(network, style_frp);
group_colors <- group_colors_theme.map(|t| GroupColors::from(*t));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why have you needed it, but I like it.

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 changed it when I tried to use actual group colors for nodes. Didn't used it in the end due to group querying complications, but I kept the nice parts of it anyway.

@@ -0,0 +1,1300 @@
//! All icons that are used in the Component Browser.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this file is just copied from old icon.rs.

And this set could be the general "GUI icons" crate, to use in not only component browser, but let's keep this refactoring for later.

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 needed it to be in a separate crate that can also be imported in the graph editor without pulling in too much stuff. I agree that moving it higher would be good, but it definitely should be done separately.

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
Comment on lines +1025 to +1027
/// NOTE: When using an inverted mask, the mask layer's own blend mode is ignored. Instead,
/// the mask layer is always rendered with the blend mode [`BlendMode::ALPHA_CUTOUT`], carving
/// out the shape of the mask from the layer it masks.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess not inverted mask also ignores own blend mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't ignore it. You can draw on the non-inverted mask layer with any blend mode. It's just the final mask composite that will always multiply the alpha channels. Though I believe we could eventually find a way to do masking purely with blend modes, avoiding the need for mask framebuffer completely (as I already do with inverted mask).

@Frizi Frizi force-pushed the wip/frizi/new-node-design branch from 67d52fb to 90f8e2e Compare July 25, 2023 13:10
@Frizi
Copy link
Contributor Author

Frizi commented Jul 25, 2023

For some reason vscode did something weird with the merge and by mistake I pushed the merged commits as part of this branch linear history. This is now fixed, but that's why a lot of people were again incorrectly pinged for review. Sorry about that.

@farmaazon
Copy link
Contributor

QA Report: 🟥

Some issues found, still testing (haven't reached list editor yet ;) )

  1. I do not see drop-down for the node below. I could pick a filename before. Is this expected change?

image

  1. Moving the mouse from node to toolbar, I see the icons disappear and appear again. Not always, but there's a path where it happens, and I hit it fairly often.
new-design-2023-07-25_14.00.52.mp4
  1. Also, it's a bit unintuitive that I need to hover the node first, and only then move the mouse left to see the icons. Just approaching from the right side does not make the icons visible. Is that by design?

  2. When connecting to an input port inside port with drop down, the drop-down is shown.

new-design-2023-07-25_14.14.15.mp4

@Frizi
Copy link
Contributor Author

Frizi commented Jul 26, 2023

Also, it's a bit unintuitive that I need to hover the node first, and only then move the mouse left to see the icons. Just approaching from the right side does not make the icons visible. Is that by design?

Yes, I've done that on purpose to not block interactions with widgets from nodes that are next to each other. Though I think instead of fully blocking the interaction, we could switch the action bar hover area layer to be below nodes until hovered. That way we can have both.

Fixed issues found during QA, the PR is ready for another round.
image

@farmaazon farmaazon mentioned this pull request Jul 27, 2023
5 tasks
@farmaazon
Copy link
Contributor

QA Report: 🟢

I only have one more comment, which was part of first round, but I posted it in the wrong PR:

  1. The area for adding elements in list editor is still hardly discoverable. It was reported, and it was planned to fix in the new design. I assume this is planned for another task?

@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 Jul 27, 2023
@Frizi
Copy link
Contributor Author

Frizi commented Jul 27, 2023

The area for adding elements in list editor is still hardly discoverable. It was reported, and it was planned to fix in the new design. I assume this is planned for another task?

Yes, I haven't modified the list editor widget in any significant way here. Its new design needs to be implemented in separate task.

Comment on lines +154 to +157
SuperUse,
SuperUseStar,
SuperPubUse,
SuperPubUseStar,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have the test code updated to cover this.

@mergify mergify bot merged commit bb39eeb into develop Jul 27, 2023
23 of 24 checks passed
@mergify mergify bot deleted the wip/frizi/new-node-design branch July 27, 2023 13:00
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
4 participants