X2CommunityCore / X2WOTCCommunityHighlander

https://steamcommunity.com/workshop/filedetails/?id=1134256495
MIT License
60 stars 69 forks source link

Change UIScreenStack to always consider subclasses #290

Closed robojumper closed 4 years ago

robojumper commented 6 years ago

UIScreenStack has a number of functions, most notably GetScreen, that deal with classes but do not account for subclasses.

    // Returns the first instance of a Screen of the target class type.
    simulated function UIScreen GetScreen( class<UIScreen> ScreenClass )
    {
        local int Index;
        for( Index = 0; Index < Screens.Length;  ++Index)
        {
            if( ScreenClass ==  Screens[Index].Class )
                return Screens[Index];
        }
        return none; 
    }

This causes a number of bugs and general weirdness in a lot of base game code and makes interoperability with mods even worse.

Even though changing it to account for subclasses is changing the behavior of the function and may introduce some bugs, we would still remove a huge number of bugs. Additionally, I have never seen code that relies on the current behavior of GetScreen (non-constructed examples welcome!)

I would suggest adding an optional boolean parameter (true by default) that controls whether subclasses are considered. One can still avoid a dependency on the Highlander by doing something like GetScreen(class'UIArmory_Promotion').class == class'UIArmory_Promotion' if subclasses are not desired.

pledbrook commented 5 years ago

Isn't the normal procedure not the change the signature of functions in vanilla classes? Is UIScreenStack a special case?

robojumper commented 5 years ago

@pledbrook Optional parameters can be added without problems if we don't expect the function to be overridden in mod subclasses.

pledbrook commented 5 years ago

So should that be the approach I take in the pull request, i.e. of just adding an option argument to the existing methods? I'm guessing that no one would or should subclass UIScreenStack, but I don't know that for sure.