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

Rename SkinComponentsContainer -> SkinnableContainer #27932

Closed

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Apr 19, 2024

I will not accept this type existing with its current name any longer. We currently have the following types:

SkinnableDrawable
ISkinComponentLookup
SkinComponentsContainer
SkinComponentsContainerLookup
ISerialisableDrawableContainer
ISerialisableDrawable

The naming of SkinComponentsContainer is confusing because it implies that it and SkinnableDrawable are totally different from one another, whereas they act pretty much the same way - both via Skin.GetDrawableComponent(). One just returns a Container and one a Drawable.
Of course, SkinnableDrawable has some extra cruft that it really should not have, but I won't address that for now.

The structure I eventually want to end up with is:

--- Common interface ---
ISerialisableDrawableContainer

--- Targets ---
ISkinComponentLookup
GlobalSkinComponentLookup
ManiaSkinComponentLookup

--- Entrypoints (what you use) ---
SkinnableDrawable
SkinnableContainer

--- Endpoints (what you see) ---
ISerialisableDrawable

@bdach
Copy link
Collaborator

bdach commented Apr 19, 2024

Neutral on this rename in general, I'd be fine with either name really, but this one is maybe slightly nicer. That said I wanna remark on one thing:

it implies that it and SkinnableDrawable are totally different from one another, whereas they act pretty much the same way - both via Skin.GetDrawableComponent(). One just returns a Container and one a Drawable.

Just to make sure / establish common ground, we agree that that distiction at the end there is important? As in, they need to coexist because of the implicit understanding that a SkinnableContainer is allowed to have children elements and a SkinnableComponent cannot, right? The "pretty much the same way" part concerns the logistics of lookups and only that?

@peppy
Copy link
Sponsor Member

peppy commented Apr 19, 2024

The weird part to me is that SkinComponentContainer was named that way to imply that the container could house skin components. The new implication is that the container is skinnable, which has less meaning than previously (what does "skinning" a container even mean?).

Would SkinLayoutContainer be a name we could consider?

@smoogipoo
Copy link
Contributor Author

smoogipoo commented Apr 19, 2024

The new implication is that the container is skinnable

The same goes for SkinnableDrawable. It, itself, is not skinnable, but the contained Drawable is.

Would SkinLayoutContainer be a name we could consider

I want any naming to match between this and SkinnableDrawable. So only if that was also renamed to SkinLayoutDrawable.

But I feel like this is also new terminology and that makes me uneasy. "Layout" is very weird terminology because everything is layout.

Just to make sure / establish common ground, we agree that that distiction at the end there is important?

If what I'm agreeing to is that it's important for someone using it to understand that a SkinnableContainer is mutable and a SkinnableDrawable is not, then yes.

However, my perspective coming into all of this as someone that has been somewhat involved with the skinning system but not in its entirety, is that it isn't intuitive as to how these behave at the end of the day. How are their lookups done, do they support configuration, what sort of children should they contain, are they directly skinnable or do I need to do something else, etc.

@peppy
Copy link
Sponsor Member

peppy commented Apr 20, 2024

SkinnableDrawable was implying made with other skin types (argon / legacy) being able to skin the drawable, which in my mind is a bit of a different concept. the Layout part is that the user can lay things out, which is a separate layer no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants