D13390: Fonts KCM: Fix text readability regression

2018-06-08 Thread Andres Betts 
abetts added a comment.


  Maybe this is for another ticket, but... can we make the field where the font 
is named the actual field where you select the fonts from?
  
  F5898561: InkedSpectacle.S22260_LI.jpg 
  
  I mean, why have so many controls when the field can do it?
  
  Also, would it be helpful that the field where the font is named to be able 
to type your font into? Without having to get a drop down?

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx, mart
Cc: abetts, mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-08 Thread Henrik Fehlauer 
This revision was automatically updated to reflect the committed changes.
Closed by commit R119:540edfdd2a88: Fonts KCM: Fix text readability regression 
(authored by rkflx).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13390?vs=35808=35826

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

AFFECTED FILES
  kcms/fonts/package/contents/ui/FontWidget.qml

To: rkflx, mart
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-08 Thread Marco Martin 
mart accepted this revision.
mart added a comment.
This revision is now accepted and ready to land.


  awesome, thanks :)

REPOSITORY
  R119 Plasma Desktop

BRANCH
  fonts-kcm-readability (branched from Plasma/5.13)

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

To: rkflx, mart
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Henrik Fehlauer 
rkflx updated this revision to Diff 35808.
rkflx edited the summary of this revision.
rkflx edited the test plan for this revision.
rkflx added a comment.


  Okay, great. Thanks for investigating! I now changed to what you proposed, so 
we have something for 5.13.1.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13390?vs=35719=35808

BRANCH
  fonts-kcm-readability (branched from Plasma/5.13)

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

AFFECTED FILES
  kcms/fonts/package/contents/ui/FontWidget.qml

To: rkflx
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Marco Martin 
mart added a comment.


  edit: Kirigami.Theme.colorGroup: was wrong,should have been
  Kirigami.Theme.colorSet: Kirigami.Theme.Window
  
  tough i prefer the latter form of the patch

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Marco Martin 
mart added a comment.


  on which it looks like that:
  F5895805: Spectacle.S22260.png 

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Marco Martin 
mart added a comment.


  In D13390#275223 , @rkflx wrote:
  
  > In D13390#275153 , @mart wrote:
  >
  > > what you should do is:
  > >  TextField {
  > >
  > >   readOnly: true
  > >   Kirigami.Theme.inherit: false   //always to be set when you are writing 
anything to the Theme attached proeprty
  > >   Kirigami.Theme.colorGroup: Kirigami.Theme.Window
  > >
  > > }
  > >
  > > changing the textfield colorgroup to window, will make its background to 
become gray, so a look somewhat less "clickable"
  >
  >
  > Now it looks as if you can edit the text, and the previews are placeholders 
for what you can type in:
  
  
  yeah, that's a bug i can reproduce, it's not supposed to look like that, i'll 
fix it
  
  in the meantime, this form is also correct but it works with current kirigami:
  
QtControls.TextField {
 readOnly: true
 Kirigami.Theme.inherit: true

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Henrik Fehlauer 
rkflx added a comment.


  In D13390#275153 , @mart wrote:
  
  > what you should do is:
  >  TextField {
  >
  >   readOnly: true
  >   Kirigami.Theme.inherit: false   //always to be set when you are writing 
anything to the Theme attached proeprty
  >   Kirigami.Theme.colorGroup: Kirigami.Theme.Window
  >
  > }
  >
  > changing the textfield colorgroup to window, will make its background to 
become gray, so a look somewhat less "clickable"
  
  
  Now it looks as if you can edit the text, and the previews are placeholders 
for what you can type in:
  
  F5895669: fonts-kcm-kirigami.png 
  
  IMO that's worse.
  
  In D13390#275119 , @broulik wrote:
  
  > What works for me is doing `root.Kirigami.Theme.textColor` which arguably 
isn't a lot better since it won't use button context but view context but I 
don't like having a random `SystemPalette` item in there.
  
  
  Let me know if I should go in that direction. It's a bit ugly as it assumes 
`root` is in `enabled` state, but at least it works.
  
  (I miss good old `KFontRequester`.)

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Marco Martin 
mart added a comment.


  In D13390#275117 , @rkflx wrote:
  
  > In D13390#275116 , @broulik 
wrote:
  >
  > > > It will lead users to click into the field, expecting to be able to 
edit the name directly
  > >
  > > Oh, you mean because the background turns white when enabled? I see.
  >
  >
  > Yup, that was the point.
  >
  > > Please use
  > > 
  > >   color: Kirigami.Theme.textColor
  >
  > Is this working for you? For me this resolves to the disabled palette (as 
it should).
  >
  > (I could use `colorGroup: Kirigami.theme`, but there should be no 
difference to `colorGroup: SystemPalette.Active`, unless I'm missing something.)
  
  
  none of those are valid values for colorGroup.
  
  what you should do is:
  TextField {
  
readOnly: true
Kirigami.Theme.inherit: false   //always to be set when you are writing 
anything to the Theme attached proeprty
Kirigami.Theme.colorGroup: Kirigami.Theme.Window
  
  }
  
  changing the textfield colorgroup to window, will make its background to 
become gray, so a look somewhat less "clickable"

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Kai Uwe Broulik 
broulik added a subscriber: mart.
broulik added a comment.


  > Is this working for you? For me this resolves to the disabled palette (as 
it should).
  
  Interesting. I just looked at `TextField`'s code and saw that it explicit 
used a different color for `!enabled` so I thought removing that check worked.
  
  What works for me is doing `root.Kirigami.Theme.textColor` which arguably 
isn't a lot better since it won't use button context but view context but I 
don't like having a random `SystemPalette` item in there. Perhaps @mart knows a 
proper solution

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: mart, broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Henrik Fehlauer 
rkflx added a comment.


  In D13390#275116 , @broulik wrote:
  
  > > It will lead users to click into the field, expecting to be able to edit 
the name directly
  >
  > Oh, you mean because the background turns white when enabled? I see.
  
  
  Yup, that was the point.
  
  > Please use
  > 
  >   color: Kirigami.Theme.textColor
  
  Is this working for you? For me this resolves to the disabled palette (as it 
should).

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Kai Uwe Broulik 
broulik added a comment.


  > It will lead users to click into the field, expecting to be able to edit 
the name directly
  
  Oh, you mean because the background turns white when enabled? I see. Please 
use
  
color: Kirigami.Theme.textColor
  
  then this is good to go imho

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Henrik Fehlauer 
rkflx added a comment.


  In D13390#275109 , @broulik wrote:
  
  > > Unfortunately that's not what I think is needed here.
  >
  > Why not? You make the text field look as though it is enabled, which is 
what readOnly does. Still being able to select text isn't too bad, is it?
  
  
  It will lead users to click into the field, expecting to be able to edit the 
name directly. If that was the case, we could show that visually, but it's not, 
so we shouldn't either.

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Kai Uwe Broulik 
broulik added a comment.


  > Unfortunately that's not what I think is needed here.
  
  Why not? You make the text field look as though it is enabled, which is what 
readOnly does. Still being able to select text isn't too bad, is it?

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: broulik, Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D13390: Fonts KCM: Fix text readability regression

2018-06-07 Thread Henrik Fehlauer 
rkflx added a comment.


  In D13390#275034 , @Zren wrote:
  
  > Would the `TextField.readOnly` property be useful here?
  >
  > http://doc.qt.io/qt-5/qml-qtquick-controls-textfield.html#readOnly-prop
  >
  > F5894577: 2018-06-06___19-47-34.png 
  
  
  I already tried that, but as you can see in your screenshot, visually 
`readonly` still indicates to the user that the field can be edited. 
Unfortunately that's not what I think is needed here.

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13390: Fonts KCM: Fix text readability regression

2018-06-06 Thread Chris Holland 
Zren added a comment.


  Would the `TextField.readOnly` property be useful here?
  
  http://doc.qt.io/qt-5/qml-qtquick-controls-textfield.html#readOnly-prop
  
  F5894577: 2018-06-06___19-47-34.png 

REPOSITORY
  R119 Plasma Desktop

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

To: rkflx
Cc: Zren, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D13390: Fonts KCM: Fix text readability regression

2018-06-06 Thread Henrik Fehlauer 
rkflx created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
rkflx requested review of this revision.

REVISION SUMMARY
  Porting the Fonts KCM to QML in 24b960f92284 
 set 
the state of the font
  preview labels to `disabled`, resulting in a light gray text with too
  little contrast to enable a usable preview of the chosen font.
  
  In the `QWidgets` based KCM the following code was used:
  
QLabel::setFrameStyle(QFrame::StyledPanel | QFrame::Sunken)
  
  As that property is not available from QML, we reset the text colour to
  solve the problem. It's still not using the proper frame colour, though,
  but that does not seem like too much of an issue.
  
  Alternatively we could use a `Frame` and a `Label`, but this approach
  has even more problems, e.g. the frame does not respect the rounded
  border mandated by Breeze, and the horizontal alignment of each row
  would need to be adapted.

TEST PLAN
  Font previews for each category in `kcmshell5 kcm_fonts` should be
  readable again.
  
  Before: F5894425: fonts-kcm-before.png 
  After: F5894424: fonts-kcm-after.png 
  Ideal look (Plasma 5.12): F5894423: fonts-kcm-5.12.png 


REPOSITORY
  R119 Plasma Desktop

BRANCH
  fonts-kcm-readability (branched from Plasma/5.13)

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

AFFECTED FILES
  kcms/fonts/package/contents/ui/FontWidget.qml

To: rkflx
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart