D8641: FormLayout

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> FormLayout.qml:8
> +
> +Control {
> +    id: root

Why do we inherit from Control?

> FormLayout.qml:20
> +        columnSpacing: Kirigami.Units.smallSpacing
> +        anchors {
> +            left: parent.left

this can be taller than the parent item.

> FormLayout.qml:27
> +    default property list<Item> __items
> +    on__ItemsChanged: {
> +        for (var i = 0; i < __items.length; ++i) {

what about when an item gets deleted?

> FormLayout.qml:31
> +
> +            if (item.parent && item.parent.parent == lay) {
> +                continue;

so this is skipping items that are already in here?

> FormLayout.qml:47-48
> +
> +            var buddy = buddyComponent.createObject(lay, {"formData": item.Kirigami.FormData})
> +            buddy.parent = lay;
> +

merge

> FormLayout.qml:83
> +            property var formData
> +            width: 1
> +            text: formData.label

If you're doing a hack round something, document what.

> FormLayout.qml:86
> +
> +            font.pointSize: formData.isSection ? Kirigami.Theme.defaultFont.pointSize * 1.2 : Kirigami.Theme.defaultFont.pointSize
> +            font.weight: formData.isSection ? Font.Light : Font.Normal

We have an entire KCM with fonts where you can customise everything.

Then we ignore it and hardcode a bunch of them. The end result is we have complexity and don't even use it.

> formlayoutattached.h:33
> +    Q_PROPERTY(bool isSection READ isSection WRITE setIsSection NOTIFY isSectionChanged)
> +    Q_PROPERTY(QQuickItem *buddyFor READ buddyFor WRITE setBuddyFor NOTIFY buddyForChanged)
> +

Why would this ever not be the parent? I think having that writable is opening a confusing mess you don't want.

> formlayoutattached.h:40
> +
> +    void setLabel(const QString &text);
> +    QString label() const;

That's a really clever idea; it makes it very flexible so a phone layout could have the labels on top ++++

so I'm a bit surprised that the FormLayout.qml is an object, and not a template; I think it's throwing away an opportunity.

REPOSITORY
  R169 Kirigami

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

To: mart, #plasma, #kirigami, hein
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171103/fdf502e5/attachment-0001.html>