D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added a comment.


  Getting there!
  
  In D19077#417120 <https://phabricator.kde.org/D19077#417120>, @filipf wrote:
  
  > In D19077#417105 <https://phabricator.kde.org/D19077#417105>, @ngraham wrote:
  >
  > > This introduces a new warning:
  > >  WARNING: viewBackgroundColor is deprecated, use backgroundColor with colorSet: Theme.View instead
  >
  >
  > I would use the new value, but it doesn't provide the same color as the old one. So that means it breaks the magical hack and I can't hit the sweet spot anymore by fiddling with opacity.
  
  
  I'm not thrilled by the need to introduce new warnings in order to implement a hack. It's all a pretty stinky code smell, to be honest. :) Maybe @davidedmundson has some ideas.
  
  Also, here are some more inline comments:

INLINE COMMENTS

> main.qml:81
> +        Label {
> +            text: description + i18n(", by ") + authorName + " (" + license + ")"
> +            Layout.fillWidth: true

This is a word puzzle and will lead to bad translations because translators won't be able to figure out what `", by "` refers to. The string should be like this instead:

`i18n("%1, by %2 (%3)", description, authorName, license)`

Of course if there's a chance any of those strings might be unset or empty, you'll need to handle that in the code or else the empty string will mess up the `i18n()` call.

> main.qml:86
>  
> -            Text {
> -                color: palette.text
> -                text: description
> -                font.bold: true
> -                font.pointSize: 13
> -            }
> -            Text {
> -                color: palette.text
> -                text: version
> -                font.bold: true
> -                font.pointSize: 10
> -                Layout.alignment: Qt.AlignVCenter | Qt.AlignRight
> -            }
> -            Text {
> -                color: palette.text
> -                text: authorName
> -                font.pointSize: 10
> -            }
> -            Text {
> -                color: palette.text
> -                text: license
> -                font.pointSize: 7
> -                Layout.alignment: Qt.AlignVCenter | Qt.AlignRight
> -            }
> -            Text {
> -                color: palette.text
> -                text: website
> -                font.pointSize: 7
> -            }
> -            Text {
> -                color: palette.text
> -                text: email
> -                font.pointSize: 7
> -                Layout.alignment: Qt.AlignVCenter | Qt.AlignRight
> -            }
> +        Label {
> +            id: url

This could use a `visible: website !== ""` so it doesn't appear and look weird when that piece of metadata isn't available.

> main.qml:95
> +
> +        Label {
> +            id: mail

^^ Ditto

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190223/89410c79/attachment-0001.html>