D22034: Introduce ContainmentLayoutManager QML plugin

[David Edmundson]

davidedmundson added a comment.


  Concept makes sense. I mostly like the layering and architecture, except for all the cases where we end up searching up the hierarchy to find objects. That doesn't seem quite as nicely layered.
  
  I'm not finished, it's a big patch - but I thought I should submit what I have for now.

INLINE COMMENTS

> abstractlayoutmanager.cpp:110
> +    QRectF candidate = candidateGeometry(item);
> +    item->setX(candidate.x());
> +    item->setY(candidate.y());

setPosition

setSize

> abstractlayoutmanager.h:44
> +     */
> +    QSizeF cellAlignedContainingSize(const QSizeF &size) const;
> +

I don't fully understand the split between AbstractLayoutManager and GridLayoutManager when AbstractLayoutManager effectively enforces a grid by having cell sizes.

> appletcontainer.cpp:32
> +{
> +    connect(this, &AppletContainer::contentItemChanged, this, [this]() {
> +        if (m_appletItem) {

How do we know this happens after the busy component is set?

> itemcontainer.cpp:364
> +
> +    QQuickItem *item;
> +    for (auto *o : m_contentData) {

scope this in the loop

> itemcontainer.cpp:370
> +        } else {
> +            o->setParent(this);
> +        }

why?

> itemcontainer.cpp:375
> +    // Search for the Layout attached property
> +    // Qt6: this should become public api
> +    for (auto *o : children()) {

no point just wishing, we need to open a bug report at least

> itemcontainer.cpp:438
> +    if (newPreferredHeight > height()) {
> +        setHeight(layout()->cellHeight() * ceil(newPreferredHeight / layout()->cellHeight()));
> +        changed = true;

why?

We're above the minimum size, the user could have resized it smaller than this

> BasicResizeHandle.qml:26
> +ContainmentLayoutManager.ResizeHandle {
> +    resizeCorner: ContainmentLayoutManager.ResizeHandle2.Right
> +

ResizeHandle2?

> resizehandle.cpp:42
> +
> +    connect(this, &QQuickItem::parentChanged, this, [this]() {
> +        QQuickItem *candidate = parentItem();

we monitor the direct parent, but ConfigOverlay could be anywhere on the ancestory tree.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22034

To: mart, #plasma
Cc: davidedmundson, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190717/18d8991f/attachment-0001.html>