[Differential] [Commented On] D4508: [WIP] Plasma controls based on QtQuickControls2

[Kai Uwe Broulik]

broulik added a comment.


  Skimmed through it until `ItemDelegate.qml`, few nitpicks for that below

INLINE COMMENTS

> BusyIndicator.qml:28
> +
> +    implicitWidth: contentItem.implicitWidth + leftPadding + rightPadding
> +    implicitHeight: contentItem.implicitHeight + topPadding + bottomPadding

Apparently in Qt 5.8 the implicit sizes now take into account padding but since we want to support 5.7 (?) makes sense.

Mentioned in the changelog: https://code.qt.io/cgit/qt/qtquickcontrols2.git/tree/dist/changes-5.8.0

> BusyIndicator.qml:41
> +
> +        anchors.centerIn: parent
> +        width:  Math.min(control.width, control.height)

You sure this doesn't break anything if you mess with anchors and sizes and don't have a wrapper `Item` around it?

> BusyIndicator.qml:44
> +        height: width
> +        smooth: !control.running || (control.hasOwnProperty("smoothAnimation") && control.smoothAnimation)
> +

I think with QtQuick 2 `smooth` doesn't matter anymore, other than making it look worse.

>From docs

> In Qt Quick 2.0, this property has minimal impact on performance.

> Button.qml:37
> +
> +    hoverEnabled: true //Qt.styleHints.useHoverEffects TODO: how to make this work in 5.7?
> +

Docs say

> You can also enable or disable hover effects for all Qt Quick Controls 2 applications by setting the QT_QUICK_CONTROLS_HOVER_ENABLED environment variable.

> Button.qml:39
> +
> +    contentItem: Label {
> +        text: control.text

What `Label` is this? I don't see any import/namespace that would provide this here

> Button.qml:41
> +        text: control.text
> +        font: control.font
> +        opacity: enabled || control.highlighted || control.checked ? 1 : 0.4

QQC2 `Label` supposedly supports `font` (and probably other) inheritance

> CheckIndicator.qml:63
> +        anchors.fill: parent
> +        state: control.activeFocus ? "focus" : (control.hovered ? "hover" : "shadow")
> +    }

Now that we have `activeFocusReason` we could potentially behave differently whether it was user-focused or tab-focused (not now but keeping that in mind for the future)

> ComboBox.qml:96
> +
> +    popup: T.Popup {
> +        y: control.height

in-window `Popup` makes me sad :(

> DialogButtonBox.qml:41
> +
> +    contentItem: ListView {
> +        implicitWidth: contentWidth

Do we really want a `ListView` here?

Also, how does it work with e.g. `HelpRole` and things that would go on the left rather than the right? Dunno if it supports that though

> Drawer.qml:35
> +
> +    topPadding: control.edge === Qt.BottomEdge ? 1 : 0
> +    leftPadding: control.edge === Qt.RightEdge ? 1 : 0

`Math.round(units.devicePixelRatio)` instead of `1`?

> Frame.qml:34
> +
> +    padding: 6
> +

Hardcoded number

REPOSITORY
  R242 Plasma Framework (Library)

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: broulik, plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170208/f8dcee9a/attachment-0001.html>