Remove requirement of base calls to ensure user skin container layouts are retrieved
#29564
Conversation
…uts are retrieved
peppy
force-pushed
the
skinnable-components-what-2
branch
from
5c9ab82
to
1435fe2
Compare
| @@ -15,33 +15,33 @@ public class GlobalSkinnableContainerLookup : ISkinComponentLookup, IEquatable<G | |||
| /// <summary> | |||
| /// The target area / layer of the game for which skin components will be returned. | |||
| /// </summary> | |||
| public readonly GlobalSkinnableContainers Target; | |||
| public readonly GlobalSkinnableContainers Component; | |||
There was a problem hiding this comment.
Not sure of this rename, I'd probably propose to call this something like GlobalSkinnableContainer or SkinnableContainer or something.
Unless I'm wildly misunderstanding and about to open another debate into naming (in which case please do feel free to immediately tell me to shut the ---- up), GlobalSkinnableContainerLookup is supposed to lookup skinnable containers, which are not individual skinnable components themselves (think score counter, health bar, etc.), but rather containers of skinnable components.
If that understanding is correct then I find this being called Component misleading because it's not going to look up a component as I think of it.
There was a problem hiding this comment.
As for my perspective, I originally wanted this to be called GlobalSkinComponentLookup, because I think of everything skinning as a "component". In that case, this being Component makes sense to me.
In my original PR, I named this Target so I'm definitely thinking in the direction of renaming, regardless of the above.
I disagree with GlobalSkinnableContainer or the other suggestion because there is a world where this is moved to ISkinComponentLookup as I originally had, for the purpose of determining the file name, at which point being called that doesn't really make sense. I'm not really sure what path this will go down in the end, but it feels too inflexible and hard-lined as that.
How about Lookup? We are throwing this around in the office and it seems agreeable.
There was a problem hiding this comment.
As for my perspective, I originally wanted this to be called
GlobalSkinComponentLookup, because I think of everything skinning as a "component". In that case, this beingComponentmakes sense to me.
If this is the consensus from everyone I'll just rewire my brain. Lookup is also fine I guess.
There was a problem hiding this comment.
I think I'm alone in that regard 😛
| /// <summary> | ||
| /// A lookup class which is only for internal use, and explicitly to get a user-level configuration. | ||
| /// </summary> | ||
| internal class UserSkinComponentLookup : ISkinComponentLookup |
There was a problem hiding this comment.
And here I guess I have my answer to the previous thing I guess, or maybe not?
The problem I have here is that UserSkinComponentLookup is going to wrap an ISkinComponentLookup, which can be a GlobalSkinnableContainerLookup, which again confuses me because I delineate the notion of components and containers of components as two notions that will not cross. If that is the case, then I'd argue the following renames should also happen:
ISkinComponentLookup->ISkinLookupUserSkinComponentLookup->UserSkinLookup
because they encapsulate both lookups of individual components as well as containers of components.
If I am not correct in assuming that containers of components and components are separate notions, then I guess disregard all I said and you can probably merge this.
There was a problem hiding this comment.
ISkinComponentLookup->ISkinLookup
I think I agree with this in principle, just to remove N+1 keywords. However....
UserSkinComponentLookup->UserSkinLookup
I don't agree with this one because it sounds like "look up this thing from the user's skin specifically", like "ignore any argon/whatever base skin". Original naming I think is best for this one.
Also, UserSkinComponentLookup may not be just containers in the future. It depends on the path this takes but it may be something passed through to a ISkin.GetConfiguration(UserSkinComponentLookup) to configure SkinnableDrawables (but not override them).
THAT SAID, I will stress that this is up in the air, so it's just my own ramblings at this point.
There was a problem hiding this comment.
Suggestions thrown around for that second one: UserConfiguredSkinLookup JsonSkinLookup. I'd be alright with that, they don't have to be 1-to-1.
There was a problem hiding this comment.
If UserSkinComponentLookup is going to wrap an ISkinComponentLookup then I'm not sure it's an option to drop Component from only one. It's essentially a decorator pattern, so it should be transformative in nature, and not add any new qualifiers or whatever - otherwise something is just straight up not shaped right.
Probably fine to leave as is, I'm not hugely fussed at this stage.
There was a problem hiding this comment.
FWIW I also proposed dropping "Component" from everywhere, including GetDrawableComponent, but smoogi didn't agree with this so I dropped it for now.
We can probably leave it everywhere for now and reconsider later.
There was a problem hiding this comment.
Rather than disagreeing, I want things to be consistent above all. It's a big part of the reason why we got into this mess in the first place (SkinComponentsContainer <> SkinnableDrawable) and the core of my arguments this year.
If we can't agree that keeping naming consistent based on what things are rather than what their end product is, then I don't think we can comfortably move forward, which is why I pushed back against GlobalSkinnableContainerLookup.
Especially during a refactoring stage when everything's new, I think it's best to keep things consistent so we don't step over our feet, and hopefully the next time we look at this code we won't have to piece together what, if any, special functionality is awarded to certain types that justifies their distinct naming.
Over time, when people get used to the structure of things, it will matter not whether a *ComponentLookup type returns a container or a drawable; but only that it represents a lookup type that's defined somewhere in a transformer. There's many paths this could go down in the future, but I'll leave that for a separate time to not derail this.
That is all to say, I'm not tied to "Component", but it is the keyword that has remained consistent and this PR is just moving the rest back in line with it. I'm not against dropping it unless it causes confusion somwhere - saying certain words like "ISkinLookup" or "OsuSkinLookup" or "OsuSkinLookups" (I guess? the enum name) out loud makes it quite appealing.
There was a problem hiding this comment.
Adding my two cents here, dropping the term "Component" in these lookup classes makes me kinda anxious because it gives a more generalised name for the type than what it's actually supposed to be used for. As in, we can have ISkinLookup, but that only will look up drawables/containers, it does not look up textures/samples/config/etc.
We even have ManiaSkinConfigurationLookup existing right now, and having that next to what would be ManiaSkinLookup does not sound good to me at all.
This is a first step in simplifying skinning.
The main goal is to remove the requirement of the
base.call in ruleset transformers in a certain place in order to get user configuration.@bdach this was agreed upon by @frenzibyte and @smoogipoo (and mostly me, although I struggled with navigation through the renaming of classes), so I'm just looking for your sign-off on the renames, and the final commit. The rename commits don't really need any checking – they are just rider refactors you can reproduce with little effort.