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

Allow viewer co customize egui style for the individual node UI #52

Merged
merged 3 commits into from
Feb 6, 2025

Conversation

juh9870
Copy link
Contributor

@juh9870 juh9870 commented Jan 11, 2025

This would allow the viewer to tweak ui style of all node elements, instead of tweaking each one individually.

In my app, I use this to lock the node to light or dark mode, or edit their full style

@zakarumych
Copy link
Owner

Would it be possible to pass less than whole Ui into the method?
Note that taking &mut Style from Ui may require allocation, so should be avoided if no modifications are intended.

@juh9870
Copy link
Contributor Author

juh9870 commented Jan 13, 2025

Passing the whole UI is done precisely to avoid allocation. The default implementation of the method does nothing with the UI and doesn't allocate anything, but the downstream implementation may use this method to call ui.style_mut() to allocate and edit customs style.

An example in my app: https://github.com/juh9870/dbe/blob/382411ab25a7b676312c3e6613eeab74a786a5e3/dbe_ui/src/workspace/graph.rs#L104-L117

I use an external crate to modify the style of the passed UI.

image

@zakarumych
Copy link
Owner

I would like to avoid giving Ui so that user won't be able to modify anything but Style.
Passing impl DerefMut<Target = Style> comes to mind, but it looks heavy for the reader.

@juh9870
Copy link
Contributor Author

juh9870 commented Jan 13, 2025

You can also go the has_node_style and apply_node_style functions route

@zakarumych
Copy link
Owner

I don't like it, but it would be consistent with the rest of the API.

Done to be consistent with the existing has_body and has_footer, and to remove the exposure of ui in the style method, while still keeping unnecessary allocations away.
@juh9870
Copy link
Contributor Author

juh9870 commented Jan 14, 2025

Sorry for force pushes.

Changed to use has_node_style, apply_node_style

Copy link
Owner

@zakarumych zakarumych left a comment

Choose a reason for hiding this comment

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

Last detail to change if you don't mind

src/ui/viewer.rs Outdated Show resolved Hide resolved
Done to be consistent with other methods
Copy link
Owner

@zakarumych zakarumych left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@zakarumych zakarumych merged commit 4bbe1fb into zakarumych:main Feb 6, 2025
@juh9870 juh9870 deleted the custom-node-style branch February 6, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants