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
