Properly deleting QGraphicsScene

Question

I created a small prototype which contains a QGraphicsView that I bind to a GraphicsScene to which I can add or remove QGraphicsTextItem. Here is the the cpp file that does the job

#include "mainwindow.h"
#include "ui_mainwindow.h"

#include <QGraphicsScene>
#include <QGraphicsTextItem>

MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::MainWindow)
{
    ui->setupUi(this);
    QGraphicsScene* scene = new QGraphicsScene();
    ui->graphicsView->setScene(scene);
    connect(ui->pushButton,SIGNAL(clicked()),this,SLOT(addGraphicsItem()));
    connect(ui->pushButton_2,SIGNAL(clicked()),this,SLOT(removeGraphicsItem()));
}

MainWindow::~MainWindow()
{
    auto scene = ui->graphicsView->scene();
    ui->graphicsView->setScene(nullptr);
    delete scene;
    delete ui;
}

void MainWindow::addGraphicsItem()
{
    QGraphicsTextItem* item = new QGraphicsTextItem("jgfkdljkdj");
    _items.push_back(item);
    ui->graphicsView->scene()->addItem(_items.back());
}

void MainWindow::removeGraphicsItem()
{
    auto item = _items.back();
    ui->graphicsView->scene()->removeItem(item);
    _items.erase(_items.end()-1);
    delete item;
}

I have trouble to understand how to manage safely my memory when creating/deleting the scene and/or its underlying items. Reading many posts about this such as this one, I came up to the aformentionned code but I have the feeling that it is overkilling code and that Qt might do the job without it. Is that really the way to do especially in MainWindow::removeGraphicsItem slot when removing and deleting one item from the scene and MainWindow::~MainWindow destructor when deleting the scene ?

Answer 1

The simplest thing to do is to let the language and the framework manage the memory for you.

Store QGraphicsScene by value, and leverage the fact that the scene is a resource manager, tightly coupled to the items. It manages the lifetime of the items and guarantees that no items will outlive the scene, i.e. it will take care of disposing of any items that weren't disposed before its destructor runs.

To remove an item from the scene, or from a parent item, simply delete it. The item will inform the scene and any parents that it's about to vanish, and the scene will remove it from its list of items. That's how QObject memory management works as well.

You may also wish to use QGraphicsScene::items or QGraphicsItemGroup::childItems instead of managing the object list manually. Using a manual list requires paying attention to synchronizing the list's contents with object lifetimes. A scene or an item group does it automatically.

If were to write it, I'd do it as follows. I also show how to mix different item lifetimes in a scene, and how to hold items by value.

// mainwindow.h
#pragma once
#include "ui_mainwindow.h"
#include <QGraphicsScene>

class MainWindow : public QMainWindow {
  Q_OBJECT
  Ui::MainWindow ui;
  QGraphicsScene m_scene;
  QGraphicsTextItem m_text{tr("foo")};
  QGraphicsItemGroup m_dynamicItems;
  template <typename T, typename ...Ar> T* newItem(Ar&&... args);
public:
  MainWindow(QWidget *parent = {});
  Q_SLOT void addItem();
  Q_SLOT void removeItem();
  Q_SLOT void removeAllItems();
};

// mainwindow.cpp
#include "mainwindow.h"
#include <utility>

template <typename T, typename ...Ar> 
T* MainWindow::newItem(Ar&&... args) {
  return new T{&this->m_dynamicItems, std::forward<Ar>(args)...};
}

MainWindow::MainWindow(QWidget *parent) :
  QMainWindow(parent) 
{
  ui.setupUi(this);
  ui.graphicsView->setScene(&m_scene);
  m_scene.addItem(&m_text);
  m_scene.addItem(&m_dynamicItems);
  auto const clicked = &QPushButton::clicked;
  connect(ui.pushButton, clicked, this, &MainWindow::addItem);
  connect(ui.pushButton2, clicked, this, &MainWindow::removeItem);
}

void MainWindow::addItem() {
  newItem<QGraphicsTextItem>(tr("jfslkdfjd"));
}

void MainWindow::removeItem() {
  auto const &items = std::as_const(m_dynamicItems.childItems());
  if (!items.isEmpty())
    delete items.back();
}

void MainWindow::removeAllItems() {
  for (auto *item : std::as_const(m_dynamicItems.childItems()))
    delete item;
  // or (deprecated but you may run into such code)
  qDeleteAll(std::as_const(m_dynamicItems.childItems()));
}