[Differential] [Commented On] D4204: Patch for plasmoid subsystem(containments/desktop) in plasma-desktop

[David Edmundson]

davidedmundson added a comment.


  That's quite a huge patch.
  
  Can you give a much bigger explanation on exactly what it's doing and why.
  
  "Fixes bug N" doesn't really explain what the original problem was.

INLINE COMMENTS

> AppletAppearance.qml:61
>  
> -    property int minimumHeight: Math.max(root.layoutManager.cellSize.height,
> -                            appletContainer.minimumHeight +
> -                            appletItem.contents.anchors.topMargin +
> -                            appletItem.contents.anchors.bottomMargin)
> +    property int minimumHeight
> +    Binding on minimumHeight {

why do you do

property int foo
Binding on foo {
value: Math.max()
}

instead of just

property int foo: Math.max

?

> AppletAppearance.qml:226
>  
> +    QtObject {
> +        id: d

Point is a native QML type.
http://doc.qt.io/qt-5/qml-point.html

property point lastPoint

then use that in your logic above.
It can be better because you then get one signal when it changes not two.

> AppletAppearance.qml:445
> +                    var maxInt = 1000000; // dirty hack
> +                    if (size.width > maxInt)
> +                        size.width = maxInt;

if maximum is infinite, (so undefined in QtQuick.Layouts) set it to 1000000

why?
It's far easier to test for isFinite() in other bits of code than a random big init

REPOSITORY
  R119 Plasma Desktop

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

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

To: konstantinshtepa, #plasma
Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170119/b943b107/attachment.html>