> On 3 Feb 2020, at 19:09, Daniel Teske <q...@squorn.de> wrote:
>
>
>> 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);
That’s my point. Parent is a property of an object. Properties in Qt are set
and retrieved via accessors that follow a very consistent naming convention.
And calling setParent is just as explicit in terms of ownership as calling
std::make_unique or QObject::makeChild.
>> 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.
So how is the above usage of makeChild an improvement over calling operator new
to instantiate your objects?
It leads to confusing code if you make a child object via a parent that - after
the layout kicks in - is actually no longer the parent (which is the case when
you refactor a complex UI, and don’t remember to change both the code that
instantiates the objects, and sets the layout up)?
>>> 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.
Ah, but not to the layout, which the code makes you believe it does. The new
owner of echoLabel is “this”, not echoLayout.
> 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.)
That seems like a terrible idea. One of the layout’s jobs is to set up the
parent/child hierarchy so that you as a developer don’t have to.
Do we really want to make it harder for people to write UIs just because the
resulting code doesn’t satisfy a C++ idiom?
> 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.
It’s a solution to a problem that people don’t seem to have though. At least I
don’t recall this being a significant source of confusion during my 9 years at
Qt support. Yes, sometimes people allocate an object on the stack and then
wonder why the widget doesn’t show up in their UI. This is not exactly a subtle
issue that only sometimes causes unexplicable undefined behavior. Ditto for
modality or window stacking.
The parent/child model in Qt has several implications which I think (and it’s
ok to disagree) are consistent, given the purpose of QObject (and more so
QWidget):
* ability to respond to events within a chain of responders
* visual hierarchy, including stacking order for top-level widgets
* child lifetime coupled to parent's
* meta-object based stuff
If you don’t care about the things that QObject/QWidget does for you, then
don’t use QObject/QWidget. And if you do use QObject/QWidget and get a
consistent object model from it, then I’m really not convinced that adding
smart pointers to the API is making the code clearer, or in any way more
explicit. It’s not like the ability of using std::unique_ptr means that people
no longer have to be familiar with Qt’s basic concepts to use it correctly.
Exceptions as mentioned are those APIs that look like they take ownership, but
don’t (such as QTabWidget::addAction).
Volker
_______________________________________________
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
