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

Change Interaction::None to NoInteraction #1583

Closed
wants to merge 3 commits into from
Closed

Change Interaction::None to NoInteraction #1583

wants to merge 3 commits into from

Conversation

anthonyarusso
Copy link

The bevy::ui::Interaction interaction enum has an unidiomatic Interaction::None value. E.g. earlier I wanted to optionally query for interactions to a button entity, however matching Query<Option<&Interaction>, With<Button> would never reach the None arm.

@anthonyarusso
Copy link
Author

@alice-i-cecile mentioned the issue of Interaction::None.is_none() == false which may not even compile.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-UI Graphical user interfaces, styles, layouts, and widgets S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Mar 7, 2021
@Davier
Copy link
Contributor

Davier commented Mar 7, 2021

earlier I wanted to optionally query for interactions to a button entity, however matching Query<Option<&Interaction>, With<Button> would never reach the None arm.

That's the expected behaviour. You never reach the None arm because all your entities with a Button component also have an Interaction component (since it's included in ButtonBundle). The query should probably be Query<&Interaction, With<Button>.

@alice-i-cecile mentioned the issue of Interaction::None.is_none() == false which may not even compile.

Not sure what that means. Is it a suggestion to implement is_none() on Interaction?

Aside from that, renaming Interaction::None makes sense if it's confusing people, so I'm in favour.

@alice-i-cecile
Copy link
Member

Not sure what that means

This was a comment by me in Discord, mentioning that it's very strange that the standard is_none method doesn't work for an enum variant called None :) It's clear why it wouldn't, but it still causes confusion and extra cognitive load.

@cart
Copy link
Member

cart commented Mar 13, 2021

Hmm imo Interaction::NoInteraction is redundant. I personally prefer Interaction::None because it's consistent with Option, not despite it. I'm not aware of any recommendations to not use None in enums. If anything, it feels idiomatic to me.

@cart
Copy link
Member

cart commented Mar 13, 2021

I'm down to consider alternatives, but I'd prefer it if they didn't have Interaction in the name (and I'm still biased toward None).

@alice-i-cecile
Copy link
Member

What if we solved this by adding and removing this as a sparse set component, then querying for the Option form? Much more feasible if we had #1613 though.

@Davier
Copy link
Contributor

Davier commented Mar 14, 2021

What if we solved this by adding and removing this as a sparse set component, then querying for the Option form? Much more feasible if we had #1613 though.

That's not possible. Interaction is set to None by default, and anytime the user stops clicking or hovering on the entity. If you remove the component, you remove the entity's ability to be interacted with.

@mockersf
Copy link
Member

If you remove the component, you remove the entity's ability to be interacted with.

The system that manages that could work differently, with an Interactable component for entity selection, and then adding/removing the Interaction component to set its status. I think that would be less ergonomic, as it means the user would now have to check the component for its value, or if it was removed, but it maybe has better semantic that way

@cart
Copy link
Member

cart commented Mar 14, 2021

I think redesigning the api should be out of scope for this pr (which is just about naming). This probably won't be the last time the Enum::None pattern shows up, so its worth discussing it. My preference is still for Enum::None over Enum::NoEnum. If nobody offers alternatives that don't repeat the enum name, I'm probably just going to close this out 😄

@cart
Copy link
Member

cart commented Mar 15, 2021

Closing for now. Feel free to discuss further here if you come up with alternative ideas.

@cart cart closed this Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants