How to better implement my C++ design pattern?

Question

I looked up all the different compiler errors that my design seems to cause. All the answers and fixes make sense but I reached a point where fixing one thing raises another bad thing.

My C++ project is getting bigger and bigger so I'm trying to generalize my problem to benefit from the experienced C++ developers out there.

The software I am writing is an XML-parser, that creates UI objects in runtime. I designed ContainerInterface for the different types of containers I want to use. For example TabWidget is a subclassed QTabWidget and it also inherits ContainerInterface. For now, those are AbsoluteWidget, TreeWidget and TabWidget. All having implementations for the following pure virtual functions defined in ContainerInterface:

virtual PushButton* createButton(const QString& label, const QString& define, const QPoint& topLeft, const QSize& size) = 0;

virtual CheckBox* createCheckBox(const QString& label, const QString& define, const QString& header, const QPoint& topLeft, const QSize& size) = 0;

virtual ComboBox* createComboBox(const QString& label, const QString& define, const QString& header, const QPoint& topLeft, const QSize& size) = 0;

virtual Image* createImage(const QString& file, const QString& define, const QPoint& topLeft, const QSize& size) = 0;

virtual Led* createLed(const QString& define, const QString& onColor, const QString& offColor, const QPoint& topLeft, const QSize& size) = 0;

virtual Text* createText(const QString& define, const QString& label, const QPoint& topLeft, const QSize& size) = 0;

So in the parser, I can use the ContainerInterface, for example:

void XmlReader::readCheckBox(ContainerInterface* container, const QString& header)
{
    Q_ASSERT(xml.isStartElement() && xml.name() == "checkbox");
    QXmlStreamAttributes attr = xml.attributes();
    CheckBox* checkBox = container->createCheckBox(getLabel(attr), getDefine(attr), getHeader(attr, header), getTopLeft(attr), getSize(attr));
    m_centralWidget->setUIElement(getDefine(attr), checkBox); //this is why i need a return value anyway
}

This saved me a lot of code and works nice. So i would like the ContainerInterface to also have:

virtual TabWidget* createTabWidget(const QPoint& topLeft, const QSize& size) = 0;

virtual TreeWidget* createTreeWidget(const QStringList& labels, const QPoint& topLeft, const QSize& size) = 0;

And now we come to the part where I'm having a hard time: this would need the implementation of createTabWidget in TabWidget and so on (which is fine because I could have a Tabwidget included in a Tabwidget, itself included in another TabWidget). If I use the same design that I used for the other elements (e.g. CheckBox), this would return a pointer to a new TabWidget:

TabWidget* TabWidget::createTabWidget(const QPoint& topLeft, const QSize& size)
{
    return new TabWidget(topLeft, size);
}

Doing so is giving me a real hard time debugging, so this raises several questions:

1. Is the upper TabWidget::createTabWidget possible? (without those, it builds fine)
2. Shall I include the files for the containers e.g. tabwidget.h in the container interface to avoid circular depencies? (this gives me expected class name before '{' token)
3. Do I need to forward declare TabWidget in TreeWidget then? (this gives me an invalid use of incomplete type error)

Answer 1

Looks like you're missing a basic concept, and that's the separation of declaration and definition.

Your .h files should contain one class definition. So TabWidget.h should contain class TabWidget etc. The corresponding methods are defined in the .cpp file.

Because of this, TabWidget.h doesn't need the implementation of PushButton. It only uses PushButton*, the pointer. That means the compiler just needs to know that PushButton is a class type : class PushButton;. However, it's quite possible that TabWidget.cpp is calling new Pushbutton, and for that you need to include PushButton.h in TabWidget.cpp.

So you see there are no cyclic dependencies. The dependencies are directional: .cpp files depend on .h files but not vice versa.

Answer 2

You are using your ContainerInterface for creating the GUI components, so do not break the concept by adding a createTabWidget() method to the TabWidget class.

Intruduce a parent argument into your create...() methods instead. It can be a nullptr by default.

interface:

virtual TabWidget* createTabWidget(const QPoint& topLeft, const QSize& size, Component* parent = nullptr) = 0;

usage:

// Create a top-level tab widget, parent is null.
TabWidget* outerWidget = container->createTabWidget(position, size);

// Create a child tab widget, set the parent.
TabWidget* innerWidget = container->createTabWidget(position, size, outerWidget);

This solution assumes that all your GUI components (TabWidget, ComboBox, Image, ...) all derived from a common base class, a Component in my example.

Answer 3

this would need the implementation of createTabWidget in TabWidget and so on

That's false. To use pointers and references to most types, you don't need the type's implementation to be visible, nor even the type's declaration, but only a forward declaration.

So, the following is how your project might look:

// ContainerInterface.h
#ifndef ContainerInterface_h
#define ContainerInterface_h
// No includes necessary

class QString;
class PushButton;
class TabWidget;
class ContainerInterface {
public:
  virtual PushButton* createButton(const QString &) = 0;
  virtual TabWidget* createTabWidget(const QString &) = 0;
};

#endif // ContainerInterface_h

// TabWidget.h
#ifndef TabWidget_h
#define TabWidget_h

#include <QTabWidget>
#include "ContainerInterface.h"

class TabWidget : public QTabWidget, ContainerInterface {
  ...
};

#endif // TabWidget_h

// TabWidget.cpp
#include "TabWidget.h" // must always be the first file included!
#include "PushButton.h"

TabWidget * TabWidget::createTabWidget(const QString & foo) {
  ...
}

PushButton * TabWidget::createPushButton(const QString & foo) {
  ...
}

This will compile, but it doesn't mean it's good design. It's quite bad that container interface must know how to create an instance of given type. The ContainerInterface idea seems fundamentally broken and unscalable.

You should flip the problem around:

Have a map of widget creators that map from an XML tag or class to a functor that takes the XML stream and returns a QWidget* pointer. Every concrete widget class simply needs to be registered in that map.

E.g.:

// ItemFactory.h
#ifndef ItemFactory_h
#define ItemFactory_h
#include <QMap>

class QXmlStreamReader;
class ItemFactory {
  QMap<QString, QWidget*(*)(QXmlStreamReader &)> m_loaders;
public:
  QWidget * loadItem(const QString & tag, QXmlStreamReader & reader);
  void registerLoader(const QString & tag, QWidget*(*loader)(QXmlStreamReader &));
  static ItemFactory & instance(); // if you want it a singleton
};

#endif // ItemFactory_h

// ItemFactory.cpp
#include "ItemFactory.h"
Q_GLOBAL_STATIC(ItemFactory, itemFactory)

QWidget * ItemFactory::loadItem(const QString & tag, QXmlStreamReader & reader) {
  auto it = m_loaders.find(tag);
  if (it == m_loaders.end())
    return nullptr;
  return (*it)(reader);
}

void ItemFactory::registerLoader(const QString & tag, QWidget*(*loader)(QXmlStreamReader &) {
  m_loaders.insert(tag, loader);
}

ItemFactory & ItemFactory::instance() {
  return *itemFactory;
}

// CheckBox.h
...
class CheckBox : public QCheckBox {
public:
  ...
  static void registerType();
}

// CheckBox.cpp
#include "CheckBox.h"
#include "ItemFactory.h"

void CheckBox::registerType() {
  ItemFactory::instance().registerLoader(QStringLiteral("CheckBox"), +[](QXmlStreamReader & xml) -> QWidget* {
    Q_ASSERT(xml.isStartElement() && xml.name() == "checkbox");
    auto attr = xml.attributes();
    auto checkBox = new CheckBox(getLabel(attr), getDefine(attr), getHeader(attr, header), getTopLeft(attr), getSize(attr));
    checkBox.setProperty("define", getDefine(attr)); // bundle the define in the widget in a generic way
  });
}

It should be easy then to allow the container to take a pointer to a generic QWidget* and add it, using the widget->property("define").toString() to get the "define" that you presumably need.

Your main problem, as is common with many other questions, is not stating what your purpose is. Yeah, you have an XML file that defines the UI, but that doesn't enforce a particular C++ design, not yet.