Q_FOREACH deleting objects

[wysota]

What's the point of using "c->deleteSelected()" instead of "delete c"?

What is that you're trying to achieve? Maybe there's a better way to do it...

[veda]

What's the point of using "c->deleteSelected()" instead of "delete c"?

Well, I use that because before deleting c I have other stuffs to perform which I perform in deleteSelected function.

What is that you're trying to achieve? Maybe there's a better way to do it...

The above given example is given for clear picture about where the application crashes. The code might look stupid as I just imagined and have jot down scenario happening in my project. As I said earlier, I have some objects on drawing area and I want to delete the one which the user has selected.

The sample below has one parent Object class with 7 children. Now I iterate over the children objects and find child2 with instance 2. If found, first I delete child2 with instance 2 and then child 3. Now my control returns to for loop, then iterate again, print "child2" with instance 3( the next child ). Finally, the last child is encountered ie child3, OH!!! but that is already deleted. but the pointer is not, hence application crashes.

As I have been saying again and again, the same below code executes without problem on Qt3.

I have same scenario happening in my application, it has some million lines of codes so I thought to write a sample program to reproduce the bug. And I could achieve.

Unless the sample given below is not solved I cannot move ahead, and you know deadline I hate it but cant help.

So, again please help me to solve the problem.

[joseph]

hai

QList<QObject*> list = obj->findChildren<QObject*>();
for( QList<QObject*>::iterator it = list.begin(); it != list.end(); ++it )
{
qDebug() << (*it)->objectName();
if( (*it)->objectName() == QString("child22") )
{
c31->removeChild2( c22 );
}
}

I think it will again crash ,because the iterator is not aware about the “DELETION” of “C31”.
For this we need a mechanism which will update the iterator .In Qt Assitant , it is said tat we should use “QmutableListIterator” for modifying the items in Qlist<>.

So we have try the following code .

QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);	
	while( it.hasNext() )
	{
 
		//qDebug() << it.next();
		QObject * tmpObj = it.next();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{			
			c31->deleteChild2( c22 );
			it.remove();
 
			//list.removeAll( c31 );   //If  tis line is here it works properly. 
							   // But in ourproject we cann't use this 
						   // Can i update the 'list'  , just by updatin iterator.
 
		}
 
	}

QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);	
	while( it.hasNext() )
	{
		
		//qDebug() << it.next();
		QObject * tmpObj = it.next();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{			
			c31->deleteChild2( c22 );
			it.remove();
			
			//list.removeAll( c31 );   //If  tis line is here it works properly. 
							   // But in ourproject we cann't use this 
						   // Can i update the 'list'  , just by updatin iterator.
			
		}

	}

To copy to clipboard, switch view to plain text mode 

Its working without crashing ( with list.removeAll( c31 ); ) . But as i mentioned , we cann't use “list.removeAll( c31)” in our project. Is there any other mechanism which will not crash or update the list ( without using list.removeAll( C31 ) ).


thanks in advance.

[joseph]

Hai

Here i have placed our code snippet . Now i can run the application without crashing. But i need a optimised code . Please help me..

#include <QtGui/QApplication>
#include <QDebug>
#include <QList>
 #include <QMutableListIterator>
 
class parentObject : public QObject
{	
public:
	parentObject( QObject* parent = 0 ) : QObject(parent){}
	~parentObject(){}
protected:
	void deleteSelected();
};
 
class child1 : public parentObject
{	
public:
	child1( QObject* parent = 0 ) : parentObject(parent){}
	~child1(){}
	void deleteSelect()
	{
		delete this;
	}
};
 
class child2 : public parentObject
{		
public:
	child2( QObject* parent = 0 ) : parentObject(parent)
	{}
	~child2(){	
 
	}
	void deleteSelect()
	{		
		delete this;		
	}
};
 
class child3 : public parentObject
{	
public:
	child3( QObject* parent = 0 ) : parentObject(parent){}
	~child3(){}
	void deleteChild2( child2* c )
	{
		c->deleteSelect();
		delete this;  //feby
	}	
	/*void deleteSelect()
	{
		delete this;
	}*/
};
 
int main(int argc, char *argv[])
{
	parentObject* obj = new parentObject;
	child1* c11 = new child1( obj );
	c11->setObjectName ( "child1" );
	child1* c12 = new child1(obj);
	c12->setObjectName ( "child1" );
	child1* c13 = new child1(obj);
	c13->setObjectName ( "child1" );
 
	child2* c21 = new child2(obj);
	c21->setObjectName ( "child2" );
	child2* c22 = new child2(obj);
	c22->setObjectName ( "child22" );
	child2* c23 = new child2(obj);
	c23->setObjectName ( "child2" );
 
	child3* c31 = new child3(obj);
	c31->setObjectName ( "child3" );
 
 
	QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);
	while( it.hasNext() )
	{
		QObject * tmpObj = it.next();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{			
			c31->deleteChild2( c22 );
			it.remove();
 
			list = obj->children();			// reinitialising the "list"
			it = list;					// reinitialising the "Iterator"
			it.toFront();				// move the iterator the "Front" , before first item.
 
		}
	}	
 
	return 0;
}

#include <QtGui/QApplication>
#include <QDebug>
#include <QList>
 #include <QMutableListIterator>

class parentObject : public QObject
{	
public:
	parentObject( QObject* parent = 0 ) : QObject(parent){}
	~parentObject(){}
protected:
	void deleteSelected();
};

class child1 : public parentObject
{	
public:
	child1( QObject* parent = 0 ) : parentObject(parent){}
	~child1(){}
	void deleteSelect()
	{
		delete this;
	}
};

class child2 : public parentObject
{		
public:
	child2( QObject* parent = 0 ) : parentObject(parent)
	{}
	~child2(){	

	}
	void deleteSelect()
	{		
		delete this;		
	}
};

class child3 : public parentObject
{	
public:
	child3( QObject* parent = 0 ) : parentObject(parent){}
	~child3(){}
	void deleteChild2( child2* c )
	{
		c->deleteSelect();
		delete this;  //feby
	}	
	/*void deleteSelect()
	{
		delete this;
	}*/
};

int main(int argc, char *argv[])
{
	parentObject* obj = new parentObject;
	child1* c11 = new child1( obj );
	c11->setObjectName ( "child1" );
	child1* c12 = new child1(obj);
	c12->setObjectName ( "child1" );
	child1* c13 = new child1(obj);
	c13->setObjectName ( "child1" );

	child2* c21 = new child2(obj);
	c21->setObjectName ( "child2" );
	child2* c22 = new child2(obj);
	c22->setObjectName ( "child22" );
	child2* c23 = new child2(obj);
	c23->setObjectName ( "child2" );
	
	child3* c31 = new child3(obj);
	c31->setObjectName ( "child3" );
	
	
	QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);
	while( it.hasNext() )
	{
		QObject * tmpObj = it.next();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{			
			c31->deleteChild2( c22 );
			it.remove();

			list = obj->children();			// reinitialising the "list"
			it = list;					// reinitialising the "Iterator"
			it.toFront();				// move the iterator the "Front" , before first item.
			
		}
	}	

	return 0;
}

To copy to clipboard, switch view to plain text mode 

just have a look at the code below , where i need to get it optimised.......

QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);
	while( it.hasNext() )
	{
		QObject * tmpObj = it.next();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{			
			c31->deleteChild2( c22 );
			it.remove();
 
			list = obj->children();		// reinitialising the "list"
			it = list;			// reinitialising the "Iterator"
			it.toFront();		// move the iterator the "Front" , before first item.
 
		}
	}

QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);
	while( it.hasNext() )
	{
		QObject * tmpObj = it.next();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{			
			c31->deleteChild2( c22 );
			it.remove();

			list = obj->children();		// reinitialising the "list"
			it = list;			// reinitialising the "Iterator"
			it.toFront();		// move the iterator the "Front" , before first item.
			
		}
	}

To copy to clipboard, switch view to plain text mode 

here after deleteChild2(), i am reinitialising the list, iterator. But it is not feasible( same thime it's not crashing ). Because if we want to call deleteChild2() , more times say 100. this iterating process will be killing overhead. So i want to optimise this code any how.

Can anybody help me out to resolve this ..??
Thanks

[wysota]

Well, I use that because before deleting c I have other stuffs to perform which I perform in deleteSelected function.

Why not put it into the class destructor? That's what it is for, isn't it?

The above given example is given for clear picture about where the application crashes. The code might look stupid as I just imagined and have jot down scenario happening in my project. As I said earlier, I have some objects on drawing area and I want to delete the one which the user has selected.

So why not keep a pointer directly to this object instead of searching for it in the list of all children?

The sample below has one parent Object class with 7 children. Now I iterate over the children objects and find child2 with instance 2. If found, first I delete child2 with instance 2 and then child 3. Now my control returns to for loop, then iterate again, print "child2" with instance 3( the next child ). Finally, the last child is encountered ie child3, OH!!! but that is already deleted. but the pointer is not, hence application crashes.

Do a recursion over the tree instead of using a flat list.

here after deleteChild2(), i am reinitialising the list, iterator.

This is not an option.

Why are you getting a list of all children instead of directly accessing the items you want from a particular subtree of objects? Iterating the list of all children is an overkill as well, as you can probably skip 90% of objects that you know upfront you won't need to check.

[veda]

Why not put it into the class destructor? That's what it is for, isn't it?

Yes I do exactly that. Something like this

void class: deleteSelected()
{
    if( isSelected )
       delete this; // calls the destructor 
} 
class::~class()
{
   //do other stuffs like unselecting and then setting some properties
}

void class: deleteSelected()
{
    if( isSelected )
       delete this; // calls the destructor 
} 
class::~class()
{
   //do other stuffs like unselecting and then setting some properties
}

To copy to clipboard, switch view to plain text mode 

So why not keep a pointer directly to this object instead of searching for it in the list of all children?

So why not keep a pointer directly to this object instead of searching for it in the list of all children?

To copy to clipboard, switch view to plain text mode 

Because there can be any number of objects and I dont know in advance which object the user has selected at run time, so I need to iterate through the list and see which object is selected and delete accordingly. There can be any number selected 1, 2, 3 etc objects. So I dont I can keep a pointer directly to this object.

Do a recursion over the tree instead of using a flat list.

Can you tell me how...

Isnt there any solution in the sample program that I have provided above???? Cannot I delete the pointer too in the above scenario....

wysota, please throw some light on above given solution as how joseph lightens...

Thanks....

[wysota]

Yes I do exactly that. Something like this

void class: deleteSelected()
{
    if( isSelected )
       delete this; // calls the destructor 
} 
class::~class()
{
   //do other stuffs like unselecting and then setting some properties
}

void class: deleteSelected()
{
    if( isSelected )
       delete this; // calls the destructor 
} 
class::~class()
{
   //do other stuffs like unselecting and then setting some properties
}

To copy to clipboard, switch view to plain text mode 

What's the point of checking it for selected if you don't know if it is selected (as you mention below)?

Because there can be any number of objects

So keep a list.

and I dont know in advance which object the user has selected at run time,

How come? The selection is made probably by clicking or choosing something so you can connect to the appropriate signal and add/remove the object from your list.

There can be any number selected 1, 2, 3 etc objects. So I dont I can keep a pointer directly to this object.

I suggest you do it. You'll solve many issues this way.

Can you tell me how...

More or less like this:

void deleteChild(QObject *o, const QString &type){
  if(type = o->metaObject()->className())
    delete o;
  else 
  foreach(QObject *child, o->children()){
    deleteChild(child, type);
  }
}

void deleteChild(QObject *o, const QString &type){
  if(type = o->metaObject()->className())
    delete o;
  else 
  foreach(QObject *child, o->children()){
    deleteChild(child, type);
  }
}

To copy to clipboard, switch view to plain text mode 

Then call this method on the top level object. It'll iterate all children and delete every one of them that is of type "type" (and all its children). It should work quite fast because it doesn't do any unnecessary checks on child object if it discovers that a parent object is to be deleted.

[freeskydiver]

Why not so? You go backward ...and you can kill the object anytime.

QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);
	it.toBack();
	while( it.hasPrevious() )
	{
		QObject * tmpObj = it.previous();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{         
			c31->deleteChild2( c22 );
			it.remove();
		}
	}

QList< QObject* > list = obj->children();
	QMutableListIterator<QObject*> it(list);
	it.toBack();
	while( it.hasPrevious() )
	{
		QObject * tmpObj = it.previous();
		qDebug() << tmpObj->objectName();
		if( tmpObj->objectName() == QString("child22" ) )
		{         
			c31->deleteChild2( c22 );
			it.remove();
		}
	}

To copy to clipboard, switch view to plain text mode 

[veda]

Wow!!!

I'm just jumping...Too happy....Great.

Thanks a lot and also to help me in optimizing my code....

Here is the working code. Any comments
Old code

QObjectList *list; 
 QListIterator<QObject*> *it;   
 
 QObject *obj;
 list = new QObjectList( children() );   
 if( list->count() != 0 )
  {
          it = new QListIterator<QObject*>( *list );
          while ( it->hasNext() )
           {
               obj = it->next();         
               if( obj->isA( "myObject" ) )
               ((myObject *)obj)->deleteSelected();
           }
           delete it;
   }
  delete list;

QObjectList *list; 
 QListIterator<QObject*> *it;   

 QObject *obj;
 list = new QObjectList( children() );   
 if( list->count() != 0 )
  {
          it = new QListIterator<QObject*>( *list );
          while ( it->hasNext() )
           {
               obj = it->next();         
               if( obj->isA( "myObject" ) )
               ((myObject *)obj)->deleteSelected();
           }
           delete it;
   }
  delete list;

To copy to clipboard, switch view to plain text mode 

New code

for(; it!=list.end(); ++it)
{
  if( ((myobject *)(*it))->isSelected() )
	selectedList.append( *it );
} 	
Q_FOREACH( QObject* obj, selectedList )
{
    ((myobject*)obj)->deleteSelected();
}

for(; it!=list.end(); ++it)
{
  if( ((myobject *)(*it))->isSelected() )
	selectedList.append( *it );
} 	
Q_FOREACH( QObject* obj, selectedList )
{
    ((myobject*)obj)->deleteSelected();
}

To copy to clipboard, switch view to plain text mode 

[wysota]

Why not so? You go backward ...and you can kill the object anytime.

It may fail if the order of objects in the list is that a child is before a parent in the list. Qt docs don't say anything about the order, so it's risky.

Veda: Your new code doesn't seem complete. And what's the point of having two loops that do the same?

[freeskydiver]

It may fail if the order of objects in the list is that a child is before a parent in the list.

Ok.. that's right. When you have dependent objects in your list.

Qt docs don't say anything about the order, so it's risky.

Really? Actually Qt change the list sequence only after the point of last deleting, or?

[wysota]

Ok.. that's right. When you have dependent objects in your list.

That's the reason why the original code crashed in the first place.

Really? Actually Qt change the list sequence only after the point of last deleting, or?

Note that the list order changes when QWidget children are raised or lowered. A widget that is raised becomes the last object in the list, and a widget that is lowered becomes the first object in the list.

So as you see the order is in general undefined. At least I wouldn't count on it being defined