Loading GUI Tabs from XML

[chronologicaldot]

Working with deserializing GUI elements for my own project, I was curious as to how tabs are loaded. Turns out, it appears they aren't properly being attached to the TabControl. The TabControl expects tabs to be passed via addTab, not addChild, because it adds them to a special list. But GUI loading via CGUIEnvironment::loadGUI only loads children, and since tabs aren't children, they either get missed, or are loaded as children but then not controlled as tabs. Deserializing the TabControl won't create tabs, and even if it could, how could anything be added to them? Given the ambiguity of what to do, I can understand why it was put on the backburner.

I'm using revision 5589 at the moment. Yes, it's alittle older (still "1.9"), so maybe this got fixed in a latter revision. Any info?

EDIT: Notably, my proposed solution would be to override addChild and have it sort tabs into the special list. I believe this would fix the immediate deserialize problem, and I can't see why anyone would want to add a tab as a child to a TabControl and NOT have it be controlled.
I'll even add it myself and post a diff if you want.

EDIT 2: Notably, there's still the issue of (SNIP: how to load elements onto the tabs and) how to save the tabs.

Sorry I'm hashing this all this out loud to myself. I intend to implement some kind of solution, so stay tuned.

[CuteAlien]

Not something that got fixed in later svn versions - didn't hear about this before. The problem is likely because tabs are not sub-elements (I think those didn't exist yet when tabcontrol was coded), so serialization is independent.

Which probably makes even sense as tabs can also be added without tab-controls, so they should have own serialization. But I agree with you that once you add them to the IGUITabControl then having them use addTab internally when addChild is called makes sense (we wouldn't even have needed addTab in the first place then...). And without looking at code right now I suppose it's likely the best workaround (other one would be special code in serialization itself - which sounds worse to me).

<--- stays tuned :-)

[chronologicaldot]

More rambling out-loud on my part here. This problem is snowballing, darn it.

Attempting to implement the addChild(), I discovered I was going to make an unsafe cast because the Tab's list requires tabs of type CGUITab. Even though we can technically detect the type, just because it is of type EGUIET_TAB doesn't mean that it descends from CGUITab. The reason for this is that this particular tab system requires an extra function: setNumber(). The only reason for this is because the interface IGUITab doesn't provide it for some odd reason.

It seems someone had a particular implementation in mind, as noted in the comment above IGUITab::getNumber(), which made it difficult for the implementation, as noted by the comment next to the Tabs array in CGUITabControl.
The tab number is used for positioning and accessing the tab, which is something you should get via the TabControl, not the tab itself. Maybe it was meant for convenience, but the problem is that the number can change internally (when two tabs have the same number, the one currently in the slot gets sent to last), and no warning is given, rendering the number useless. It'd be easier just to save a pointer to the tab you want to make active, so for that, I could add IGUITabControl::getOrder( IGUITab* ) (or getNumber() if you prefer, though this is really arbitrary for a name).

To preserve the API and let TabControl accept IGUITab (instead of only CGUITab), I could rewrite CGUITabControl to not use setNumber() - instead it'll use the REAL index. It makes IGUITab::getNumber() meaningless, but it wasn't adding any value anyways.

There's already IGUITabControl::setActiveTab( IGUITab* ), and CGUITabControl::setActiveTab(s32) uses the real index.

I looked at CGUIEnvironment::writeGUIElement(), and I can say that tabs are correctly serialized if they remain as NOT sub-elements. i.e. They're fine as-is right now. It also makes sense in regards to GUI element focus. EDIT: I feel silly reiterating that after re-reading what you said. Oh well..

Here's a bizarre fact: IGUITabControl::removeElement() contains no cut-off for when a tab is found and actually assumes that the tab itself has been added as a child!
Also, the loop for removing tabs will remove all instances from it from the Tabs list, even though IGUIElement::removeChild() (called at the end) will only remove one instance from a GUI element's child list. I'll probably change this (and make it only one tab removed). EDIT: Actually, the addTab() forbids adding the same tab twice. :/ But what if I want that? Ugh.

That said, my current analysis suggests it's safe to just add the tabs as children also. Yes, the tabs will technically be saved twice (at least once in the Children and once in the Tabs), but it's the least disruptive way of handling this situation so we can have the full GUI loading.

[chronologicaldot]

(Pardon the double-post. At least this way you know I'm posting something new.)
Update: So just changing addChild() won't quite work either. It does help, but not entirely. When loading tabs, the function called is, sadly, addChildToEnd (not addChild) (via IGUIElement::addTab() used by CDefaultGUIElementFactory). This means the tabs loaded from XML aren't sorted into the Tabs list. And it's illegal to have addChild() in the constructor because it's virtual... so I'm stuck at the moment.

If I ditched the Tabs array altogether and slowly worked through the child array... the whole system would be slower, though loading would be correct. :-/ Ugh. I shouldn't be working on this at 1:30 in the morning.

Aside:
The OverrideTextColorEnabled was incorrectly set to the reverse when deserialized. I believe the person writing the deserialize simply wanted the default to be "true", so I've made it so it does default to true.

I ran into this because it turns out that CGUITabControl wants to refresh the tab text color in its draw() method using refreshTextColor(). I decided to ditch that and changed getTextColor() to return the default skin color when the override was turned off. The results are exactly the same as before with the exception that the TextColor of the tab is not pointlessly overwritten by the skin color (which would default the purpose of setting it in the first place and having a setting (the override boolean) to turn it on and off).

[CuteAlien]

Hm, not seeing the bug with OverrideTextColorEnabled. It's set to false when it wasn't loaded. Which could be improved with 1.9 as we can pass now default parameters to getAttributeAsBool (was not yet possible before when this was coded) in case it fails and it should be set to old OverrideTextColorEnabled as default parameter.

The other problems - will have to check myself, not understanding right now why you need a cast when you overload addChild.

Also - I do not see any removeElement function in IGUITabControl, do you mean removeTab?

For the setNumber/getNumber stuff... will have to read code myself again, not sure right now what is going on there. edit: On a very quick 1-minute view... I suppose you are right - those number stuff looks like it was supposed to help with something like keeping active index when tabs are inserted - and is likely not needed at all (but will have to take a longer look in case I miss something).

[chronologicaldot]

Hm... seeing as it's not in commit 5064 either. EDIT: Oops. I meant removeChild. Sorry. I confused you and then that confused me.

The bug with OverrideTextColorEnabled is just that: It's set to false when not loaded or returns false. What happens when it is loaded? If false, then the condition makes it true (when it should be false), but if true, then the condition ( if( !override) ) makes it true. However, if you look up at serializeAttributes(), it's saved directly - there's no flipping.

The list of tabs is of type core::array<CGUITab*> instead of core::array<IGUITab*>, so in order to save a GUI element to that list, it has to be casted to CGUITab*.

[CuteAlien]

Ah right with OverrideTextColorEnabled. Oh - my bug... I guess back then the idea in my head was not to break downward compatibility - and then to mess up code completely :-) (confusing return parameter with.. uhm... no idea - that looks like a complete brain-fart ^^) Guess I'll fix that on even for 1.8 (should even be fixed for 1.7, but I'm not going to maintain that anymore). Thanks!

[CuteAlien]

OK, serializing that flag is fixed. That was the easy part...

[chronologicaldot]

Thinking it over, the easiest walk-around for fixing the add-tab-to-Tabs-list problem would be simply removing the tab (from the child list) and re-adding it with addChild(). Since we're hiding the implementation of CGUITab behind the IGUIEnvironment interface, this would work.

Code: Select all

 
//! Adds tab to the environment.
IGUITab* CGUIEnvironment::addTab(const core::rect<s32>& rectangle,
    IGUIElement* parent, s32 id)
{
    IGUITab* t = new CGUITab(-1, this, parent ? parent : this,
        rectangle, id);
    // Addendum by chronologicaldot to fix tab control problem
    if ( parent && parent->getType() == EGUIET_TAB_CONTROL ) {
        parent->removeChild(t);
        parent->addChild(t); // Constructor uses addChildToEnd but we want addChild
    }
    t->drop();
    return t;
}
 

This solution should be harmless regardless of the implementation of IGUITabControl but at least this way it gives the tab control a chance to add the tab to a Tabs list.
Now I need to test this solution.

[chronologicaldot]

Sorry to double post. In case you're curious, here's a peek at the changes in CGUTabControl.cpp

Code: Select all

 
//! Adds child, sorting tabs into the special list
void CGUITabControl::addChild(IGUIElement* child) {
    // chronologicaldot fix for tab management issue
    if ( child && child != this ) {
        if ( child->getType() == EGUIET_TAB )
            addTab((IGUITab*)child);
        else
            IGUIElement::addChild(child);
    }
}
 
//! Adds a tab
IGUITab* CGUITabControl::addTab(const wchar_t* caption, s32 id)
{
    CGUITab* tab = new CGUITab(Tabs.size(), Environment, this, calcTabPos(), id);
 
    tab->setText(caption);
    tab->setAlignment(EGUIA_UPPERLEFT, EGUIA_LOWERRIGHT, EGUIA_UPPERLEFT, EGUIA_LOWERRIGHT);
    tab->setVisible(false);
    Tabs.push_back(tab);
    // chronologicaldot fix for tab management issue
    tab->grab();
    Children.push_back(tab);
    tab->updateAbsolutePosition();
 
    if (ActiveTab == -1)
    {
        ActiveTab = 0;
        tab->setVisible(true);
    }
 
    recalculateScrollBar();
 
    return tab;
}
 
 
//! adds a tab which has been created elsewhere
void CGUITabControl::addTab(IGUITab* tab)
{
    if (!tab)
        return;
 
    tab->grab();
    Tabs.push_back(tab);
    // chronologicaldot fix for tab management issue
    addChildToEnd(tab);
    tab->updateAbsolutePosition();
 
    if ( ActiveTab == -1 ) {
        ActiveTab = Tabs.size() - 1;
        setActiveTab(ActiveTab);
    }
}
 
//! Insert the tab at the given index
IGUITab* CGUITabControl::insertTab(s32 idx, const wchar_t* caption, s32 id)
{
    if ( idx < 0 || idx > (s32)Tabs.size() )    // idx == Tabs.size() is indeed ok here as core::array can handle that
        return NULL;
 
    CGUITab* tab = new CGUITab(idx, Environment, this, calcTabPos(), id);
 
    tab->setText(caption);
    tab->setAlignment(EGUIA_UPPERLEFT, EGUIA_LOWERRIGHT, EGUIA_UPPERLEFT, EGUIA_LOWERRIGHT);
    tab->setVisible(false);
    Tabs.insert(tab, (u32)idx);
    // chronologicaldot fix for tab management issue
    tab->grab();
    Children.push_back(tab);
    tab->updateAbsolutePosition();
 
    if (ActiveTab == -1)
    {
        ActiveTab = 0;
        tab->setVisible(true);
    }
 
    recalculateScrollBar();
 
    return tab;
}
 
//! Removes a tab from the tabcontrol
void CGUITabControl::removeTab(s32 idx)
{
    if ( idx < 0 || idx >= (s32)Tabs.size() )
        return;
 
    //Tabs[(u32)idx]->drop();
    //Tabs.erase((u32)idx);
 
    // chronologicaldot fix for tab management issue
    IGUITab* t = Tabs[(u32)idx];
    t->drop();
    Tabs.erase((u32)idx);
    IGUIElement::removeChild(t);
}
 
//! Clears the tabcontrol removing all tabs
void CGUITabControl::clear()
{
    for (u32 i=0; i<Tabs.size(); ++i)
    {
        if (Tabs[i]) {
            Tabs[i]->drop();
            // chronologicaldot fix for tab management issue
            IGUIElement::removeChild(Tabs[i]);
        }
    }
    Tabs.clear();
}
 

[chronologicaldot]

Updated above code with updateAbsolutePosition and proper reference clearing of the original parent.

[CuteAlien]

Uhm, I have to check why this works :-) Will try to find some time, but will take a few days probably.

[chronologicaldot]

Don't worry about it yet. I'm still working on it. There's a few things to figure out. When I'm done, I'll post diff files and my test program.

[chronologicaldot]

Looking at it again with fresh eyes helps.
Tabs are already loaded as children. How tabs are added to the Tabs list was via CGUITab via deserializeAttributes. However, deserializeAttributes casts to CGUITabControl (for EGUIET_TAB_CONTROL), so I added IGUITabControl::addTab( IGUITab* ) and changed the casts. CGUITabControl::addTab( CGUITab* ) seems to have been meant only for deserialization purposes due to how it it leaves empty slots so it can insert tabs in the right order. The tab order was also stored in the tabs for the same reason. But this tied the implementations of the tab control and tabs together (which also makes the interfaces needless extras).

To fix it, I changed CGUITabControl::addTab( CGUITab* ) to addTab( IGUITab* tab, s32 desiredIndex ) and added this to the IGUITabControl interface. In CGUITab::deserializeAttributes(), I changed the parent->addTab(this) call to parent->addTab(this, Number), so it could use the number it just got from file. I had to make some changes in CGUITabControl::addTab and elsewhere to remove the dependence on the tab number, but the number wasn't being used anyways (just thrown around until serialization, where it would just become the index).

IGUITabControl.h diff https://pastebin.com/zS2JWqm9
CGUITabControl.h diff https://pastebin.com/9x6Dzpk1
CGUITabControl.cpp diff https://pastebin.com/cibwUkJ9

In short, these changes make the interface safer. (No casting to CGUITabControl nor CGUITab from EGUIET_TAB_CONTROL and EGUIET_TAB). There should be no effect on backwards compatibility other than that tab numbers won't necessarily be indices even though the order they are loaded in should be the same as before.

[chronologicaldot]

I forgot to mention that, without my changes, (that is, adding IGUITabControl::addTab()) IGUIEnvironment::addTab() is useless to anyone trying to make a tab via IGUIEnvironment. This is because there is currently no way of adding these tabs. (You can only create tabs with IGUITabControl.) The reason someone might want to add tabs is because they also might want to remove them temporarily.