D16782: Use plasma TextArea for sharing text in plasmoid

[Simon Redman]

sredman added subscribers: apol, sredman.
sredman added a comment.


  This works on my computer. I have attached a screenshot. The borders, etc. look good with one line as well as many lines
  F6403712: PlasmaTextArea.png <https://phabricator.kde.org/F6403712>
  
  @apol Is the best for giving feedback on this. My concerns would be:
  
  - Does using the Plasma TextArea make us dependent on Plasma, or was that already implied by being a plasmoid?
  - What does the Plasma TextArea offer vs. the QtQuickControls 2 TextArea?

INLINE COMMENTS

> DeviceDelegate.qml:254
>              width: parent.width;
> -            height: Math.max(shareText.contentHeight + shareText.textMargin * 3, shareText.textMargin * 7 + shareText.font.pixelSize)
> +            height: Math.max(contentHeight + 12, theme.mSize(theme.defaultFont).height * 2 + 3)
>          }

Magic numbers are to be avoided. If the constant offset is necessary, it would be best to give it a name by adding a `readonly property <type> <name>: <value>` then use the name. That way, at least it is clear what the thing being added is. Bonus points for a comment on the line declaring the property to say how it was calculated or its purpose :)

I would like to see a comment explaining what Math.max is calculating. In case of an unbounded height, it seems quite strange.

REPOSITORY
  R224 KDE Connect

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

To: blaws
Cc: sredman, apol, kdeconnect, shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, mikesomov, tctara
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20181109/9c1e6da1/attachment.html>