Changing the return type of UIElement::LoadChildXML()

Everything about the development of Urho3D.

Changing the return type of UIElement::LoadChildXML()

PostPosted by TheComet » 14 Oct 2016, 10:28

I have this situation in my code where I've subclassed UIElement and am maintaining a child UIElement that gets loaded from XML (a pattern I learned from Qt now that I think about it).

What I'd like to be able to write is this:
Code: Select all
void MenuScreen::ReloadUI()
{
    ResourceCache* cache = GetSubsystem<ResourceCache>();

    if(ui_)
        RemoveChild(ui_);
    ui_ = LoadChildXML(
        xml_->GetRoot(),
        cache->GetResource<XMLFile>("UI/DefaultStyle.xml")
    );
}


Where:
Code: Select all
SharedPtr<XMLFile> xml_;
SharedPtr<UIElement> ui_;


Except that I can't, because LoadChildXML() returns a bool and not a pointer to the child that was created. There seems to be no easy way to fish out the created child afterwards, because I can't rely on the name nor can I rely on ui_ being the only child of MenuScreen.

I propose the function be changed to return the child pointer. No functionality is lost, since you can still check if creation failed by comparing to NULL:
Code: Select all
UIElement* UIElement::LoadChildXML(const XMLElement& childElem, XMLFile* styleFile, bool setInstanceDefault)
{
    bool internalElem = childElem.GetBool("internal");
    if (internalElem)
    {
        URHO3D_LOGERROR("Loading internal child element is not supported");
        return NULL;
    }

    String typeName = childElem.GetAttribute("type");
    if (typeName.Empty())
        typeName = "UIElement";
    unsigned index = childElem.HasAttribute("index") ? childElem.GetUInt("index") : M_MAX_UNSIGNED;
    UIElement* child = CreateChild(typeName, String::EMPTY, index);

    if (child)
    {
        if (!styleFile)
            styleFile = GetDefaultStyle();
        if (!child->LoadXML(childElem, styleFile, setInstanceDefault))
            return NULL;
    }

    return child;
}


This would require changes to the angelscript bindings:

APITemplates.h:953
Code: Select all
static UIElement* UIElementLoadChildXML(XMLFile* file, XMLFile* styleFile, UIElement* ptr)
{
    if (file == NULL)
        return NULL;

    XMLElement rootElem = file->GetRoot("element");
    if (rootElem)
        return ptr->LoadChildXML(rootElem, styleFile);
}


APITemplates.h:1055
Code: Select all
    engine->RegisterObjectMethod(className, "UIElement@+ LoadChildXML(const XMLElement&in, XMLFile@+ arg1 = null, bool arg2 = false)", asMETHOD(T, LoadChildXML), asCALL_THISCALL);
    engine->RegisterObjectMethod(className, "UIElement@+ LoadChildXML(XMLFile@+, XMLFile@+ arg1 = null)", asFUNCTION(UIElementLoadChildXML), asCALL_CDECL_OBJLAST);


As well as in the editor script EditorUIElement.as:319
Code: Select all
if (editUIElement.LoadChildXML(xmlFile, uiElementDefaultStyle !is null ? uiElementDefaultStyle : uiStyle) !is null)


I can make a PR if you want.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Re: Changing the return type of UIElement::LoadChildXML()

PostPosted by cadaver » 14 Oct 2016, 10:40

I believe this would make sense. If you want, you can make a PR (it's not a large change)

One thing to consider is what to do when the child has been created but load fails. Should the child be destroyed then? Presently it isn't, but false would be returned regardless in the current implementation.
User avatar
cadaver
Urho3D author
Urho3D author
 
Posts: 1802
Joined: 16 Jan 2014, 14:52
Location: Finland

Re: Changing the return type of UIElement::LoadChildXML()

PostPosted by TheComet » 14 Oct 2016, 10:57

Good point. I'd argue it would make sense to destroy the child if loading fails. It makes little sense to keep an empty child around. If the child isn't destroyed, it would be kind of deceitful to return NULL.

Returning the empty child would be the other option, however, that makes it harder to check if loading fails.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Re: Changing the return type of UIElement::LoadChildXML()

PostPosted by cadaver » 14 Oct 2016, 12:04

If you make the PR, you can make it so that the child is destroyed. We can correct later if it turns out other behavior is needed. This error case shouldn't happen in normal editor use; only in the case of malformed handwritten UI XML, but in that case I don't think it would even get as far as creating the child.
User avatar
cadaver
Urho3D author
Urho3D author
 
Posts: 1802
Joined: 16 Jan 2014, 14:52
Location: Finland

Re: Changing the return type of UIElement::LoadChildXML()

PostPosted by TheComet » 14 Oct 2016, 19:20

Here you go: https://github.com/urho3d/Urho3D/pull/1649

I made sure the editor works and I tested the behaviour of the function in my own code.

There appear to be no lua bindings for this function. I didn't create any because I don't understand it enough.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany


Return to Developer Talk

Who is online

Users browsing this forum: No registered users and 0 guests

cron