Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-15 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/
---

(Updated Aug. 15, 2016, 1:34 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Changes
---

Submitted with commit cb9ec94625ebca02926579c16c96309127aacfc9 by David 
Edmundson to branch master.


Repository: plasma-framework


Description
---

Similar to KPasswordWidget in kwidgetaddons


Diffs
-

  src/declarativeimports/plasmacomponents/qml/TextField.qml 
c0d9155df5bb584cd3070a66c99d98465b81a5ef 
  src/declarativeimports/plasmastyle/TextFieldStyle.qml 
05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
  tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 

Diff: https://git.reviewboard.kde.org/r/128660/diff/


Testing
---

Updated manual test. See screenshot.


File Attachments


Spectacle.B12253.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png


Thanks,

David Edmundson

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-15 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/#review98401
---


Fix it, then Ship it!





src/declarativeimports/plasmacomponents/qml/TextField.qml (line 113)


This always confuses me but:
the LTR or RTL suffix does NOT indicate the writing direction but the 
direction of the arrow (ie. it's vice-versa as you would think it is). That 
Breeze uses the same icon for both doesn't help with this, of course, so should 
be:

source: LayoutMirroring.enabled ? "edit-clear-locationbar-ltr" : 
"edit-clear-locationbar-rtl"


http://www.iconarchive.com/show/oxygen-icons-by-oxygen-icons.org/Actions-edit-clear-locationbar-ltr-icon.html
 ? that's the mirrored one with "ltr" suffix


- Kai Uwe Broulik


On Aug. 15, 2016, 11:22 vorm., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128660/
> ---
> 
> (Updated Aug. 15, 2016, 11:22 vorm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Similar to KPasswordWidget in kwidgetaddons
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/plasmacomponents/qml/TextField.qml 
> c0d9155df5bb584cd3070a66c99d98465b81a5ef 
>   src/declarativeimports/plasmastyle/TextFieldStyle.qml 
> 05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
>   tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 
> 
> Diff: https://git.reviewboard.kde.org/r/128660/diff/
> 
> 
> Testing
> ---
> 
> Updated manual test. See screenshot.
> 
> 
> File Attachments
> 
> 
> Spectacle.B12253.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-15 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/
---

(Updated Aug. 15, 2016, 11:22 a.m.)


Review request for KDE Frameworks and Plasma.


Repository: plasma-framework


Description
---

Similar to KPasswordWidget in kwidgetaddons


Diffs (updated)
-

  src/declarativeimports/plasmacomponents/qml/TextField.qml 
c0d9155df5bb584cd3070a66c99d98465b81a5ef 
  src/declarativeimports/plasmastyle/TextFieldStyle.qml 
05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
  tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 

Diff: https://git.reviewboard.kde.org/r/128660/diff/


Testing
---

Updated manual test. See screenshot.


File Attachments


Spectacle.B12253.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png


Thanks,

David Edmundson

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-15 Thread Kai Uwe Broulik 


> On Aug. 15, 2016, 10:52 vorm., Kai Uwe Broulik wrote:
> > src/declarativeimports/plasmastyle/TextFieldStyle.qml, line 80
> > 
> >
> > Don't add units.smallSpacing if there's no actions, the padding is now 
> > uneven left and right when no buttons are present

Also, shouldn't that be a base.margins.right or sth like that?


- Kai Uwe


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/#review98397
---


On Aug. 14, 2016, 10:48 nachm., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128660/
> ---
> 
> (Updated Aug. 14, 2016, 10:48 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Similar to KPasswordWidget in kwidgetaddons
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/plasmacomponents/qml/TextField.qml 
> c0d9155df5bb584cd3070a66c99d98465b81a5ef 
>   src/declarativeimports/plasmastyle/TextFieldStyle.qml 
> 05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
>   tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 
> 
> Diff: https://git.reviewboard.kde.org/r/128660/diff/
> 
> 
> Testing
> ---
> 
> Updated manual test. See screenshot.
> 
> 
> File Attachments
> 
> 
> Spectacle.B12253.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-15 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/#review98397
---




src/declarativeimports/plasmastyle/TextFieldStyle.qml (line 78)


Don't add units.smallSpacing if there's no actions, the padding is now 
uneven left and right when no buttons are present


- Kai Uwe Broulik


On Aug. 14, 2016, 10:48 nachm., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128660/
> ---
> 
> (Updated Aug. 14, 2016, 10:48 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Similar to KPasswordWidget in kwidgetaddons
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/plasmacomponents/qml/TextField.qml 
> c0d9155df5bb584cd3070a66c99d98465b81a5ef 
>   src/declarativeimports/plasmastyle/TextFieldStyle.qml 
> 05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
>   tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 
> 
> Diff: https://git.reviewboard.kde.org/r/128660/diff/
> 
> 
> Testing
> ---
> 
> Updated manual test. See screenshot.
> 
> 
> File Attachments
> 
> 
> Spectacle.B12253.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-15 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/#review98396
---




src/declarativeimports/plasmacomponents/qml/TextField.qml (line 92)


While at it: spaces



src/declarativeimports/plasmacomponents/qml/TextField.qml (line 113)


You use the same icon in both cases



src/declarativeimports/plasmastyle/TextFieldStyle.qml (line 72)


Dream on... :)



src/declarativeimports/plasmastyle/TextFieldStyle.qml (line 77)


number of


- Kai Uwe Broulik


On Aug. 14, 2016, 10:48 nachm., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128660/
> ---
> 
> (Updated Aug. 14, 2016, 10:48 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Similar to KPasswordWidget in kwidgetaddons
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/plasmacomponents/qml/TextField.qml 
> c0d9155df5bb584cd3070a66c99d98465b81a5ef 
>   src/declarativeimports/plasmastyle/TextFieldStyle.qml 
> 05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
>   tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 
> 
> Diff: https://git.reviewboard.kde.org/r/128660/diff/
> 
> 
> Testing
> ---
> 
> Updated manual test. See screenshot.
> 
> 
> File Attachments
> 
> 
> Spectacle.B12253.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-14 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/
---

(Updated Aug. 14, 2016, 10:48 p.m.)


Review request for KDE Frameworks and Plasma.


Changes
---

Did Kai's comments.

This patch also fixes:

margins being re-evaluated on property changes
RTL icons
Actual use of the style in QQC without use of PlasmaComponents


Repository: plasma-framework


Description
---

Similar to KPasswordWidget in kwidgetaddons


Diffs (updated)
-

  src/declarativeimports/plasmacomponents/qml/TextField.qml 
c0d9155df5bb584cd3070a66c99d98465b81a5ef 
  src/declarativeimports/plasmastyle/TextFieldStyle.qml 
05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
  tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 

Diff: https://git.reviewboard.kde.org/r/128660/diff/


Testing
---

Updated manual test. See screenshot.


File Attachments


Spectacle.B12253.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png


Thanks,

David Edmundson

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-12 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/#review98328
---



Sorry, found two issues.


src/declarativeimports/plasmacomponents/qml/TextField.qml (line 45)


Add @since



src/declarativeimports/plasmastyle/TextFieldStyle.qml (line 70)


The +6 is wrong here, it doesn't needs to be added only once (not 
multipplied by the button count below)



src/declarativeimports/plasmastyle/TextFieldStyle.qml (line 74)


Here you need the +6 and the units.smallSpacing is wrong, I think.

At least with two buttons the padding on the right is wrong here.


- Kai Uwe Broulik


On Aug. 12, 2016, 12:36 vorm., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128660/
> ---
> 
> (Updated Aug. 12, 2016, 12:36 vorm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Similar to KPasswordWidget in kwidgetaddons
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/plasmacomponents/qml/TextField.qml 
> c0d9155df5bb584cd3070a66c99d98465b81a5ef 
>   src/declarativeimports/plasmastyle/TextFieldStyle.qml 
> 05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
>   tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 
> 
> Diff: https://git.reviewboard.kde.org/r/128660/diff/
> 
> 
> Testing
> ---
> 
> Updated manual test. See screenshot.
> 
> 
> File Attachments
> 
> 
> Spectacle.B12253.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 128660: Add an optional reveal password button to TextField

2016-08-12 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/#review98326
---


Fix it, then Ship it!




Neat.

Some minor nitpicks below.

And thanks for already incorporating the fixes of Reviews 128551 and 128549 
which I forgot about :)


src/declarativeimports/plasmacomponents/qml/TextField.qml (line 85)


Hardcoded pixel size (hm was there before, so..)



src/declarativeimports/plasmacomponents/qml/TextField.qml (line 88)


That is the default for Row, only RowLayout has a default spacing



src/declarativeimports/plasmacomponents/qml/TextField.qml (line 92)


===



src/declarativeimports/plasmastyle/TextFieldStyle.qml (line 70)


Coding style, add spaces



src/declarativeimports/plasmastyle/TextFieldStyle.qml (line 71)


Could be simplified to

var actionCount = control.clearButtonShown + 
control.revealPasswordButtonShown



tests/components/textfield.qml (line 37)


Two empty lines


- Kai Uwe Broulik


On Aug. 12, 2016, 12:36 vorm., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128660/
> ---
> 
> (Updated Aug. 12, 2016, 12:36 vorm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Similar to KPasswordWidget in kwidgetaddons
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/plasmacomponents/qml/TextField.qml 
> c0d9155df5bb584cd3070a66c99d98465b81a5ef 
>   src/declarativeimports/plasmastyle/TextFieldStyle.qml 
> 05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
>   tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 
> 
> Diff: https://git.reviewboard.kde.org/r/128660/diff/
> 
> 
> Testing
> ---
> 
> Updated manual test. See screenshot.
> 
> 
> File Attachments
> 
> 
> Spectacle.B12253.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Review Request 128660: Add an optional reveal password button to TextField

2016-08-11 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128660/
---

Review request for KDE Frameworks and Plasma.


Repository: plasma-framework


Description
---

Similar to KPasswordWidget in kwidgetaddons


Diffs
-

  src/declarativeimports/plasmacomponents/qml/TextField.qml 
c0d9155df5bb584cd3070a66c99d98465b81a5ef 
  src/declarativeimports/plasmastyle/TextFieldStyle.qml 
05ff3d542ad0de6879ec1de832b5e5d9ed33281e 
  tests/components/textfield.qml 0a7f8886ba0552c2bb23424ee873423cc5ee0585 

Diff: https://git.reviewboard.kde.org/r/128660/diff/


Testing
---

Updated manual test. See screenshot.


File Attachments


Spectacle.B12253.png
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/08/12/b308072d-a1c2-4bd8-9405-2826fc9f4086__Spectacle.B12253.png


Thanks,

David Edmundson