destroy Tabpages

Jack106

23-06-2010 12:16:16

Hey,
i have a little problem to destroy tabpages. I use QuickGUI 9.10 and when i call tabcontroller->destroyTabPage(index) the application crashs. In addition when I destroy the TabConroller, it works. The TabPage has no dependencies. Maybe its a bug?
I hope for an answer as soon as possible.
cheers

Jack106

23-06-2010 13:15:34

hey i think i have found the bug.
In QuickGUITabControl.cpp line 228 you have this loop.

for(std::list<TabPage*>::iterator it = mTabs.begin(); it != mTabs.end(); ++it)
{
if(updatePages)
{
// Update index
(*it)->setIndex(count);
}

if(!updatePages && (count == index))
{
updatePages = true;

removeChild((*it));
mTabs.erase(it);

if(mDesc->sheet != NULL)
mDesc->sheet->mFreeList.push_back((*it));
else
Root::getSingleton().mGUIManagers.begin()->second->mFreeList.push_back((*it));

--count;
}

++count;
}


You have forgotten to consider the last item. When the last item is deleted, the Problem is that your list decreases, but the iterator stays unchanged. As a result , the next loop execution trys to call a not existing objekt : (*it)->setIndex(count);

You need a condition for the last item, which decreases the Iterator. In Addition maybe you should use "it < mTabs.end()" instead of "it != mTabs.end()".

Calder

23-06-2010 16:25:10

Do STL iterators define a '<' method? I thought they were only privy to their own value and a pointer to the previous and next values, so they don't need to understand the structure of the container as a whole.

I think you're right on the rest though, Jack. I remember running into this earlier, and the conclusion is that when you erase by iterator from an STL container, the behavior is undefined. So Microsoft might have sugar-coated it in their compiler, but there's no guarantee it will work on others. The solution, I think, would be to replace this:

for (std::list<Something>::iterator it = somelist.begin; it!=somelist.end(); ++it)
{
somelist.erase(it);
}

with this:

for (std::list<Something>::iterator it = somelist.begin; it!=somelist.end(); ++it)
{
somelist.erase(it--);
}

Jack106

23-06-2010 17:35:00

Hey I came to the same solution, but there were more problems.
I wrote the loop as it should be:


for(std::list<TabPage*>::iterator it = mTabs.begin(); it != mTabs.end(); ++it)
{
if(it == mTabs.end())
{
break;
}
if(updatePages)
{
// Update index
(*it)->setIndex(count);
}

if(!updatePages && (count == index))
{
updatePages = true;
std::list<TabPage*>::iterator it2 = it;
if((*it) == mSelectedTab)
{
mSelectedTab = NULL;
}
removeChild((*it));
if(mDesc->sheet != NULL)
mDesc->sheet->mFreeList.push_back((*it));
else
Root::getSingleton().mGUIManagers.begin()->second->mFreeList.push_back((*it));
it2--;
mTabs.erase(it);
it = it2;
--count;
}

++count;
}

updateTabPositions();
}


I have tested the code, it works good for me. For a second i thought about to hang myself up, but luckily i found this solution before ;-) .

cheers

Calder

23-06-2010 17:40:00

Haha, glad you fixed it! :D

Zini

23-06-2010 18:46:04

Sorry, but that looks wrong. Think about, what would happen, if the first entry in the container is deleted.

Hint: For this kind of operation a while loop is better than a for loop.

Calder

23-06-2010 19:27:13

Oh yikes, you are quite right, good sir! :? I guess there's no slick way around this then?

for (std::list<Something>::iterator it = somelist.begin; it!=somelist.end(); ++it)
{
if (it == somelist.begin()) { somelist.erase(it); it = somelist.begin(); }
else { somelist.erase(it--); }
}


Unless of course your using your own custom container classes... I'm already doing that in order to provide thread-safety, but that doesn't help most people. Could you do this?

template <class K, class V>
class Map : std::map<K,V>
{
public:
void erase (iterator& it)
{
if (it == begin()) { erase(it); it = begin(); }
else { std::map<K,V>::erase(it--); }
}
};

Jack106

23-06-2010 22:45:17

Yeah you are absolutely right, i will try to fix my code tomorrow. Thx for your help i didn't saw this problem.

kungfoomasta

23-06-2010 23:00:05

Wow you guys are fast! Sorry for my buggy code, there is a lot of areas that aren't tested well enough. :oops:

I like the erase(it--), looks slick, but has that problem Zini mentioned. I guess in order to make thing simple you could just:

1. Iterate through list, find tab and remove it.
2. Iterate through list again and correct tab indices

Not as efficient, but less error prone. That or we can try to special-case removing first tab. I think we can assume tab controls won't have hundreds of tabs on them, so first choice probably best one IMO.

kallitokaco

23-06-2010 23:33:29

Hey guys,

I think I found a quite good solution, based on Jacks code. Could you please check it, if it produces errors. I think it should work but for me it's to late to test it.


std::list<TabPage*>::iterator it = mTabs.begin();
std::list<TabPage*>::iterator it2 = it;
while( it != mTabs.end()) //used while because it++ is not needed anymore and a second iterator definition sould be outside the while( dont have to allocate it every time)
{
if(updatePages)
{
// Update index
(*it)->setIndex(count); //not checked
}

if(!updatePages && (count == index))
{
updatePages = true;

if((*it) == mSelectedTab)
{
mSelectedTab = NULL;
}
removeChild((*it));
if(mDesc->sheet != NULL)
mDesc->sheet->mFreeList.push_back((*it));
else
Root::getSingleton().mGUIManagers.begin()->second->mFreeList.push_back((*it));

it2 = it; //copy the current it
if(it== mTabs.begin())
{ //if it stays at the 1. item
if(it2++ == mTabs.end()) //you stay at the first and the next item does not exitit
{ //=>there is only one item
mTabs.erase(it); //erase it
it= it2; //make it valid again
}
else
{ //if it stays at the 1 but there are more further items
mTabs.erase(it++); //go to the 2. item and delete the first
}
}
else
{ //if it stays before the end or within the list
if(it2++ == mTabs.end()) //if it stays before the end
{
mTabs.erase(it); //delete it
it= it2; // make it valid
}
else //will be executed quite often if you use lagre lists
{ //within the list
mTabs.erase(it--); //erase the item and go back
++it; //go to the next item to prove
}
}
--count;
}

++count;
}

updateTabPositions();
}

I hope its not so buggy. ;-)

gn8

Jack106

24-06-2010 08:27:28

Hey kallitokaco, i think your code looks right, i will test it later.

@kungfoomasta

Hey you mean sth. in this way?:

std::list<TabPage*>::iterator it = mTabs.begin();
for(int count = 0; it != mTabs.end(); count++)
{
if(count == index)
{
if((*it) == mSelectedTab)
{
mSelectedTab = NULL;
}
removeChild((*it));
if(mDesc->sheet != NULL)
mDesc->sheet->mFreeList.push_back((*it));
else
Root::getSingleton().mGUIManagers.begin()->second->mFreeList.push_back((*it));
mTabs.erase(it);
break;
}
it++;
}

it = mTabs.begin();
for(int i = 0;it != mTabs.end(); i++)
{
(*it)->setIndex(i);
it++;
}


The code isn't very optimal because the second loop starts from the beginning, but it shout work too.

Zini

24-06-2010 10:05:16

Huh? Stop it! You are making me dizzy. The problem at hand is entirely trivial.

Untested code with some pseudo code elements:


std::list<TabPage*>::iterator it = mTabs.begin();

while( it != mTabs.end())
{
... do whatever you have to do with all entries, regardless if they are to be deleted or not.

if (it should be removed)
{
... do whatever processing is needed on entries to be deleted.
mTabs.erase (it++);
}
else
{
++it;
}
}



Really, there is no need for special case code or two loops.

kungfoomasta

25-06-2010 02:04:26

@kallitokaco: wow that solution looks really complicated actually... :P

@Jack106: yah thats what I was proposing.

@Zini: thats a good idea actually. The code doesn't actually have to set the index of tabs that are not affected, ie everything prior to the tab to be removed. But if I recall correctly, I don't think the setIndex function would require a lot of processing. So in theory you could do:

// for loop

// if we want to remove this tab, remove it and decrement counter

// else set the index of the tab to counter

// increase counter

Stardragon

27-08-2010 08:58:07

Y'know, y'all could jes' use a vector instead of a list... </drawl>

Might solve a couple of problems at once. Either that or I'm missing something --- which is highly likely as I've had no coffee yet and am just skimming this while at work :lol:

kungfoomasta

27-08-2010 21:45:28

Vectors aren't ideal for removing and inserting elements in the container, right? TabPages can shift around, or the user might want to delete the 2nd one out of 5 tabs, for example.

Stardragon

28-08-2010 00:31:06

Vectors aren't ideal for removing and inserting elements in the container, right? TabPages can shift around, or the user might want to delete the 2nd one out of 5 tabs, for example.
They're not as efficient as lists, no. But you could have a vector of structs, and the struct contains a pointer to the tab and a number. When you erase one tab and the others shuffle down, you can grab the iterator that the erase() function returns, and then just iterate through to the end and renumber... so, basically, the number of the vector is the number of the tab...

Made sense earlier today, even if it doesn't now :P :lol: