-
Notifications
You must be signed in to change notification settings - Fork 323
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
New node design #7311
Conversation
- 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
There was a problem hiding this 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/view/component-browser/component-list-panel/grid/src/entry/style.rs
Outdated
Show resolved
Hide resolved
let group_colors_theme = GroupColorsTheme::from_theme(network, style_frp); | ||
group_colors <- group_colors_theme.map(|t| GroupColors::from(*t)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
/// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
67d52fb
to
90f8e2e
Compare
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. |
QA Report: 🟥Some issues found, still testing (haven't reached list editor yet ;) )
new-design-2023-07-25_14.00.52.mp4
new-design-2023-07-25_14.14.15.mp4 |
QA Report: 🟢I only have one more comment, which was part of first round, but I posted it in the wrong PR:
|
Yes, I haven't modified the list editor widget in any significant way here. Its new design needs to be implemented in separate task. |
SuperUse, | ||
SuperUseStar, | ||
SuperPubUse, | ||
SuperPubUseStar, |
There was a problem hiding this comment.
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.
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
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.