Review: Abstain
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.
Diff comments:
>
> === modified file 'src/app/qml/AuthenticationService.qml'
> --- src/app/qml/AuthenticationService.qml 2016-01-09 01:47:56 +0000
> +++ src/app/qml/AuthenticationService.qml 2016-02-02 21:39:09 +0000
> @@ -32,6 +32,8 @@
> Component.onCompleted: {
Try to move the declaration of AuthenticationService at the end of
'src/app/qml/ubuntu-terminal-app.qml'.
This 'Component.onCompleted' slot will be executed earlier and the focus will
be stolen by the terminal.
According to the Qt docs[1], "The order of running the onCompleted handlers is
undefined."
Although that seems to happen systematically, in theory neither the
'onCompleted' slot in the main QML file is grant to be executed for last.
Even if the patch works great, it'd be better to do this in a different way. A
small change in one of the other QML documents can potentially break the focus
behaviour.
A workaround is to add a Timer component with interval=1, in order to delay the
execution of the code at the next event loop iteration (i.e. after all the
'onCompleted' slots have been called).
In the AuthenticationDialog, you can set the keyboard focus as follows:
Timer {
interval: 1
running: true
onTriggered: passwordField.forceActiveFocus()
}
If you do that, please add a comment and mark it as "WORKAROUND" for a future
reference, explaining the reason why it has been done so.
See
http://bazaar.launchpad.net/~notes-app-dev/reminders-app/trunk/view/head:/src/app/qml/ui/EditNoteView.qml#L195
as reference.
Otherwise, you can still perform this this at the 'Component.onCompleted' slot
of the main QML file. In that case you'd need to expose a boolean property so
that you can detect whether an authentication is required or not.
======
[1] http://doc.qt.io/qt-5/qml-qtqml-component.html#completed-signal
> if ( systemAuthentication.requireAuthentication() &&
> !noAuthentication) {
> displayLoginDialog();
> + } else {
> + tabsModel.selectTab(0);
> }
> }
>
> @@ -49,6 +51,7 @@
> if ( systemAuthentication.validatePasswordToken( password ) ) {
> granted();
> PopupUtils.close( authentication_dialog );
> + tabsModel.selectTab(0);
AuthenticationService has a 'onGranted' signal which I guess should be use for
performing this operation.
QML aims to create a set of reusable components, so in the 'internal' logic
there should be only the code strictly necessary for the component itself.
See https://www.ics.com/files/qtdocs/qml-extending-types.html
> }
> else {
> var dialog_options = {
>
> === modified file 'src/app/qml/ubuntu-terminal-app.qml'
> --- src/app/qml/ubuntu-terminal-app.qml 2016-01-29 02:16:38 +0000
> +++ src/app/qml/ubuntu-terminal-app.qml 2016-02-02 21:39:09 +0000
> @@ -125,8 +125,4 @@
> model:
> ["GreenOnBlack","WhiteOnBlack","BlackOnWhite","BlackOnRandomLight","Linux","cool-retro-term","DarkPastels","BlackOnLightYellow",
> "Ubuntu"]
> }
> }
> -
> - Component.onCompleted: {
If you go for the tips at the first in-line comment, you can restore the code
you removed here.
> - tabsModel.selectTab(0);
> - }
> }
--
https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/auto-focus-auth/+merge/284502
Your team Ubuntu Terminal Developers is requested to review the proposed merge
of lp:~mcintire-evan/ubuntu-terminal-app/auto-focus-auth into
lp:ubuntu-terminal-app.
--
Mailing list: https://launchpad.net/~ubuntu-touch-coreapps-reviewers
Post to : ubuntu-touch-coreapps-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~ubuntu-touch-coreapps-reviewers
More help : https://help.launchpad.net/ListHelp
