PDA

View Full Version : Destructor overriding



Daylight
28th February 2013, 15:35
I examined the Diagram Scene Example and faced this code:


void DiagramItem::removeArrows()
{
foreach (Arrow *arrow, arrows) {
arrow->startItem()->removeArrow(arrow);
arrow->endItem()->removeArrow(arrow);
scene()->removeItem(arrow);
delete arrow;
}
}


Where Arrow is:

class Arrow : public QGraphicsLineItem
My question is, whether it is better to implement DiagramItem::removeArrows() method that way instead of overriding the Arrow's destructor:

Arrow::~Arrow() //Virtual destructor
{
startItem()->removeArrow(this);
endItem()->removeArrow(this);

}

Then DiagramItem::removeArrows() would look like:

void DiagramItem::removeArrows()
{
foreach (Arrow *arrow, arrows) {
scene()->removeItem(this);
delete arrow;
}
}

wysota
28th February 2013, 16:28
As long as the method is not virtual -- yes.

Daylight
28th February 2013, 16:51
As long as the method is not virtual -- yes.
Why? It seems to me that destructor overriding might prevent potential code duplication if one would need to delete arrows in other places. It will also prevent from potential repeated deletion, if one deleted an arrow, but forgot to clean the pointers in endItem() startItem() and objects. Cleaning of the arrow pointers in outer objects must occur every time the arrow is deleted, so why not implement it in destructor?
Overriding of destructor will allow to make Arrow::stratItem() and Arrow::endItem() methods private.

wysota
28th February 2013, 16:58
Why?
Why what? :)

Daylight
28th February 2013, 17:01
Why is it better to implement separate DiagramItem::removeArrows() function with explicit pointer cleaning outside of Arrow object (like it is done in Diagram Scene Example)?

wysota
28th February 2013, 17:09
I didn't say it was better or worse. Both are acceptable solutions as far as they serve your goal. As long as you don't call a virtual method on oneself from its own destructor, everything is fine. Consider the fact that QGraphicsItem has its own destructor that will remove all its children and remove itself from the scene. It will not detach it from items that are not part of its parent-child relationship. Your destructor has this problem that it an arrow is not attached to an item (e.g. the item is destroyed before the arrow is removed), your program will crash. In the original code an arrow can function without an object attached to it since it is a "stupid" object that doesn't care about its environment.

Daylight
28th February 2013, 17:24
I didn't say it was better or worse. Both are acceptable solutions as far as they serve your goal. As long as you don't call a virtual method on oneself from its own destructor, everything is fine. Consider the fact that QGraphicsItem has its own destructor that will remove all its children and remove itself from the scene. It will not detach it from items that are not part of its parent-child relationship. Your destructor has this problem that it an arrow is not attached to an item (e.g. the item is destroyed before the arrow is removed), your program will crash. In the original code an arrow can function without an object attached to it since it is a "stupid" object that doesn't care about its environment.
If the item is destroyed before the arrow is removed, the program will crash anyway, because the other item will try to access destroyed item via Arrow::start/endItem(). It is not possible, though, because every item stores the list of attached arrows and removes them before destruction.
But, youre right, it seems it is better to make arrow a child of this->startItem(), this is more safe. Example's objects architecture seems a bit odd to me...

wysota
28th February 2013, 22:17
No, making the arrow a child of any item is not a good idea. Think what happens if you try to rotate or scale such item.

Daylight
1st March 2013, 00:15
No, making the arrow a child of any item is not a good idea. Think what happens if you try to rotate or scale such item.

It should be fine. Arrows rendering is treated in Arrow:: paint(), so it should be ok.

wysota
1st March 2013, 01:18
It will not be fine. The arrow will be rotated or scaled as well.

Daylight
1st March 2013, 10:55
It will not be fine. The arrow will be rotated or scaled as well.
Why would it be? It will be repainted (correctly) each time the parent object is changed. The painting is done with respect to start/endItem() coordinates using the mapFromItem(myStart/endItem, 0, 0).

wysota
1st March 2013, 11:02
The painting is done with respect to start/endItem() coordinates using the mapFromItem(myStart/endItem, 0, 0).
Coordinates of the items will not change if they are rotated or scaled. If you counter that then you counter everything GraphicsView stands for. The arrow simply can't be a child of an item if it is supposed to be independent of the item. Besides it is attached to two items and it can't have two parents.

Daylight
1st March 2013, 11:09
Coordinates of the items will not change if they are rotated or scaled. If you counter that then you counter everything GraphicsView stands for. The arrow simply can't be a child of an item if it is supposed to be independent of the item. Besides it is attached to two items and it can't have two parents.
From the logical point of view, arrow may be a child of an item it starts from. It is not completely independent, it should always be connected to two items. Arrows can not be moved directly, only by moving start/end Items.
This is from the example:


void Arrow::updatePosition()
{
QLineF line(mapFromItem(myStartItem, 0, 0), mapFromItem(myEndItem, 0, 0));
setLine(line);
}

wysota
1st March 2013, 12:05
Ok, here is a small framework. Try completing it so that it works properly.


#include <QtGui>
#if QT_VERSION >= 0x050000
#include <QtWidgets>
#endif

class Arrow;

class RotatingItem : public QGraphicsRectItem {
public:
RotatingItem(const QRectF &rect, QGraphicsItem *parent = 0) : QGraphicsRectItem(rect, parent){
setFlag(ItemSendsGeometryChanges, true);

}
protected:
void wheelEvent(QGraphicsSceneWheelEvent *event) {
if(event->modifiers() & Qt::ShiftModifier) {
setScale(scale()* (event->delta() > 0 ? 1.1 : 0.9));
} else {
setRotation(rotation()+(event->delta()/15.0));

}
}
QVariant itemChange(GraphicsItemChange change, const QVariant &value) {
if(change == ItemPositionHasChanged && scene()) {
foreach(Arrow *a, m_arrowStartsHere) updateStartArrowPosition(a);
foreach(Arrow *a, m_arrowEndsHere) updateEndArrowPosition(a);

}
return QGraphicsRectItem::itemChange(change, value);
}
void updateStartArrowPosition(Arrow *);

void updateEndArrowPosition(Arrow *);

private:
QList<Arrow*> m_arrowStartsHere;
QList<Arrow*> m_arrowEndsHere;


friend class Arrow;
};

class Arrow : public QGraphicsLineItem {
public:
Arrow(RotatingItem *st, RotatingItem *en) :QGraphicsLineItem(st) {
startItem = st;
endItem = en;
QPointF end = mapFromItem(endItem, endItem->boundingRect().center());
setLine(0,0, end.x(), end.y());
startItem->m_arrowStartsHere << this;
endItem->m_arrowEndsHere << this;

m_text = new QGraphicsSimpleTextItem(this);
QFont f;
f.setPointSize(8);
m_text->setFont(f);
updateArrowContent();
}
RotatingItem *start() const { return startItem; }
RotatingItem *end() const { return endItem; }
void updateArrowContent() {
m_text->setPos(line().pointAt(0.5));
m_text->setText(QString::number(line().length()));
}

protected:
RotatingItem *startItem, *endItem;
QGraphicsSimpleTextItem *m_text;
};

class GraphicsView : public QGraphicsView {
public:
GraphicsView(QWidget *parent = 0) : QGraphicsView(parent){}
protected:
void paintEvent(QPaintEvent *event) {
QGraphicsView::paintEvent(event);
QPainter p(viewport());
p.drawText(viewport()->rect().adjusted(10,10,-10,-10), "Drag item to move\nWheel to rotate\nSHIFT+wheel to scale");
}
};

int main(int argc, char **argv) {
QApplication app(argc, argv);
GraphicsView view;
QGraphicsScene scene;
view.setScene(&scene);
RotatingItem *item1 = new RotatingItem(QRect(-50, -50, 100, 100));
RotatingItem *item2 = new RotatingItem(QRect(-50, -50, 100, 100));
scene.addItem(item1);
scene.addItem(item2);
item1->setPos(100,100);
item2->setPos(200,200);
item1->setFlag(QGraphicsItem::ItemIsMovable);
item2->setFlag(QGraphicsItem::ItemIsMovable);
item1->setBrush(Qt::red);
item2->setBrush(Qt::yellow);
Arrow *arr = new Arrow(item1, item2);
QPen p;
p.setWidth(4);
p.setColor(Qt::blue);
arr->setPen(p);
item1->setZValue(10);
item2->setZValue(0);
arr->setZValue(10);
view.setRenderHint(QPainter::Antialiasing);
view.resize(400,400);
view.show();
return app.exec();
}


void RotatingItem::updateStartArrowPosition(Arrow *arrow)
{
QLineF l = arrow->line();
l.setP2(arrow->end()->mapToItem(arrow, arrow->end()->boundingRect().center()));
arrow->setLine(l);
arrow->updateArrowContent();
}

void RotatingItem::updateEndArrowPosition(Arrow *arrow)
{
QLineF l = arrow->line();
l.setP2(mapToItem(arrow, boundingRect().center()));
arrow->setLine(l);
arrow->updateArrowContent();
}

Problems to solve:
1. make sure the line sticks to its items when rotation and scale of any of them changes
2. make sure the length reported by the line is consistent when the line's starting item is scaled
3. make sure the width of the line doesn't depend on the scale of the red item or that it depends on the scale of both the red and the yellow item (the choice is yours)
4. make it possible for the yellow item to be over the red item (in terms of Z-value) and the line to be on top of the yellow item (in other words make it possible so that the line is always on top of the other items regardless of the stacking order of the items themselves)