Merge lp:~jonas-drange/ubuntu-system-settings/security-pass-input-fix-1357548 into lp:ubuntu-system-settings

[Jonas G. Drange]

[security] replace inputmask for validator and maximum length

[security] replace inputmask for validator and maximum length

[Iain Lane (laney)]

On Fri, Jan 09, 2015 at 04:20:36PM -0000, Jonas G. Drange wrote:
> Jonas G. Drange has proposed merging lp:~jonas-drange/ubuntu-system-settings/security-pass-input-fix-1357548 into lp:ubuntu-system-settings.
>
> Commit message:
> [security] replace inputmask for validator and maximum length

Are you saying with this that the UITK is not going to change?

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1247
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1890/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/758
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/160
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/659
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/756
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/756/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16973

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1890/rebuild

review: Needs Fixing (continuous-integration)

[Ken VanDine (ken-vandine)]

In a couple of places you have the IntValidator bottom as 1000, I think this should be 0.

review: Needs Fixing

[lp:~jonas-drange/ubuntu-system-settings/security-pass-input-fix-1357548]

1248. By Jonas G. Drange on 2015-01-12

using new method

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1248
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1891/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/771
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/161
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/671
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/769
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/769/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17001

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1891/rebuild

review: Needs Fixing (continuous-integration)

[Matthew Paul Thomas (mpt)]

I'm delighted that this confusing bug is being fixed! I am not an engineer, and I might be greatly misunderstanding what is going on ... But this approach does not look right to me.

Pat wrote in bug 1357548 that "We currently set an inputMask of '9999' which causes the pre-population of asterisks". From what I can tell, you have then brainstormed a way to achieve the same effect as an inputMask, without using inputMask. Correct?

If so, this has two problems. First, the code is longer.

Second, if any third-party developer writes an app containing a password field, where the password characters need to be restricted in any way, they might reasonably read <https://developer.ubuntu.com/api/qml/sdk-14.10/Ubuntu.Components.TextField/#inputMask-prop> and think that using an inputMask is a good way to achieve that. But if they do, they will have exactly the same bug in their app: weird bullet characters in an empty field. Right? It will be a booby-trap in the toolkit.

Either inputMask is a good idea or it isn't. If it is, ubuntu-system-settings should continue using it, and this bug should be fixed in the toolkit. And if it isn't, it shouldn't be in the toolkit at all.

review: Needs Information

[Ken VanDine (ken-vandine)]

Looks good, works around the issue nicely.

review: Approve

=== modified file 'plugins/security-privacy/LockSecurity.qml'
--- plugins/security-privacy/LockSecurity.qml	2014-10-31 08:45:10 +0000
+++ plugins/security-privacy/LockSecurity.qml	2015-01-12 13:31:35 +0000
@@ -70,6 +70,11 @@
         dlg.newMethod = newMethod
     }
 
+    RegExpValidator {
+        id: passcodeValidator
+        regExp: /\d{4}/
+    }
+
     Component {
         id: dialogComponent
 
@@ -142,13 +147,6 @@
                     else
                         return Qt.ImhNone
                 }
-                inputMask: {
-                    if (changeSecurityDialog.oldMethod ===
-                            UbuntuSecurityPrivacyPanel.Passcode)
-                        return "9999"
-                    else
-                        return ""
-                }
                 visible: changeSecurityDialog.oldMethod ===
                             UbuntuSecurityPrivacyPanel.Passphrase ||
                          changeSecurityDialog.oldMethod ===
@@ -160,6 +158,24 @@
                 }
             }
 
+            /* Using bindings since it is, according to documentation,
+            impossible to unset both validator and maximumLength properties */
+            Binding {
+                target: currentInput
+                property: "validator"
+                value:  passcodeValidator
+                when: changeSecurityDialog.oldMethod ===
+                    UbuntuSecurityPrivacyPanel.Passcode
+            }
+
+            Binding {
+                target: currentInput
+                property: "maximumLength"
+                value:  4
+                when: changeSecurityDialog.oldMethod ===
+                    UbuntuSecurityPrivacyPanel.Passcode
+            }
+
             Label {
                 id: incorrect
                 text: ""
@@ -197,20 +213,28 @@
                     else
                         return Qt.ImhNone
                 }
-                inputMask: {
-                    if (changeSecurityDialog.newMethod ===
-                            UbuntuSecurityPrivacyPanel.Passcode)
-                        return "9999"
-                    else
-                        return ""
-                }
                 visible: changeSecurityDialog.newMethod ===
                             UbuntuSecurityPrivacyPanel.Passcode ||
                          changeSecurityDialog.newMethod ===
                             UbuntuSecurityPrivacyPanel.Passphrase
-                // Doesn't get updated if you set this in enabled of confirmButton
-                onTextChanged: confirmButton.enabled =
-                               (acceptableInput && (!visible || text.length > 0))
+            }
+
+            /* Using bindings since it is, according to documentation,
+            impossible to unset both validator and maximumLength properties */
+            Binding {
+                target: newInput
+                property: "validator"
+                value: passcodeValidator
+                when: changeSecurityDialog.newMethod ===
+                    UbuntuSecurityPrivacyPanel.Passcode
+            }
+
+            Binding {
+                target: newInput
+                property: "maximumLength"
+                value:  4
+                when: changeSecurityDialog.newMethod ===
+                    UbuntuSecurityPrivacyPanel.Passcode
             }
 
             Label {
@@ -243,19 +267,30 @@
                     else
                         return Qt.ImhNone
                 }
-                inputMask: {
-                    if (changeSecurityDialog.newMethod ===
-                            UbuntuSecurityPrivacyPanel.Passcode)
-                        return "9999"
-                    else
-                        return ""
-                }
                 visible: changeSecurityDialog.newMethod ===
                             UbuntuSecurityPrivacyPanel.Passcode ||
                          changeSecurityDialog.newMethod ===
                             UbuntuSecurityPrivacyPanel.Passphrase
             }
 
+            /* Using bindings since it is, according to documentation,
+            impossible to unset both validator and maximumLength properties */
+            Binding {
+                target: confirmInput
+                property: "validator"
+                value:  passcodeValidator
+                when: changeSecurityDialog.newMethod ===
+                    UbuntuSecurityPrivacyPanel.Passcode
+            }
+
+            Binding {
+                target: confirmInput
+                property: "maximumLength"
+                value:  4
+                when: changeSecurityDialog.newMethod ===
+                    UbuntuSecurityPrivacyPanel.Passcode
+            }
+
             Label {
                 id: notMatching
                 wrapMode: Text.Wrap
@@ -303,7 +338,7 @@
                         else
                             return i18n.tr("Set")
                     }
-                    enabled: false
+                    enabled: newInput.acceptableInput
                     onClicked: {
                         changeSecurityDialog.enabled = false