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

[David Edmundson]

davidedmundson added a comment.


  I want to do another full review next week when I'm home; but I'd be happy with doing that after we merge it, as it'll only be small things, and nothing which will affect API.
  
  In general it all looks very good, I like that you split out ButtonShadow and all the rest seems neat. Good stuff.

INLINE COMMENTS

> Button.qml:28
> +
> +    implicitWidth: Math.max(background ? background.implicitWidth : 0,
> +                            contentItem.implicitWidth + leftPadding + rightPadding)

I don't get why we check background here and in other places. It would only be null if a user subclassed this and then overwrote the property to undefined, at which point they deserve errors.

> Button.qml:51
> +        //retrocompatibility with old controls
> +        implicitWidth: units.gridUnit * 6
> +        Private.ButtonShadow {

we do someting differently for widths and heights which isn't ideal, can we move the units.gridUnit * 1.6 into here as an implicitHeight?

> CheckIndicator.qml:32
>      opacity: control.enabled ? 1 : 0.6
>      property int checkState: control.checkState
>  

why?

> GroupBox.qml:34
>  
>      padding: 6
>      topPadding: padding + (label && label.implicitWidth > 0 ? label.implicitHeight + spacing : 0)

booo!

> Label.qml:27
>  
>      height: Math.round(Math.max(paintedHeight, units.gridUnit * 1.6))
>      verticalAlignment: lineCount > 1 ? Text.AlignTop : Text.AlignVCenter

Now we have an API break, lets fix this.

As soon as you put this in a layout it won't do anything, so it's quite inconsistent.
If anything it should be an implicitHeight, or just set the lineHeight?

But I'd rather we didn't have it at all, we have to work round it in lots of places.

If we want a padded Label, we can make a new subclass in the import with all the other headings.

> Label.qml:31
>      activeFocusOnTab: false
>      renderType: Text.NativeRendering
>  

Bhushan did a patch for P-F that changed this on the current label if we're on mobile, is that still possible?

> TextField.qml:60
>  
>      background: Item {
>          Private.TextFieldFocus {

can we kill an item here?

background: FrameSvgItem {

  TextFiledFocus {
      anchors.fill:parent
   }

}

> qmldir:39
> +ToolButton 3.0 ToolButton.qml
> +ToolTip 3.0 ToolTip.qml

maybe we don't want:

ToolTip, Popup, Dialog, DialogButtonBox here and only in the style?

Also which is the "correct" ItemDelegate we have one of them in PlasmaExtras

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: davidedmundson, 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/20170209/758bdb77/attachment-0001.html>