ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

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.