Hi Daniel,
I like many things in this proposal, especially the principles; thanks for that!
Where I’m on the fence is the replacing of setParent with “adoptChild”. I think
of “parent” as a property of an object, so setParent/parent accessors are the
API that fits into my mental model. I have a hard time thinking about “list of
children” as that property..
Well this should not compile. (Or rather be optionally deprecated.)
unique_ptr<> a;
a->setParent(b);
But I think the example below shows exactly the problem <cliff_hanger>
Now for some classes the parent ptr has additional meaning beyond just being
the owner. E.g. for the layout classes. For that reason there's a bit of
infrastructure, which makes this work:
parent->makeChild<QTabWidget>();
or to take a more complex example:
parent->makeChild<QLabel>("Hello World!");
(The infrastructure is a third constructor, and a forwarding function defined
via Q_OBJECT)
For example, converting some random code on
https://doc.qt.io/qt-5/qtwidgets-widgets-lineedits-example.html to use
makeChild would look like this:
Window::Window(QWidget
*parent)
:
QWidget
(parent)
{
QGroupBox *echoGroup = makeChild<QGroupBox>(tr("Echo"));
QLabel *echoLabel =
echoGroup
->makeChild<QLabel>(tr("Mode"));
QComboBox *comboBox =
echoGroup
->makeChild<QComboBox>();
[...]
QGridLayout *echoLayout = echoGroup->makeChild<QGridLayout>();
echoLayout->addWidget(echoLabel, 0, 0);
echoLayout
->addWidget(echoComboBox, 0, 1);
echoLayout
->addWidget(echoLineEdit, 1, 0, 1, 2);
This works, but now you are encoding the visual hierarchy of the widgets in two
places: when asking the presumed parent to create the widgets; and when setting
up the layout, which might do things differently (esp once you start
refactoring complex UIs).#
That's not a problem. The code simply behaves as if you would have used
the current constructors that take a parent parameter.
Ownership transfers between different qt parents don't violate the
invariant. And they continue to work just like before.
Or if using unique_ptrs:
Window::Window(QWidget
*parent)
:
QWidget
(parent)
{
auto echoGroup = std::make_unique<QGroupBox>(tr("Echo"));
auto echoLabel = std::make_unique<QLabel>(tr("Mode"));
auto comboBox = std::make_unique<QComboBox>();
[...]
auto echoLayout = std::make_unique<QGridLayout>();
echoLayout->addWidget(std::move(echoLabel), 0, 0);
echoLayout
->addWidget(std::move(echoComboBox), 0, 1);
echoLayout
->addWidget(std::move(echoLineEdit), 1, 0, 1, 2);
I don’t think these lines above are correct.
QBoxLayout::addWidget does *not* take ownership. Widgets are reparented to
their correct parent once you call QWidget::setLayout in the next line, and
QLayout operates on QLayoutItems that hold a weak pointer to the widgets that
the layout places.
It's even more complicated than that, if the layout is already set on a
widget a call to QBoxLayout::addWidget does transfer ownership.
But for a parent-less QBoxLayout::addWidget(QWidget *) does not take
ownership. So whether a call to addWidget does transfer ownership is
really hard to tell.
Which I personally find surprising and I actually missed that in my
patch and Giuseppe had to point it out.
But that just means that QBoxLayout::addWidget(std::unique_ptr<>) has to
behave imho saner in that edge case and actually delete the widgets that
were added but never parented. (And I should really implement that to
show how it would work.)
There's another pain point in the patch currently, and that has to do
with the overloaded meaning of the parent ptr for QDialog. For QDialog
in addition to the memory ownership that also controls modality. The
other cases of overloaded meaning that I found in qtbase are as far as I
can tell fine.
Meaning a line such as this:
QDialog dialog(parent);
is technically a doubly owned object, because it's both owned by the
stack and by the passed parent. And that same problem makes it hard to
make this memory safe.
The solution to that, is to fix Qt here. Modality and parent should not
be coupled in this way.
daniel
_______________________________________________
Development mailing list
[email protected]
https://lists.qt-project.org/listinfo/development
