D19599: textfield height based only on clear text

2019-03-08 Thread Marco Martin 
This revision was automatically updated to reflect the committed changes.
Closed by commit R242:a2749dc9c718: textfield height based only on clear text 
(authored by mart).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19599?vs=53446=53449

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

AFFECTED FILES
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: mart, #plasma, ngraham, rooty
Cc: sitter, rooty, ngraham, kde-frameworks-devel, michaelh, bruns

D19599: textfield height based only on clear text

2019-03-08 Thread Marco Martin 
mart updated this revision to Diff 53446.
mart added a comment.


  - better formatting

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19599?vs=53445=53446

BRANCH
  arcpatch-D19599

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

AFFECTED FILES
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: mart, #plasma, ngraham, rooty
Cc: sitter, rooty, ngraham, kde-frameworks-devel, michaelh, bruns

D19599: textfield height based only on clear text

2019-03-08 Thread Marco Martin 
mart updated this revision to Diff 53445.
mart added a comment.


  - use just M

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19599?vs=53381=53445

BRANCH
  arcpatch-D19599

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

AFFECTED FILES
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: mart, #plasma, ngraham, rooty
Cc: sitter, rooty, ngraham, kde-frameworks-devel, michaelh, bruns

D19599: textfield height based only on clear text

2019-03-08 Thread Harald Sitter 
sitter added a comment.


  LGTM, but do we need the MMM there? Wouldn't one M do? Or if control.text is 
always a string we don't need the ""+ at all?

INLINE COMMENTS

> TextFieldStyle.qml:60
>  
> -implicitHeight: Math.max(control.cursorRectangle.height * 1.6, 
> control.cursorRectangle.height + base.margins.top + base.margins.bottom)
> +implicitHeight: Math.max(metrics.height * 1.6, metrics.height + 
> base.margins.top + base.margins.bottom)
>  implicitWidth: theme.mSize(theme.defaultFont).width * 12

maybe add a linebreak between the two sides so it's easy to spot the 
metrics.height on both sides of the max.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  phab/echo

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

To: mart, #plasma, ngraham, rooty
Cc: sitter, rooty, ngraham, kde-frameworks-devel, michaelh, bruns

D19599: textfield height based only on clear text

2019-03-07 Thread Krešimir Čohar 
rooty added a dependent revision: D19214: [sddm-theme] Replace login button 
label with icon.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  phab/echo

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

To: mart, #plasma, ngraham, rooty
Cc: rooty, ngraham, kde-frameworks-devel, michaelh, bruns

D19599: textfield height based only on clear text

2019-03-07 Thread Krešimir Čohar 
rooty accepted this revision.
rooty added a comment.


  Good things come to those who wait, thanks for the fix.
  I can also confirm that this doesn't affect any other font adversely in any 
way.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  phab/echo

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

To: mart, #plasma, ngraham, rooty
Cc: rooty, ngraham, kde-frameworks-devel, michaelh, bruns

D19599: textfield height based only on clear text

2019-03-07 Thread Nathaniel Graham 
ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  It's fixed
  
  Of course now the issue with the dots is worse since the dots are just too 
high up in the text field: F6670642: Peek 2019-03-07 10-44.gif 

  
  However that's an issue in Noto Symbols itself. Aoparently all the characters 
in the whole font have a descent value that's too large (7 vs 4 for in regular 
Noto Sans). We're tracking this with 
https://github.com/googlei18n/noto-fonts/issues/1468. Thanks so much for fixing 
this, Marco!

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  phab/echo

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

To: mart, #plasma, ngraham
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D19599: textfield height based only on clear text

2019-03-07 Thread Marco Martin 
mart created this revision.
mart added a reviewer: Plasma.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mart requested review of this revision.

REVISION SUMMARY
  as the implicit height could change when switching to password mode,
  use a textmetrics which always computes it from the visible text
  
  BUG:399155

TEST PLAN
  height doesn't change

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  phab/echo

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

AFFECTED FILES
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: mart, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns