-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add AtomLayout
, abstracing layouting within widgets
#5830
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
Conversation
Preview available at https://egui-pr-preview.github.io/pr/5830-lucasexperimentswidgetlayout |
This is mostly in preparation for #5830 where I want to ensure that I don't introduce any regressions
9b01154
to
d468565
Compare
AtomicLayout
that abstracts layouting within widgetsAtomicLayout
, abstracing layouting within widgets
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.
Please run some benchmarks (the ones that are already there, and maybe some new ones more focused on e.g. just Button
)
@@ -99,7 +99,7 @@ fn menu_close_on_click_outside() { | |||
harness.run(); | |||
|
|||
harness | |||
.get_by_label("Submenu C (CloseOnClickOutside)") | |||
.get_by_label_contains("Submenu C (CloseOnClickOutside)") |
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.
Does the label not contain more than just the text?
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 added a todo for this here:
https://github.com/emilk/egui/pull/5830/files#diff-7fd879073a3ce1839ef5c8fdd4dc5e50afc139c018b71c35ca5660bf89420470R657-R673
The text now unfortunately contains the ⏵ from the submenu button. Not sure what a good solution is to handle this. Maybe a flag on the Atomic? Maybe the Atomic could have a alt_text you could use for images, and if you add a text like ⏵ you could do "⏵".a_alt_text("")
or something like that.
This would also solve accessibility for Icon Fonts.
Remember to run |
…h the current one. - offset frame margin for stroke width - handle expansion - add min_size - calculate image size based on font height - correctly handle alignment / ui layout
Ran the demo benchmark for main and the PR and it seems to be slightly slower but I feel the slow down is acceptable for the features we gain:
See this comment for the button benchmark results. |
@@ -87,6 +87,7 @@ ahash.workspace = true | |||
bitflags.workspace = true | |||
nohash-hasher.workspace = true | |||
profiling.workspace = true | |||
smallvec.workspace = true |
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.
nit: sort
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.
mnopqrstu... It should be sorted correctly
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.
Unrelated to this PR: I suspect this grid would be a lot easier to read if we replaced all cross_
/main_
with horiz_
/vert_
instead, e.g. so that the rightmost column is always horiz_align: Right
/// | ||
/// You can use this to first allocate a response and then modify, e.g., the [`Frame`] on the | ||
/// [`AllocatedAtomicLayout`] for interaction styling. | ||
pub struct AtomicLayout<'a> { |
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.
AtomicLayout
is 432 bytes now. Most of that is Atomics
(360 bytes), because it has a SmallVec 2x
of Atomic
(176 bytes), most of which is Image
(152 bytes).
Not something we need to fix right now; I just wanted to investigate it a bit.
A quick fix would be to change AtomicKind::Image
into a Box<Image>
, i.e. optimize for the case where there is no image. That shrinks AtomicLayout
from 432 to to 176 bytes, a 60% reduction.
# Conflicts: # crates/egui/src/widgets/button.rs
AtomicLayout
, abstracing layouting within widgetsAtomLayout
, abstracing layouting within widgets
### Related - atomics landed in egui (emilk/egui#5830) ### What Updates egui to latest master and uses atoms in the help view, removing the icon_text macro since it was basically atomics lite Seems like the snapshots the only thing that changed is the three dot icon slightly moved. But it seems a bit clearer now, so that's a win.
Today each widget does its own custom layout, which has some drawbacks:
Image
toButton
but it will always be shown on the left sideImage
to a e.g. aSelectableLabel
This PR introduces
Atoms
andAtomLayout
which abstracts over "widget content" and layout within widgets, so it'd be possible to add images / text / custom rendering (for e.g. the checkbox) to any widget.A simple custom button implementation is now as easy as this:
The initial implementation only does very basic layout, just enough to be able to implement most current egui widgets, so:
There is a trait
IntoAtoms
that conveniently allows you to constructAtoms
from a tupleto get a button with image and text.
This PR reimplements three egui widgets based on the new AtomicLayout:
I plan on updating TextEdit based on AtomicLayout in a separate PR (so you could use it to add a icon within the textedit frame).