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

Reply via email to