Rosegarden Coding Style: Qt Layouts

When doing new work in Rosegarden, it is perfectly acceptable to use Qt Designer if you prefer. One good example of a new Designer-based dialog is the MIDI device manager. However, most of our layout code is hand-written Qt, and most of us have seemed to prefer this approach over the years, so we will probably go on having a high proportion of hand-written layout code.

Much of our existing layout code started when KDE and Qt were at version 2.x, and it has survived the transition to Qt and KDE 3.x and then finally to Qt 4.x. Adding to the amount of legacy cruft present in this code, much of the transition to Qt 4 was done by automatic conversion scripts. This has left us with a lot of layout code that gets the job done, but it is a terrible mess, and full of all kinds of inconsistencies.

After doing a lot of work on these legacy layouts, I think it is time to set out some guidelines for new work so that people will be encouraged to follow best practices going forward, and not look back at bad old examples. Some of the strange things I've been dealing with have been from scripts that could only do so much automated work, and some it is actually from the initial conversion guidelines that were published at the start of the porting effort, which recommended practices that seem bizarre and confusing to me today.

It is true that in order to add a layout in Qt 4 you have to have a QWidget, but if you have a QDialog, you have a QWidget. You do not need to create a new QWidget inside the dialog and add a layout to that.

We have a lot of old legacy code like:

MyDialog::MyDialog(QWidget *parent) :
        QDialog(parent)
{
    QGridLayout *metagrid = new QGridLayout;
    setLayout(metagrid);
    QWidget *vBox = new QWidget(this);
    QVBoxLayout *vBoxLayout = new QVBoxLayout;
    metagrid->addWidget(vBox, 0, 0);
    ...
}

This code creates a layout of type QGridLayout and sets it to be the layout for the QDialog object, then it creates a new QWidget called vBox, and sets up a QVBox layout inside it. This extra complication is not necessary, and it should be avoided in future code. We can reduce all of this to:

MyDialog::MyDialog(QWidget *parent) :
        QDialog(parent)
{
    QVBoxLayout *vBoxLayout = new QVBoxLayout;
    setLayout(vBoxLayout);
    ...
}

We have a lot of legacy code like this snippet:

    QFrame *frame = new QFrame( vBox );
    vBoxLayout->addWidget(frame);
 
    QGridLayout *layout = new QGridLayout(frame);
 
    m_button = new QPushButton(tr("Button"), vBox);
    layout->addWidget(m_button);

All of this vBox begat frame begat QPushButton code can be removed for simplicity. It is rarely necessary or beneficial to supply all these optional parent parameters. This has many practical advantages, and has been greatly helpful to me in resolving broken layouts during this restructuring work. If an existing layout ain't broke, there is little point in fixing it, but please do not write code like this in new layouts. The example above will work perfectly fine when reduced to:

    QFrame *frame = new QFrame;
    vBoxLayout->addWidget(frame);
 
    QGridLayout *layout = new QGridLayout;
 
    m_button = new QPushButton(tr("Button"));
    layout->addWidget(m_button);

Furthermore, it seems to be widely accepted Qt 4 idiom to create most objects with no or few initialization parameters, and then use member functions to set things up after creation. This makes for very clean, clear code that is easy to follow.

[Real example TBA, I'm looking for something like

m_foo = new QWhatzit(myParent, height, width, favoritePoison, name);

becoming

m_foo = new QWhatzit; m_foo→setHeight(height); m_foo→setWidth(width); m_foo→setFavoritePoison(BEEEEEEEEEEEER);

whenever I run across a real one in the code]

This kind of thing is driving me closer and closer to going insane the more often I deal with it.

Somebody promoted the practice of calling setLayout() on a widget only after populating that layout with widgets. We have widget creation here, and then 100 lines down, the widget gets a layout, usually in code where some other widget was just created (which widget won't get its own layout until much further down in turn.)

I have some vague idea that someone believed you couldn't set a layout on something until the layout was populated. That is not true at all. Go ahead and set the layout immediately, then populate it later.

Here is an unusually simple and compact example to follow, with extra comments added:

    QGridLayout *metagrid = new QGridLayout;
    setLayout(metagrid);
 
    QWidget *vBox = new QWidget(this);          // vBox gets created here
    QVBoxLayout *vBoxLayout = new QVBoxLayout;  // vBox gets a layout created
    metagrid->addWidget(vBox, 0, 0);   
 
 
    QFrame *frame = new QFrame( vBox );         // population of vBox starts here
    vBoxLayout->addWidget(frame);
    vBox->setLayout(vBoxLayout);                // vBox gets a layout set now

This is much easier to follow when:

    QGridLayout *metagrid = new QGridLayout;
    setLayout(metagrid);
 
    QWidget *vBox = new QWidget(this);          // vBox gets created here
    QVBoxLayout *vBoxLayout = new QVBoxLayout;  // vBox gets a layout created
    vBox->setLayout(vBoxLayout);                // vBox gets a layout set now
    metagrid->addWidget(vBox, 0, 0);   
 
 
    QFrame *frame = new QFrame( vBox );         // population of vBox starts here
    vBoxLayout->addWidget(frame);

Before:

PitchPickerDialog::PitchPickerDialog(QWidget *parent, int initialPitch, QString text) :
        QDialog(parent)
{
    setModal(true);
    setWindowTitle(tr("Pitch Selector"));
 
    QGridLayout *metagrid = new QGridLayout;
    setLayout(metagrid);
    QWidget *vBox = new QWidget(this);
    QVBoxLayout *vBoxLayout = new QVBoxLayout;
    metagrid->addWidget(vBox, 0, 0);
 
 
    QFrame *frame = new QFrame( vBox );
    vBoxLayout->addWidget(frame);
    vBox->setLayout(vBoxLayout);
 
    frame->setContentsMargins(10, 10, 10, 10);
    QGridLayout *layout = new QGridLayout(frame);
    layout->setSpacing(5);
 
    m_pitch = new PitchChooser(text, frame, initialPitch);
    layout->addWidget(m_pitch, 0, 0, 0- 0+1, 2-0+ 1, Qt::AlignHCenter);
    QDialogButtonBox *buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
    metagrid->addWidget(buttonBox, 1, 0);
    metagrid->setRowStretch(0, 10);
    connect(buttonBox, SIGNAL(accepted()), this, SLOT(accept()));
    connect(buttonBox, SIGNAL(rejected()), this, SLOT(reject()));
}

After:

// We keep the parent parameter here, because parents control the location at
// which a dialog pops up, and they influence style inheritance.
PitchPickerDialog::PitchPickerDialog(QWidget *parent, int initialPitch, QString text) :
        QDialog(parent)
{
    setModal(true);
    setWindowTitle(tr("Pitch Selector"));
 
    QVBoxLayout *vBoxLayout = new QVBoxLayout;
    setLayout(vBoxLayout);
 
    QFrame *frame = new QFrame;
    vBoxLayout->addWidget(frame);
 
    frame->setContentsMargins(10, 10, 10, 10);
    QGridLayout *frameLayout = new QGridLayout;
    frameLayout->setSpacing(5);
    frame->setLayout(frameLayout);
 
    m_pitch = new PitchChooser(text, frame, initialPitch);         // internal class still needs a parent
    frameLayout->addWidget(m_pitch, 0, 0, 1, 3, Qt::AlignHCenter); // simplified conversion legacy 0-0+1
                                                                   // == 1; 2-0+1 == 3
 
    // Since we're stacking this in a VBox, we can just stack the button box on
    // the bottom layer of the main layout, without any top level grid.
    QDialogButtonBox *buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
    vBoxLayout->addWidget(buttonBox);
 
    connect(buttonBox, SIGNAL(accepted()), this, SLOT(accept()));
    connect(buttonBox, SIGNAL(rejected()), this, SLOT(reject()));
}

And here's a diff:

Index: src/gui/dialogs/PitchPickerDialog.cpp
===================================================================
--- src/gui/dialogs/PitchPickerDialog.cpp       (revision 10438)
+++ src/gui/dialogs/PitchPickerDialog.cpp       (working copy)
@@ -29,32 +29,33 @@
 namespace Rosegarden
 {
 
+// We keep the parent parameter here, because parents control the location at
+// which a dialog pops up, and they influence style inheritance.
 PitchPickerDialog::PitchPickerDialog(QWidget *parent, int initialPitch, QString text) :
         QDialog(parent)
 {
     setModal(true);
     setWindowTitle(tr("Pitch Selector"));
 
-    QGridLayout *metagrid = new QGridLayout;
-    setLayout(metagrid);
-    QWidget *vBox = new QWidget(this);
     QVBoxLayout *vBoxLayout = new QVBoxLayout;
-    metagrid->addWidget(vBox, 0, 0);
+    setLayout(vBoxLayout);
 
-
-    QFrame *frame = new QFrame( vBox );
+    QFrame *frame = new QFrame;
     vBoxLayout->addWidget(frame);
-    vBox->setLayout(vBoxLayout);
 
     frame->setContentsMargins(10, 10, 10, 10);
-    QGridLayout *layout = new QGridLayout(frame);
-    layout->setSpacing(5);
+    QGridLayout *frameLayout = new QGridLayout;
+    frameLayout->setSpacing(5);
+    frame->setLayout(frameLayout);
 
-    m_pitch = new PitchChooser(text, frame, initialPitch);
-    layout->addWidget(m_pitch, 0, 0, 0- 0+1, 2-0+ 1, Qt::AlignHCenter);
+    m_pitch = new PitchChooser(text, frame, initialPitch);         // internal class still needs a parent
+    frameLayout->addWidget(m_pitch, 0, 0, 1, 3, Qt::AlignHCenter); // simplified conversion legacy 0-0+1 == 1; 2-0+1 == 3
+
+    // Since we're stacking this in a VBox, we can just stack the button box on
+    // the bottom layer of the main layout, without any top level grid.
     QDialogButtonBox *buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
-    metagrid->addWidget(buttonBox, 1, 0);
-    metagrid->setRowStretch(0, 10);
+    vBoxLayout->addWidget(buttonBox);
+
     connect(buttonBox, SIGNAL(accepted()), this, SLOT(accept()));
     connect(buttonBox, SIGNAL(rejected()), this, SLOT(reject()));
 }