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.

enter image description here

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()));
}

• yuri: Does Qt depend on something from stdio.h or why is it included?
• Jack M: @yuri Missed that. Leftover from earlier stages of code.
• Toby Speight: You seem to have omitted MainWindow.h, MenuPage.h and SubPage.h. Without those, the code won't compile.
• Jack M: @TobySpeight I have them of course, but I thought it would be superfluous to post them and that it would just clutter up the question. It didn't occur to me somebody might want to compile it. Here they are: pastebin.com/8Ea8rWaW
• Toby Speight: It's nice to have complete code - I can then show you ideas with full confidence that they work. Thanks for the pointer (in my answer, I've just presented everything as a single file, including the declarations).

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"

• Jack M: In the SubPage class, where there's only one button, and so it doesn't make sense to use a QSignalMapper, how should I map the button's clicked() signal onto some backButtonPressed() signal that can be accessed from outside of the class without making the button a public attribute?
• Toby Speight: If you look in the worked example, I added a done() signal, and connect the button's clicked() to it (yes, you can connect a signal to another signal;there's a performance cost, but not perceptible to an interactive user).
• Jack M: Oh, great! I was connecting clicked() to an intermediate slot on SubPage that served only to emit a new signal, but this is much simpler. I hadn't noticed at first that you included code for SubPage as well.