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

Add Button::image_and_text #832

Merged
merged 4 commits into from
Nov 13, 2021
Merged

Add Button::image_and_text #832

merged 4 commits into from
Nov 13, 2021

Conversation

d10sfan
Copy link
Contributor

@d10sfan d10sfan commented Oct 22, 2021

Been using egui with the sdl2 backend for a short while now and it's been a great experience, making UI has been nicely easy with it after learning it.

One of the things I was wanted to do which the integration with sdl2 made possible was to have a hybrid mouse and controller UI, where someone could navigate and press buttons using the controller as well.

Part of that I wanted to be able to have a button with a controller icon in it, to show which one could trigger it. I created a "ImageButtonWithText", which tried to merge the ideas of an ImageButton and a Button, similar to how the Radio Button or Checkbox ones work.

This was mainly copying and pasting the Button implementation and adding the necessary spacing and positions for putting the image and text inside the button. It worked well visually, as can be seen in the screenshot below, which was built using this idea, although not sure if it may be better to add this as an additional feature to the "Button" instead of it being its own Button type.

The code to add this type looks like this, assuming that "texture_confirm" was already created (same idea behind adding an Image to the ui).

if ui.add(egui::ImageButtonWithText::new("Ok", texture_confirm, egui::vec2(32 as f32, 32 as f32))).clicked() {
    ok = true;
}

image

@emilk
Copy link
Owner

emilk commented Oct 25, 2021

Thanks!

although not sure if it may be better to add this as an additional feature to the "Button" instead of it being its own Button type.

I think this would be worth trying, basically adding a constructor Button::image_and_text(ok_texture, "OK"). Less code to maintain!

@d10sfan
Copy link
Contributor Author

d10sfan commented Oct 25, 2021

Thanks for the feedback and taking a look!

I've made a new version, committed to the same branch, that does it to the existing Button implementation, instead of that new one. Does lead to alot less code, and the changes to the existing button were tried to be as small as possible.

With this I also added a convenience function like the "button" one called "button_with_image", so you don't have to call the full Button::image_and_text.

I saw that there were some validation checks that were run, I ran the same ones locally and fixed a few things, code cleanliness wise.

Also, not sure the best way you'd like the commits to be presented, I can do a squash on this branch if you're happy with these changes.

@d10sfan d10sfan marked this pull request as ready for review October 27, 2021 16:23
Copy link
Owner

@emilk emilk left a 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 demonstrate this, perhaps in egui_glium/examples/native_texture.rs (which has an actual texture loaded). if ui.button("Quit").clicked() { could be replaced with the new Button::image_and_text

egui/src/widgets/button.rs Outdated Show resolved Hide resolved
egui/src/widgets/button.rs Show resolved Hide resolved
egui/src/widgets/button.rs Outdated Show resolved Hide resolved
egui/src/ui.rs Outdated Show resolved Hide resolved
@d10sfan d10sfan requested a review from emilk November 8, 2021 04:28
@d10sfan
Copy link
Contributor Author

d10sfan commented Nov 8, 2021

I re-based with the latest and removed the ui... function.

I added the change to the example glium but not sure how to test it, I didn't see a way to run it like you can with the demo, but it should work.

@emilk
Copy link
Owner

emilk commented Nov 8, 2021

cargo run --example native_texture

@emilk
Copy link
Owner

emilk commented Nov 8, 2021

Screen Shot 2021-11-08 at 21 34 57

The composition is not great. Both the image and the text is too far to the left

egui/src/widgets/button.rs Outdated Show resolved Hide resolved
@d10sfan d10sfan requested a review from emilk November 8, 2021 22:16
@emilk emilk changed the title Add ImageButtonWithText Add Button::image_and_text Nov 13, 2021
@emilk emilk merged commit 90757ca into emilk:master Nov 13, 2021
emilk added a commit that referenced this pull request Nov 13, 2021
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