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 Into bound into id methods #4680

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

creativcoder
Copy link

I've felt the need for this several times, and assigning ids adds a little bit of friction by having the user call into() when passing a String or &str.

This PR adds the Into trait bound on the id method that reduces this small friction from the end user.

@emilk
Copy link
Owner

emilk commented Jun 23, 2024

I haven't written this down before (will do after writing this here first), but there is a reason for the current design:

An Id is supposed to be globally unique. This is ensured by having Ui construct a tree of locally unique "ID sources" (anything implementing Hash, but often string literals in practice).

So when constructing a child-region of some kind (Grid, CollapsingHeader, …) you specify a locally unique ID source. When constructing a top-level thing (e.g. an Area or Window) you specify a globally unique Id.

I want to make this somewhat clear in the API (though obviously I haven't succeeded) so the rough idea is this: The locally unique ID sources are more common, so we optimize the ergonomics for those. So for those we take anything impl Hash. For the globally unique Ids however, we force the user to explicitly type Id::new(…) or Id::from(…) or ui.id().with(…) so they think through the promise that they make to egui.

Does that make sense?


For the plot items the only use of the Id is to match against PlotResponse::hovered_plot_item, which is an Option<Id>, so you want an Id there somehow anyway, I believe.

@creativcoder
Copy link
Author

Ah I see

It's a little less clear that we want to create a unique global id when we have to create them from Id::new() or Id::from(), because I still ended up creating an Id passing a string lying around in my scope in one of my codebase.

Maybe a change in method name like global_id for containers and top level ui elements would make the intention more clear. Does that work?

Also a few places, some of the methods that assign an id have different names like: collapsing_header, they can be renamed to just id() like others.

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.

None yet

2 participants