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, but probably seemed like sensible best guesses to make at a time when all of this was months away from compiling, and there was no way to get immediate feedback and try different ideas.

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 (for example), 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. (QLayout::addItem() which is eventually called by addWidget() will take ownership of the widget. So there is no danger of memory leaks.) 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);

In the past, it used to be common to create Qt objects with all kinds of parameters, including an optional name. We have an abundance of code like this example:

    m_dialogButtonBox = new QDialogButtonBox ( sbuttons, Qt::Horizontal, this );
    m_dialogButtonBox->setObjectName ( "dialog_base_button_box" );
    m_mainLayout->addWidget ( m_dialogButtonBox );

The QDialogButtonBox is created with three parameters, and then the object's name is set with a follow-up call to setObjectName() that looks like it is probably the result of automatic conversion, as none of the modern ctors for this class take a name parameter:

  QDialogButtonBox ( QWidget * parent = 0 )
  QDialogButtonBox ( Qt::Orientation orientation, QWidget * parent = 0 )
  QDialogButtonBox ( StandardButtons buttons, Qt::Orientation orientation = Qt::Horizontal, QWidget * parent = 0 )

We have never done much with the name property, and it is just as well whenever it happens to have been split out of a declaration like this one. Most objects we have named throughout our code have names that serve no particular purpose. Of the few exceptions to this rule of thumb, almost all meaningful object names are related to our new stylesheet-based look.

I suggest using the excellent ack utility to search the code tree for occurrences of any questionable name. In this case, it looks like “dialog_base_button_box” used to be named for some purpose that is probably no longer valid:

src/gui/dialogs/ConfigureDialogBase.cpp
73:    m_dialogButtonBox->setObjectName( "dialog_base_button_box" );
231:    //QDialogButtonBox *dbb = findChild<QDialogButtonBox *>( "dialog_base_button_box" );

src/gui/studio/SynthPluginManagerDialog.cpp
128:        m_dialogButtonBox->setObjectName ( "dialog_base_button_box" );

The call to findChild has been commented out without causing any observed problems in the past 656 revisions. Still, just in case, we'll leave it for now and move on.

Of the remaining parameters in the declaration:

    m_dialogButtonBox = new QDialogButtonBox ( sbuttons, Qt::Horizontal, this );

We can and should remove the parent entirely, and we could use setStandardButtons() and setOrientation(). It is not such a big deal to refactor changes like these last two in current code, but it is a good idea to use this kind of idiom in the future. The entire code snippet, then, could be written out like this:

    m_dialogButtonBox = new QDialogButtonBox;
    m_dialogButtonBox->setObjectName("dialog_base_button_box");
    m_dialogButtonBox->setStandardButtons(sbuttons);
    m_dialogButtonBox->setOrientation(Qt::Horizontal);
    m_mainLayout->addWidget(m_dialogButtonBox);

It is true that this requires more typing, but it saves having to browse Qt documentation to find out what the parameters are when revisiting old code, and it doesn't take code very long at all to become old in terms of remembering what does what. Just in the six or so months I've been most deeply involved with all this layout work, I've really learned to appreciate the examples where I did the kind of refactoring I'm promoting as the good practice standard here.

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