D19077: Redesign the theme preview window

[Filip Fila]

This revision was automatically updated to reflect the committed changes.
Closed by commit R123:9094e982ff0c: Redesign the theme preview window (authored 
by filipf).

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=56329&id=56411

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf updated this revision to Diff 56329.
filipf added a comment.


  add authorship info, @GB_2 you can do the same

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=56314&id=56329

BRANCH
  master

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added a comment.


  Yeah, go ahead. It's kind of a judgment call, but so much of the file is 
changed that I say do it. :)

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  arcpatch-D19077

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  In D19077#450746 , @GB_2 wrote:
  
  > Fix "No Preview" icon
  
  
  Thanks, that fixed it!
  
  Should @GB_2 and I add ourselves as the file's authors? (never read up on how 
that works)

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  arcpatch-D19077

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 accepted this revision.
GB_2 added a comment.


  Now it's great.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  arcpatch-D19077

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 updated this revision to Diff 56314.
GB_2 added a comment.


  Fix "No Preview" icon

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=56310&id=56314

BRANCH
  arcpatch-D19077

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  ...And it saves memory too. :)

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  master

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf updated this revision to Diff 56310.
filipf added a comment.


  set sourceSize width and height (wow that's even better than mipmap)

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=56309&id=56310

BRANCH
  master

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added a comment.


  In D19077#450725 , @filipf wrote:
  
  > Also the scaled down image quality isn't that great, can I use `mipmap`?
  
  
  No, instead set `sourceSize` to be the size you want it displayed at.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  master

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  Also the scaled down image quality isn't that great, can I use `mipmap`?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  master

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf updated this revision to Diff 56309.
filipf added a comment.


  somewhat smarter code

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=56301&id=56309

BRANCH
  master

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  In D19077#450657 , @ngraham wrote:
  
  > I think that looks great!
  
  
  Thanks! Tiny problem though, the icon and the text get messed up when 
sddm-kcm is opened in System Settings, as opposed to standalone:
  
  F6774535: image.png 
  
  Would you happen to know what might be the cause?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  master

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added a comment.


  I think that looks great!

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  master

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  No preview thumbnail looks like this: F6774505: image.png 


REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  master

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf updated this revision to Diff 56301.
filipf added a comment.


  - add a dummy "No preview available" rectangle when there is no preview image
  - adjust shadows to mimick GridDelegate

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=56268&id=56301

BRANCH
  master

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 updated this revision to Diff 56268.
GB_2 added a comment.


  Try to fix diff

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=56266&id=56268

BRANCH
  arcpatch-D19077

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  All right, let's do it.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  arcpatch-D19077

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

To: filipf, #plasma, #vdg, ngraham, GB_2
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 added a comment.


  In D19077#442323 , @mart wrote:
  
  > In D19077#422017 , @filipf wrote:
  >
  > > @mart could you help us out a bit please? I used code with 
"Kirigami.Theme.viewBackgroundColor.r", which throws out a warning that the 
value is deprecated. The one suggested for replacement (Theme.View) isn't the 
same as the deprecated one though. How bad would it be to leave the code with 
the deprecated value?
  >
  >
  > that rectangle should havethe attached property:
  >  Rectangle {
  >
  >   Kirigami.Theme.inherit: false
  >   Kirigami.Theme.colorSet: Kirigami.Theme.View
  >   
  >
  > }
  >
  > then, anywhere under this item you access Kirigami.Theme.backgroundColor 
and will be the background color of the view, because you defined it to use the 
color set for views.
  
  
  This method doesn't work, I've used a different method with the correct 
result and no warnings now.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 updated this revision to Diff 56266.
GB_2 added a comment.


  Prevent deprecation warning

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=52351&id=56266

BRANCH
  arcpatch-D19077

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Marco Martin]

mart added a comment.


  In D19077#422017 , @filipf wrote:
  
  > @mart could you help us out a bit please? I used code with 
"Kirigami.Theme.viewBackgroundColor.r", which throws out a warning that the 
value is deprecated. The one suggested for replacement (Theme.View) isn't the 
same as the deprecated one though. How bad would it be to leave the code with 
the deprecated value?
  
  
  that rectangle should havethe attached property:
  Rectangle {
  
Kirigami.Theme.inherit: false
Kirigami.Theme.colorSet: Kirigami.Theme.View
  
  }
  
  then, anywhere under this item you access Kirigami.Theme.backgroundColor and 
will be the background color of the view, because you defined it to use the 
color set for views.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  In D19077#442288 , @GB_2 wrote:
  
  > I like B too.
  >  BTW, can "Background:" and the background selection button be centered 
like in a form layout?
  
  
  Sure, we could see if that would look better. Since I know you've been doing 
this kind of UI work lately, would you want to continue work on D19209 
?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 added a comment.


  I like B too.
  BTW, can "Background:" and the background selection button be centered like 
in a form layout?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  In D19077#442281 , @GB_2 wrote:
  
  > Ping
  
  
  I've been putting this off because we couldn't quite reach consensus on the 
general layout. Putting it to a vote again:
  
  A) simplistic
  F6745779: image.png 
  
  B) diff at its current state
  F6745785: image.png 
  
  FWIW I'm more in favor of B, but it needs to have the "Customize theme" 
heading removed (different patch).
  
  On a minor note @GB_2 , since no senior dev has approved it, the color 
matching hack will probably have to go.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 added a comment.


  Ping

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a subscriber: mart.
filipf added a comment.


  @mart could you help us out a bit please? I used code with 
"Kirigami.Theme.viewBackgroundColor.r", which throws out a warning that the 
value is deprecated. The one suggested for replacement (Theme.View) isn't the 
same as the deprecated one though. How bad would it be to leave the code with 
the deprecated value?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mart, GB_2, mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  P.S. I moved the background button to the bottom, F6635412: image.png 

  and I removed the vertical spacer so the label below the picture would never 
get cut off again (that glitch was fairly annoying and unpredictable...)

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

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  Maybe we should switch tack - the other KCMs just state "Icon theme by 
Author" or "Cursor theme by Author" in their tooltips.
  We could just do that here. "Theme by Author"
  
  The license and email/web addresses of the author don't seem to be crucial 
information and we can just leave them out.
  And this leaves us with Theme Name by Author, which is a simple if-else 
problem (if author then both else just name).
  The Author's name could be made into a link (to their email address, provided 
they provided one).

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

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added a comment.


  +1

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

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> filipf wrote in main.qml:81
> >   you'll need to handle that in the code
> 
> How to do this? Sometimes some information really is missing so it could be 
> an issue.

Then you'll need to have separate strings based on what could be missing. For 
example if only the authorName could be missing, you do this:

  text: if (authorname.length() === 0) {
  return i18n("%1 (%3)", description, license)
  } else {
  return i18n("%1, by %2 (%3)", description, authorName, license)
  }

If both authorName and license could be missing you'd need to add more 
conditions.

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

D19077: Redesign the theme preview window

[Filip Fila]

filipf added inline comments.

INLINE COMMENTS

> ngraham wrote in main.qml:81
> 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.

>   you'll need to handle that in the code

How to do this? Sometimes some information really is missing so it could be an 
issue.

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

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  In D19077#417814 , @ngraham wrote:
  
  >
  
  
  
  
  > 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.
  
  Sure. But in case he doesn't I don't think it should be dismissed - the code 
works (and is used elsewhere I think) and is fairly innocuous.

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

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added a comment.


  Getting there!
  
  In D19077#417120 , @filipf wrote:
  
  > In 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

D19077: Redesign the theme preview window

[Filip Fila]

filipf updated this revision to Diff 52351.
filipf added a comment.


  save some vertical space, don't use max line count and elision

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=52249&id=52351

BRANCH
  master

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

AFFECTED FILES
  src/qml/main.qml

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

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  In 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.
  
  > +1 on removing the italic styling. I also think bolding the title is 
unnecessary too while it's using its maximum size (level 1). Bolding it only 
seems necessary when it's closer to the size of the text below it.
  
  Both of these points make sense to me, +1
  
  > I still don't like how the ColumnLayout of information causes the 
bottom-most items to get cut off with the default System Settings window 
height. I think we could consolidate some of the information, like moving the 
license onto one of the other lines.
  
  It still gets cut off? Darn. Hmm OK, will have a better think about it myself 
as well.

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

D19077: Redesign the theme preview window

[Nathaniel Graham]

ngraham added a comment.


  This introduces a new warning:
  
WARNING: viewBackgroundColor is deprecated, use backgroundColor with 
colorSet: Theme.View instead
  
  +1 on removing the italic styling. I also think bolding the title is 
unnecessary too while it's using its maximum size (level 1). Bolding it only 
seems necessary when it's closer to the size of the text below it.
  
  I still don't like how the ColumnLayout of information causes the bottom-most 
items to get cut off with the default System Settings window height. I think we 
could consolidate some of the information, like moving the license onto one of 
the other lines.
  
  Here's basically how it would look:
  F6628909: Screenshot_20190221_160448.png 


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

D19077: Redesign the theme preview window

[Björn Feber]

GB_2 added a comment.


  In D19077#417065 , @filipf wrote:
  
  > OK so down to only one hack now. If possible I'd really like to keep it 
because it works in 100% of the cases I tested it with and results in better 
visuals; don't know how to fix on the C++ side of things.
  >
  > Couple of things that might be worthy of being discussed:
  >
  > 1. changes to the layout of the text - @ngraham already had some 
suggestions, @abetts do you have some other ideas?
  > 2. add a dummy rectangle that would show up when there is no previewImage 
and that would say "Image preview not available"?
  
  
  It could show a Kirigami Icon "view-preview" when there's no image. 
  Can you also change the description to not be italic?

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

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  OK so down to only one hack now. If possible I'd really like to keep it 
because it works in 100% of the cases I tested it with and results in better 
visuals; don't know how to fix on the C++ side of things.
  
  Couple of things that might be worthy of being discussed:
  
  1. changes to the layout of the text - @ngraham already had some suggestions, 
@abetts do you have some other ideas?
  2. add a dummy rectangle that would show up when there is no previewImage and 
that would say "Image preview not available"?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D19077: Redesign the theme preview window

[Filip Fila]

filipf updated this revision to Diff 52249.
filipf added a comment.


  clean up whitespace, remove MouseArea hacks, remove the double Rectangle hack 
by defining height 
  with Units.gridUnit, add spacing below the last label, better samples value 
for the DropShadow

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19077?vs=52181&id=52249

BRANCH
  master

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

AFFECTED FILES
  src/qml/main.qml

To: filipf, #plasma, #vdg, ngraham
Cc: mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  In D19077#416726 , @mmustac wrote:
  
  > To be honest this kcm looks out of place when I compare it to the newer 
refurbished ones. The new look is quite an improvement compared to the current 
state but when I look at the color, window decoration kcm we have at first a 
big grid view and can then customize those themes by pressing the floating edit 
buttons which appear while hovering over them. Personally I would suggest to 
transfer those behavior to this kcm too to keep the uniefed design and workflow.
  
  
  I fully agree, but porting the whole KCM is outside the scope of my skills at 
this moment unfortunately.
  
  P.S Pozdrav ako si naš sunarodnjak

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  In D19077#416726 , @mmustac wrote:
  
  > To be honest this kcm looks out of place when I compare it to the newer 
refurbished ones. The new look is quite an improvement compared to the current 
state but when I look at the color, window decoration kcm we have at first a 
big grid view and can then customize those themes by pressing the floating edit 
buttons which appear while hovering over them. Personally I would suggest to 
transfer those behavior to this kcm too to keep the uniefed design and workflow.
  
  
  I agree. But in the meantime this doesn't seem like a bad fix either. You 
should check out D19209  as well

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D19077: Redesign the theme preview window

[Marijo Mustac]

mmustac added a comment.


  To be honest this kcm looks out of place when I compare it to the newer 
refurbished ones. The new look is quite an improvement compared to the current 
state but when I look at the color, window decoration kcm we have at first a 
big grid view and can then customize those themes by pressing the floating edit 
buttons which appear while hovering over them. Personally I would suggest to 
transfer those behavior to this kcm too to keep the uniefed design and workflow.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: mmustac, davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  This color "hack" actually makes the rectangle's color match some kind of 
weird in-between color that's not in the color scheme:
  F6628246: 75f5ca80-946e-4324-8f24-977fc96bc09b.jpeg 

  It takes on a color like the Applications tab in this picture - a color that 
can't be found in the color scheme as far as I can tell.
  The same thing happens in the SDDM KCM, so naturally none of the Kirigami 
theme colors fit.
  
  Is this a bug, by the way? Or a feature?
  Because it seems to, in a sense, override color scheme settings?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  The shadow's the culprit! If you add the height of the shadow (assign it an 
id beforehand), then there's no white bar at the bottom :D

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  In D19077#416439 , @filipf wrote:
  
  > In D19077#416432 , @rooty wrote:
  >
  > > In D19077#416430 , @filipf 
wrote:
  > >
  > > > You have to install the Elarun theme because it's the only one that's 
considerably taller than the other themes. Elarun will then break the height 
for your other themes.
  > >
  > >
  > > Still no problems.
  >
  >
  > You have to try harder to reproduce it - more themes and long descriptions 
vs. themes with even some missing info. It's not just my install, this is from 
a fresh KDE Neon Dev Unstable install:
  >
  > F6627477: image.png 
  
  
  You're right, I can reproduce it now

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  In D19077#416432 , @rooty wrote:
  
  > In D19077#416430 , @filipf wrote:
  >
  > > You have to install the Elarun theme because it's the only one that's 
considerably taller than the other themes. Elarun will then break the height 
for your other themes.
  >
  >
  > Still no problems.
  
  
  You have to try harder to reproduce it - more themes and long descriptions. 
It's not just my install, this is from a fresh KDE Neon Dev Unstable install:
  
  F6627477: image.png 

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  In D19077#416430 , @filipf wrote:
  
  > You have to install the Elarun theme because it's the only one that's 
considerably taller than the other themes. Elarun will then break the height 
for your other themes.
  
  
  Still no problems.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  You have to install the Elarun theme because it's the only one that's 
considerably taller than the other themes. Elarun will then break the height 
for your other themes.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[Krešimir Čohar]

rooty added a comment.


  In D19077#416427 , @filipf wrote:
  
  > @davidedmundson yes, so without the hack I get this happening:
  >
  > F6627430: image.png 
  >
  > When using childrenRect, I believe the rectangle's height is properly 
adjusted for the **tallest** theme preview, but ends up being too short for the 
other ones, thereby exposing the white-backgrounded QQuickWidget that contains 
this QML file.
  >
  > The hack unfortunately throws the root Rectangle's height property in a 
binding loop, but seems to be effective in both adjusting the height and not 
having the underlying widget exposed.
  
  
  If I remove that extra rectangle (the hack) and add one unit of largeSpacing 
to line 24, everything works both on Arch and on Neon
  F6627438: image.png  
  (I changed the layout of Background to accommodate the qml label margins, 
still working on the wallpaper button tho)

INLINE COMMENTS

> davidedmundson wrote in main.qml:39
> can you explain this?
> 
> It's the size of root and the same colour

i removed this extra rectangle and added large.Spacing to the height at line 24 
and everything works fine both on my arch rig and on neon?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[Filip Fila]

filipf added a comment.


  @davidedmundson yes, so without the hack I get this happening:
  
  F6627430: image.png 
  
  When using childrenRect, I believe the rectangle's height is properly 
adjusted for the **tallest** theme preview, but ends up being too short for the 
other ones, thereby exposing the white-backgrounded QQuickWidget that contains 
this QML file.
  
  The hack unfortunately throws the root Rectangle's height property in a 
binding loop, but seems to be effective in both adjusting the height and not 
having the underlying widget exposed.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D19077: Redesign the theme preview window

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> main.qml:39
>  
> +//HACK: fills the QuickWidget area root wouldn't normally cover
> +Rectangle {

can you explain this?

It's the size of root and the same colour

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: filipf, #plasma, #vdg, ngraham
Cc: davidedmundson, abetts, rooty, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart