Merge lp:~mcintire-evan/ubuntu-terminal-app/auto-focus-auth into lp:~ubuntu-terminal-dev/ubuntu-terminal-app/reboot

[Evan McIntire]

Autofocus the authentication popup at app launch. In addition, only load the terminal page after auth is complete/not required so that it doesnt not steal focus, which allowed users to use a bluetooth keyboard to bypass authentication

Autofocus the authentication popup at app launch

[Evan McIntire (mcintire-evan)]

I haven't been able to test this quite yet - running on an emulator/sandbox has been hard, as the auth dialog only comes up on unity 8

[Stefano Verzegnassi (verzegnassi-stefano)]

Hi Evan!
As long as you only need to test the TextField focus, you can force the auth dialog to appear on desktop too. In AuthenticationService.qml, line 33, you can remove the 'if' condition, so that the dialog will be always shown on any platform.

http://bazaar.launchpad.net/~ubuntu-terminal-dev/ubuntu-terminal-app/reboot/view/head:/src/app/qml/AuthenticationService.qml#L33

As a side note, the auth dialog does not get the focus because of the multiple requests of 'forceActiveFocus()' during initialization, therefore the focus stays on the terminal widget. By fixing this, you will probably fix also the bug at:
  https://bugs.launchpad.net/ubuntu-terminal-app/+bug/1488481/comments/3

[Evan McIntire (mcintire-evan)]

Cool, thanks for the advice! That other bug you linked is pretty nasty so I'll see if I can rework things a little to get them both done :)

[Stefano Verzegnassi (verzegnassi-stefano)]

The bug about the bluetooth keyboard should be easily reproducible on desktop too: just ensure to make the auth dialog visible and type some text with your USB/PS2/... keyboard.

Currently the keyboard focus is owned by the terminal widget, that's the reason why it can be typed some command in the console. Once you properly give the focus to the dialog, there's no one to move the focus to another component (UITK Dialog itself has some internal that prevents this).

If you need some test on a Ubuntu device, feel free to ping me

Keep up the good work! :)

[Evan McIntire (mcintire-evan)]

Yup - I've been able to reproduce it, I almost have a fix ready, just have to fine tune a little bit and we'll be golden! Thanks for the help!

[Evan McIntire (mcintire-evan)]

It seems a bunch of stuff snuck into the POT file, but I suppose it needs to be updated eventually?

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:159
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/987/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/532/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/987/rebuild

review: Approve (continuous-integration)

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:160
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/988/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/533/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/988/rebuild

review: Approve (continuous-integration)

[Stefano Verzegnassi (verzegnassi-stefano)]

I've tested it on my BQ and it works fine! Good work!

However, giving a look at the code I saw there may be some complication in keeping this change working in future.

Mine just wants to be a suggestion on how to prevent this from happening, but I understand there may be some urgency in getting this patch landed and release a new version of terminal-app in the store. For that reason I'll abstain from doing a review.

Anyway, it's great to see new people joining the Core Apps team, so thank you! :)

P.S. I'd wait for Alan, David or Niklas to provide a review before doing any of the changes I suggested.

review: Abstain

[Evan McIntire (mcintire-evan)]

Thanks! I wasn't entirely sold by the way I fixed it, thanks for the ideas! We'll wait until someone else reviews to see where to go :)

[Evan McIntire (mcintire-evan)]

Thanks again Stefano, the timer idea works great in keeping everything working smoothly with the least change, so Im going with that - the new diff will be here in a moment, and _hopefully_ I will have done all the branch stuff correctly

I feel the timer is a tad hacky, but this fixes some important stuff, and we can always take another look at the auth stuff when looking at things like #1416886 and #1493786

Thanks again for the help :)

[Stefano Verzegnassi (verzegnassi-stefano)]

Yes, it's more a workaround than a "rock-solid" solution, but it does the job well and don't require to change things where it's not strictly required.

Below the Timer {}, I see that you left the Component.onCompleted handler. It isn't necessary anymore.

Anyway, LGTM, so there's no reason why I shouldn't approve it. Great work! :)

review: Approve

[Niklas Wenzel (nikwen)]

@Stefano: If you think that this can be merged, feel free to do so. I don't think that we need approvals from everyone all the time. ;)

(To be clear: I haven't had a chance to look into it or test it yet.)

=== modified file 'src/app/qml/AuthenticationDialog.qml'
--- src/app/qml/AuthenticationDialog.qml	2016-01-22 17:50:33 +0000
+++ src/app/qml/AuthenticationDialog.qml	2016-02-13 04:00:46 +0000
@@ -29,6 +29,15 @@
     signal passwordEntered(string password)
     signal dialogCanceled
 
+    // Due to several different things forcing focus
+    // on creation, we simply create this timer to
+    // work around that (see bugs #1488481 and #1499994)
+    Timer {
+        interval: 1
+        running: true
+        onTriggered: passwordField.forceActiveFocus()
+    }
+
     Component.onCompleted: {
         passwordField.forceActiveFocus();
     }
@@ -36,7 +45,6 @@
     TextField {
         id: passwordField
         objectName: "inputField"
-
         placeholderText: i18n.tr("passcode or passphrase")
         echoMode: TextInput.Password