D27840: Introduce SettingState* elements to ease KCM writing

[Kai Uwe Broulik]

broulik added a comment.


  Cool

INLINE COMMENTS

> SettingStateBinding.qml:40
> +     * target: Item
> +     * The graphical element which state we want to manage based on a setting
> +     */

*whose

> SettingStateBinding.qml:58
> +     * extraEnabledPredicate: bool
> +     * An extra predicate which will be applied to the enabled property
> +     * of the target combined with the immutable state of the setting

This description didn't really help me in understanding what it does until I read the code further down.

> SettingStateBinding.qml:75
> +        property KCM.SettingStateIndicator indicator: null
> +        // Necessary for proper binding within the Component element,
> +        // otherwise it would get null for its created instances

Can you explain this.
Can't you

  indicatorComponent.createObject(target, {
      settingsState: settingsState
  });

> SettingStateBinding.qml:91
> +
> +    // We use it via a Component because we potentially need
> +    // to escape the parent/siblings only constraint for anchoring

We could also bind `parent: target` instead?

> SettingStateBinding.qml:97
> +        KCM.SettingStateIndicator {
> +            x: helper.leftCoord - (root.indicatorAsOverlay ? width : 0)
> +            y: root.indicatorAsOverlay ? 0 : (parent.height - height) / 2

Please check if this works with right-to-left languages (run with `-reverse` argument to test)

> SettingStateBinding.qml:98
> +            x: helper.leftCoord - (root.indicatorAsOverlay ? width : 0)
> +            y: root.indicatorAsOverlay ? 0 : (parent.height - height) / 2
> +            settingState: impl.settingState

Always `Math.round` whenever you divide sizes to ensure integer alignment

> SettingStateIndicator.qml:38
> +
> +    width: 16
> +    height: 16

Probably should be `Kirigami.Units.iconSizes.small`.
Generally, when creating reusable components, avoid giving them a `width` and `height`.
Instead, define an `implicitWidth` and `implicitHeight`.

> SettingStateIndicator.qml:40
> +    height: 16
> +    visible: icon.source !== ""
> +

Perhaps bind to `icon.valid`?

> SettingStateIndicator.qml:48
> +
> +    MouseArea {
> +        anchors.fill: parent

You could make the `root` `Item` a `MouseArea` instead.
Also, I think this should get some kind of hover and/or pressed indication?
Also, what about `Accessible` and keyboard focus? Should this be a proper `ToolButton` control instead?

> settingstatebindingprivate.cpp:30
> +{
> +    // Split since some exported types will be of the form: Foo_QMLTYPE_XX
> +    const auto className = QByteArray(item->metaObject()->className()).split('_').first();

Odd. Wouldn't we just get `QQuickRowLayout` et al? Or is that if you do a custom item with a `Layout` as a base?

> settingstatebindingprivate.cpp:82
> +        m_target->setProperty(bindingProperty, QVariant::fromValue(this));
> +        connect(m_target, &QQuickItem::parentChanged, this, &SettingStateBindingPrivate::triggerCoordChanges);
> +        connect(m_target, &QQuickItem::widthChanged, this, &SettingStateBindingPrivate::triggerCoordChanges);

Too bad `QQuickItemChangeListener` is private.

Also, does any of this need event compression?

> settingstatebindingprivate.cpp:90
> +    emit targetChanged();
> +    emit leftCoordChanged();
> +}

Check if it actually changed before emitting

> settingstatebindingprivate.cpp:107
> +    emit indicatorChanged();
> +    emit leftCoordChanged();
> +}

Check if it actually changed before emitting

> settingstatebindingprivate.h:29
> +    Q_OBJECT
> +    Q_PROPERTY(QQuickItem* target READ target WRITE setTarget NOTIFY targetChanged)
> +    Q_PROPERTY(QQuickItem* indicator READ indicator WRITE setIndicator NOTIFY indicatorChanged)

Coding style, `QQuickItem *target`

REPOSITORY
  R296 KDeclarative

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

To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, #frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200327/207d80d2/attachment-0001.html>