[Merge] lp:~artmello/webbrowser-app/webbrowser-app-history_view_convergence into lp:webbrowser-app

Olivier Tilloy olivier.tilloy at canonical.com
Wed Aug 12 17:40:51 UTC 2015


Some comments on the changes in Browser.qml:

There seems to be inconsistencies in how the focus is handled in historyViewLoader (for both versions of the view):
 - why is internal.resetFocus() called in Keys.onEscapePressed but not in onDone?
 - to avoid the inconsistency above, can’t internal.resetFocus() be called in an else branch in the onStatusChanged handler?

The code for the timer that sets the model asynchronously on the history view is duplicated. It could be factored out, with one single timer as child of historyViewLoader, with its running property set to 'historyViewLoader.active'.

Similarly, I suspect that the Keys.onEscapePressed handler can be de-duplicated by moving it to the loader (to be tested to confirm that this works).

There is an extra whitespace at the end of the onTriggered handler for the history action.

In the Ctrl+H keyboard shortcut handler, no need to check whether historyViewLoader.active is false before setting it to true: if it was true already, this will effectively be a no-op (and there is an extra whitespace at the end of that line too).
-- 
https://code.launchpad.net/~artmello/webbrowser-app/webbrowser-app-history_view_convergence/+merge/263052
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list