D8641: FormLayout

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> FormLayoutGallery.qml:79
> +            text: item ? "Remove Field" : "Add Field"
> +            property var item
> +            onClicked: {

`property Item`?

That's in a couple of places

> FormLayout.qml:45
> +
> +        Timer {
> +            id: hintCompression

Use `Qt.callLater` (new in Qt 5.8) if you pass it an actual function (ie. not an inline one) it will compress subsequent calls to one: https://doc.qt.io/qt-5/qml-qtqml-qt.html#callLater-method

There's probably more places in the code where you could do this.

> formlayoutattached.cpp:28
> +    m_buddyFor = qobject_cast<QQuickItem *>(parent);
> +    qApp->installEventFilter(this);
> +}

This class does not have an `eventFilter`

> formlayoutattached.h:37
> +
> +    explicit FormLayoutAttached(QObject *parent = 0);
> +    ~FormLayoutAttached();

`nullptr`

> formlayoutattached.h:38
> +    explicit FormLayoutAttached(QObject *parent = 0);
> +    ~FormLayoutAttached();
> +

`override`

> formlayoutattached.h:56
> +    void isSectionChanged();
> +    void mnemonicChanged();
> +    void decoratedLabelChanged();

Unused, there's also no property.

> formlayoutattached.h:57
> +    void mnemonicChanged();
> +    void decoratedLabelChanged();
> +

Also unused

> formlayoutattached.h:61
> +    QString m_label;
> +    QString m_actualDecoratedLabel;
> +    QString m_decoratedLabel;

Unused

> formlayoutattached.h:62
> +    QString m_actualDecoratedLabel;
> +    QString m_decoratedLabel;
> +    QPointer <QQuickItem> m_buddyFor;

Unused

> mnemonicattached.cpp:52
> +{
> +    if (s_objectToSequence.contains(this)) {
> +        s_sequenceToObject.remove(s_objectToSequence.value(this));

Double lookup (contains+value), also below

> mnemonicattached.cpp:62
> +
> +    if (m_richTextLabel.length() == 0) {
> +        return false;

`isEmpty`

> mnemonicattached.cpp:167
> +        m_mnemonicLabel = m_actualRichTextLabel;
> +        emit mnemonicLabelChanged();
> +        emit richTextLabelChanged();

Can we emit this stuff only if actually changed or can this be assumed here?

> mnemonicattached.cpp:229
> +{
> +    return m_actualRichTextLabel.length() > 0 ? m_actualRichTextLabel : m_label;
> +}

`!isEmpty?

> mnemonicattached.cpp:287
> +    } else {
> +        m_weight = m_baseWeight + m_weights.keys().last();
> +    }

`(m_weights.constEnd() - 1).key()`? Avoids creating a `keys()` temporary list

> mnemonicattached.h:38
> +    Q_PROPERTY(QKeySequence sequence READ sequence NOTIFY sequenceChanged)
> +    Q_ENUMS(ControlType)
> +public:

Use `Q_ENUM`

> mnemonicattached.h:48
> +
> +    explicit MnemonicAttached(QObject *parent = 0);
> +    ~MnemonicAttached();

`nullptr`

REPOSITORY
  R169 Kirigami

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

To: mart, #plasma, #kirigami, hein
Cc: broulik, colomar, ngraham, 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/20171116/2c40d3f7/attachment-0001.html>