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
