My first Qt application - a multiple-state window with a menu

Question

This is my first Qt app and also some of my first non-trivial C++ code. The application displays a menu offering two options. Clicking either option takes you to a sub-page with some text and a back button, taking you back to the menu.

Feedback I'm especially interested in:

1. Am I doing anything weird, culturally, either in C++ culture or Qt culture?
2. Am I using the wrong tool for the job at any point, for instance in my use of QStackedLayout?
3. Is the design robust? For instance, adding buttons to the menu happens separately to adding pages to the StackedLayout. Should I create a MultiPageEnvironment class or something to handle this automatically?
4. Any major C++ or Qt no-nos?

main.cpp

#include <QApplication>

#include "MainWindow.h"

int main(int argc, char **argv)
{
    QApplication *app = new QApplication(argc, argv);
    MainWindow *mw = new MainWindow();
    mw -> show();
    return app -> exec();
}

MainWindow.cpp

#include <stdio.h>
#include <QWidget>
#include <QPushButton>
#include <QStackedLayout>

#include "MainWindow.h"
#include "MenuPage.h"
#include "SubPage.h"

MainWindow::MainWindow(QWidget *parent) : QWidget(parent)
{
    setWindowTitle("Application");
    layout = new QStackedLayout();
    MenuPage *menuPage = new MenuPage(this);
    SubPage  *page1    = new SubPage("This is page one.", this);
    SubPage  *page2    = new SubPage("This is page two.", this);

    layout -> addWidget(menuPage);
    layout -> addWidget(page1);
    layout -> addWidget(page2);
    layout -> setCurrentIndex(0);
    this   -> setLayout(layout);

    menuPage -> addButton();
    menuPage -> addButton();
}

void MainWindow::setPage(int index)
{
    layout -> setCurrentIndex(index);
}

void MainWindow::goToMenu()
{
    layout -> setCurrentIndex(0);
}

MenuPage.cpp

#include <stdio.h>
#include <QWidget>
#include <QPushButton>
#include <QSignalMapper>

#include "MenuPage.h"

MenuPage::MenuPage(MainWindow *parent) : QWidget(parent)
{
    signalMapper = new QSignalMapper(this);
    connect(signalMapper, SIGNAL(mapped(int)), parent, SLOT(setPage(int)));

    layout = new QVBoxLayout();
    setLayout(layout);

    nButtons = 0;
}

void MenuPage::addButton()
{
    nButtons += 1;

    QString label = (new QString("Page %1")) -> arg(nButtons);
    QPushButton *button = new QPushButton(label, this);
    layout -> addWidget(button);

    signalMapper -> setMapping(button, nButtons);
    connect(button, SIGNAL(clicked()), signalMapper, SLOT(map()));
}

SubPage.cpp

#include <stdio.h>
#include <QWidget>
#include <QPushButton>
#include <QVBoxLayout>

#include "SubPage.h"
#include "MainWindow.h"

SubPage::SubPage(const char messageString[], MainWindow *parent) : QWidget(parent)
{
    message = new QLabel(messageString, this);
    backButton = new QPushButton("Back", this);

    QVBoxLayout *layout = new QVBoxLayout();
    layout -> addWidget(message);
    layout -> addWidget(backButton);
    setLayout(layout);

    connect(backButton, SIGNAL(clicked()), parent, SLOT(goToMenu()));
}

Answer 1

That's a good first Qt program. Most of my comments are preferences rather than rules, but I'll try to provide justifications.

In main(), no need to new

I would write

int main(int argc, char **argv)
{
    QApplication app(argc, argv);
    MainWindow mw;
    mw.show();
    return app.exec();
}

That way, you don't need to remember to delete the objects app and mw. (You can argue that there's no need to clean up when you're exiting, but I find it helps to avoid false-positives when I'm checking memory correctness with Valgrind).

In constructors, prefer to use initializers

I can write

MainWindow::MainWindow(QWidget *parent)
    : QWidget(parent),
      layout(new QStackedLayout(this))
{

This allows us to declare layout as a const pointer, for increased safety:

QStackedLayout *const layout;

Note that I've passed this to the QStackedLayout constructor: that ensures that it always has an owner that's responsible for deleting it, and it installs it as the layout manager for the MainWindow widget (saving us a line of code).

Use new-style connect() when you can

Unless you don't know the complete type of the objects being connected, you can connect by pointer-to-member-function. This allows more errors to be caught at compilation time (a Good Thing):

MenuPage::MenuPage(MainWindow *parent)
    : QWidget(parent),
      layout(new QVBoxLayout(this)),
      signalMapper(new QSignalMapper(this))
{
    void(QSignalMapper::*signal)(int) = &QSignalMapper::mapped;
    connect(signalMapper, signal, parent, &MainWindow::setPage);
}

However, I'd move the signal mapper and connect() call up into MainWindow, so that MenuPage doesn't need to know about its parent.

Reduce coupling

Both MainWindow and MenuPage need to know about MainWindow in their constructors. Although we can simply pass a QObject, and connect by name, there's still coupling because they impose requirements on the slots exposed, so they can't be used more generally.

The way to fix this coupling is for the pages to emit a signal to indicate user actions; the MainWindow (which knows about its contents) can then connect those signals to the relevant actions.

MenuPage then looks like

public:
    explicit MenuPage(QWidget *parent);
signals:
    void buttonPressed(int button_number);

and its constructor is now

MenuPage::MenuPage(QWidget *parent)
    : QWidget(parent),
      layout(new QVBoxLayout(this)),
      signalMapper(new QSignalMapper(this))
{
    void(QSignalMapper::*signal)(int) = &QSignalMapper::mapped;
    connect(signalMapper, signal, this, &MenuPage::buttonPressed);
}

(The temporary variable signal is to avoid the problem of connecting overloaded signals and slots in Qt 5).

In MainWindow, we connect it directly:

connect(menuPage, &MenuPage::buttonPressed, layout, &QStackedLayout::setCurrentIndex);

We can do something similar in SubPage, but this is a little simpler, as we don't have an overloaded signal. So just create a signal (call it done(), perhaps) and make the page's constructor connect the button's clicked() to the page's done().

A memory leak

Here, we allocate a new QString, which is never released:

QString label = (new QString("Page %1")) -> arg(nButtons);

It's easy to fix this - QString objects are fine to pass by value:

QString label = QString(tr("Page %1")).arg(nButtons);

Allow for translation

If you get into the habit of using tr() on all your user-facing strings, you've made a good start on internationalizing your application

---

Full worked example

I've inlined the methods just to make this smaller - I don't expect that in a real, multiple-source-file program.

#include <QWidget>
#include <QPushButton>
#include <QStackedLayout>
#include <QVBoxLayout>
#include <QSignalMapper>
#include <QLabel>

class MenuPage : public QWidget
{
    Q_OBJECT

public:
    explicit MenuPage(QWidget *parent)
        : QWidget(parent),
          layout(new QVBoxLayout(this)),
          signalMapper(new QSignalMapper(this))
    {
        void(QSignalMapper::*signal)(int) = &QSignalMapper::mapped;
        connect(signalMapper, signal, this, &MenuPage::buttonPressed);
    }

    void addButton()
    {
        ++nButtons;

        QString label = QString(tr("Page %1")).arg(nButtons);
        QPushButton *button = new QPushButton(label, this);
        layout->addWidget(button);

        signalMapper->setMapping(button, nButtons);
        connect(button, SIGNAL(clicked()), signalMapper, SLOT(map()));
    }

signals:
    void buttonPressed(int button_number);

private:
    QVBoxLayout *const layout;
    QSignalMapper *const signalMapper;
    int nButtons = 0;
};

class SubPage : public QWidget
{
    Q_OBJECT

public:
    explicit SubPage(const QString& messageText, QWidget *parent)
        : QWidget(parent),
          message(new QLabel(messageText, this)),
          backButton(new QPushButton(tr("Back"), this))
    {
        QVBoxLayout *const layout = new QVBoxLayout(this);
        layout->addWidget(message);
        layout->addWidget(backButton);
        connect(backButton, &QPushButton::clicked, this, &SubPage::done);
    }

signals:
    void done();

private:
    QLabel *const message;
    QPushButton *const backButton;
};

class MainWindow : public QWidget
{
    Q_OBJECT

public:
    explicit MainWindow(QWidget *parent = 0)
        : QWidget(parent),
          layout(new QStackedLayout(this))
    {
        setWindowTitle(tr("Application"));
        MenuPage *const menuPage = new MenuPage(this);
        SubPage  *const page1    = new SubPage(tr("This is page one."), this);
        SubPage  *const page2    = new SubPage(tr("This is page two."), this);

        layout->addWidget(menuPage);
        layout->addWidget(page1);
        layout->addWidget(page2);
        layout->setCurrentIndex(0);

        menuPage->addButton();
        menuPage->addButton();
        connect(menuPage, &MenuPage::buttonPressed, layout, &QStackedLayout::setCurrentIndex);
        connect(page1, &SubPage::done, this, &MainWindow::goToMenu);
        connect(page2, &SubPage::done, this, &MainWindow::goToMenu);
    }

private:
    QStackedLayout *const layout;
public slots:
    void setPage(int index)
    {
        layout->setCurrentIndex(index);
    }

    void goToMenu()
    {
        layout->setCurrentIndex(0);
    }
};

#include <QApplication>

int main(int argc, char **argv)
{
    QApplication app(argc, argv);
    MainWindow mw;
    mw.show();
    return app.exec();
}

#include "165088_moc.cpp"