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

Introduce egui::FullOutput, returned from Context::run #1292

Merged
merged 2 commits into from Feb 22, 2022
Merged

Conversation

emilk
Copy link
Owner

@emilk emilk commented Feb 22, 2022

This adds a new struct:

pub struct FullOutput {
    pub output: Output,
    pub needs_repaint: bool,
    pub textures_delta: epaint::textures::TexturesDelta,
    pub shapes: Vec<epaint::ClippedShape>,
}

The name is not great, nor the name of the nested Output, but structurally it works rather well.

Closes #1281

@derivator
Copy link

I think I prefer the names I suggested: since this is a breaking change anyway, you might as well rename Output... FullOutput is a bit awkward.
It's fine though, at least handle_egui_output does not return TexturesDelta anymore, I think that was the most confusing thing. 👍

@emilk
Copy link
Owner Author

emilk commented Feb 22, 2022

If we rename Output we should probably also rename ctx.output() which makes it even more of a breaking change :/

@derivator
Copy link

derivator commented Feb 22, 2022

Ah, I see. I guess the use case there is e.g. for custom widgets to be able to open a URL or set the cursor? Seems like it's not something that most users would encounter often. egui itself only calls output() 15 times. Maybe deprecate it with a note to use the new name? That way calls of the form ui.ctx().output() would still work even though the name of the return type changed...

This makes me notice another issue though: custom widgets probably want to be able to set needs_repaint, right? Possibly even insert textures or shapes into the output? That seems like it has huge footgun potential though...

Ultimately, this is getting into egui internals that I'm not that familiar with, so I'm happy to just trust your judgement here :)

@emilk
Copy link
Owner Author

emilk commented Feb 22, 2022

This makes me notice another issue though: custom widgets probably want to be able to set needs_repaint, right? Possibly even insert textures or shapes into the output? That seems like it has huge footgun potential though...

No - users should call ctx.request_repaint() and use the TextureManager and Painter API:s instead of touching the output directly.

@emilk
Copy link
Owner Author

emilk commented Feb 22, 2022

I renamed it PlatformOutput but kept the name ctx.output() for now. I don't want to spend too much time on this (I want to release egui 0.17.0!)

@derivator
Copy link

Looks good to me 👍 Thanks for all the work on egui, it really is a fantastic library :)

@emilk emilk merged commit 31d3249 into master Feb 22, 2022
@emilk emilk deleted the refactor-output branch February 22, 2022 16:13
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.

Awkward API for TexturesDelta in integrations
2 participants