ubuntu-system-settings

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

  1. security-pass-input-fix-1357548
  2. Merge into trunk
Proposed by Jonas G. Drange
Status: Merged
Approved by: Ken VanDine
Approved revision: 1248
Merged at revision: 1253
Proposed branch: lp:~jonas-drange/ubuntu-system-settings/security-pass-input-fix-1357548
Merge into: lp:ubuntu-system-settings
Diff against target: 139 lines (+60/-25)
1 file modified
plugins/security-privacy/LockSecurity.qml (+60/-25)
To merge this branch: bzr merge lp:~jonas-drange/ubuntu-system-settings/security-pass-input-fix-1357548
Reviewer Review Type Date Requested Status
Ken VanDine Approve
Matthew Paul Thomas (community) Needs Information
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+245980@code.launchpad.net

Commit message

[security] replace inputmask for validator and maximum length

Description of the change

[security] replace inputmask for validator and maximum length

To post a comment you must log in.
Revision history for this message
Iain Lane (laney) wrote :

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> ]

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

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 updated
1248. By Jonas G. Drange

using new method

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Matthew Paul Thomas (mpt) wrote :

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
Revision history for this message
Ken VanDine (ken-vandine) wrote :

Looks good, works around the issue nicely.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
70 dlg.newMethod = newMethod70 dlg.newMethod = newMethod
71 }71 }
7272
73 RegExpValidator {
74 id: passcodeValidator
75 regExp: /\d{4}/
76 }
77
73 Component {78 Component {
74 id: dialogComponent79 id: dialogComponent
7580
@@ -142,13 +147,6 @@
142 else147 else
143 return Qt.ImhNone148 return Qt.ImhNone
144 }149 }
145 inputMask: {
146 if (changeSecurityDialog.oldMethod ===
147 UbuntuSecurityPrivacyPanel.Passcode)
148 return "9999"
149 else
150 return ""
151 }
152 visible: changeSecurityDialog.oldMethod ===150 visible: changeSecurityDialog.oldMethod ===
153 UbuntuSecurityPrivacyPanel.Passphrase ||151 UbuntuSecurityPrivacyPanel.Passphrase ||
154 changeSecurityDialog.oldMethod ===152 changeSecurityDialog.oldMethod ===
@@ -160,6 +158,24 @@
160 }158 }
161 }159 }
162160
161 /* Using bindings since it is, according to documentation,
162 impossible to unset both validator and maximumLength properties */
163 Binding {
164 target: currentInput
165 property: "validator"
166 value: passcodeValidator
167 when: changeSecurityDialog.oldMethod ===
168 UbuntuSecurityPrivacyPanel.Passcode
169 }
170
171 Binding {
172 target: currentInput
173 property: "maximumLength"
174 value: 4
175 when: changeSecurityDialog.oldMethod ===
176 UbuntuSecurityPrivacyPanel.Passcode
177 }
178
163 Label {179 Label {
164 id: incorrect180 id: incorrect
165 text: ""181 text: ""
@@ -197,20 +213,28 @@
197 else213 else
198 return Qt.ImhNone214 return Qt.ImhNone
199 }215 }
200 inputMask: {
201 if (changeSecurityDialog.newMethod ===
202 UbuntuSecurityPrivacyPanel.Passcode)
203 return "9999"
204 else
205 return ""
206 }
207 visible: changeSecurityDialog.newMethod ===216 visible: changeSecurityDialog.newMethod ===
208 UbuntuSecurityPrivacyPanel.Passcode ||217 UbuntuSecurityPrivacyPanel.Passcode ||
209 changeSecurityDialog.newMethod ===218 changeSecurityDialog.newMethod ===
210 UbuntuSecurityPrivacyPanel.Passphrase219 UbuntuSecurityPrivacyPanel.Passphrase
211 // Doesn't get updated if you set this in enabled of confirmButton220 }
212 onTextChanged: confirmButton.enabled =221
213 (acceptableInput && (!visible || text.length > 0))222 /* Using bindings since it is, according to documentation,
223 impossible to unset both validator and maximumLength properties */
224 Binding {
225 target: newInput
226 property: "validator"
227 value: passcodeValidator
228 when: changeSecurityDialog.newMethod ===
229 UbuntuSecurityPrivacyPanel.Passcode
230 }
231
232 Binding {
233 target: newInput
234 property: "maximumLength"
235 value: 4
236 when: changeSecurityDialog.newMethod ===
237 UbuntuSecurityPrivacyPanel.Passcode
214 }238 }
215239
216 Label {240 Label {
@@ -243,19 +267,30 @@
243 else267 else
244 return Qt.ImhNone268 return Qt.ImhNone
245 }269 }
246 inputMask: {
247 if (changeSecurityDialog.newMethod ===
248 UbuntuSecurityPrivacyPanel.Passcode)
249 return "9999"
250 else
251 return ""
252 }
253 visible: changeSecurityDialog.newMethod ===270 visible: changeSecurityDialog.newMethod ===
254 UbuntuSecurityPrivacyPanel.Passcode ||271 UbuntuSecurityPrivacyPanel.Passcode ||
255 changeSecurityDialog.newMethod ===272 changeSecurityDialog.newMethod ===
256 UbuntuSecurityPrivacyPanel.Passphrase273 UbuntuSecurityPrivacyPanel.Passphrase
257 }274 }
258275
276 /* Using bindings since it is, according to documentation,
277 impossible to unset both validator and maximumLength properties */
278 Binding {
279 target: confirmInput
280 property: "validator"
281 value: passcodeValidator
282 when: changeSecurityDialog.newMethod ===
283 UbuntuSecurityPrivacyPanel.Passcode
284 }
285
286 Binding {
287 target: confirmInput
288 property: "maximumLength"
289 value: 4
290 when: changeSecurityDialog.newMethod ===
291 UbuntuSecurityPrivacyPanel.Passcode
292 }
293
259 Label {294 Label {
260 id: notMatching295 id: notMatching
261 wrapMode: Text.Wrap296 wrapMode: Text.Wrap
@@ -303,7 +338,7 @@
303 else338 else
304 return i18n.tr("Set")339 return i18n.tr("Set")
305 }340 }
306 enabled: false341 enabled: newInput.acceptableInput
307 onClicked: {342 onClicked: {
308 changeSecurityDialog.enabled = false343 changeSecurityDialog.enabled = false
309344

Subscribers

People subscribed via source and target branches