Code review comment for lp:~uriboni/webbrowser-app/keyboard-navigation

Revision history for this message
Olivier Tilloy (osomon) wrote :

When in the tabs view, pressing Ctrl+T to open a new tab should probably dismiss the view (just like clicking the "New Tab" button does).

> I suck at naming variables. preventSimplifyText is the best I could
> come up with. Hope it works.

While it works, I must say I don’t like the name very much either. I would prefer 'canSimplifyText' (changing the semantics of the property to its opposite).

> You could possibly generalize some of the database access in a base
> class and allow subclasses to provide only the list of data, but it
> would be much work for little advantage in my opinion.

Yeah, that’s what I was kind of suggesting. We can do that separately later on though.

In AddressBar.qml:
  - updateUrlFromFocus() doesn’t seem to be used externally, can it be made a private (internal) function?
  - I’m not sure I understand why the test on 'preventSimplifyText' is not inside updateUrlFromFocus(). When the address bar has active focus, we always want the text to be expanded, right? So the check would be better placed in the 'else if' branch of the function, no?

As pointed out by Bill in his e-mail review (adding it here for future reference), the following need implementing (per design spec):
 - F11 to toggle fullscreen mode
 - F6 should focus the address bar, just like Ctrl+L and Alt+D

 - and F5 for page reload, as you suggested, would be a useful addition, too

review: Needs Fixing

« Back to merge proposal