Code review comment for lp:~artmello/webbrowser-app/webbrowser-app-bookmarks_view

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

If I press Ctrl+F while in the bookmarks view and then exit the view, the find-in-page mode has been activated in the current tab. This shortcut should be deactivated while in the bookmarks view. Hint: update the 'enabled' property of the corresponding KeyboardShortcut element.

Similarly, if I press Ctrl+L while in the bookmarks view, it looses focus and I can’t regain it. In general, please make sure that all the app’s keyboard shortcuts don’t have unintended effects while in the view.

A keyboard shortcut to open the view would be useful. Both chromium and firefox on desktop use Ctrl+Shift+O, so we could use that one too.

In UrlDelegate.qml, listActions doesn’t need to be a property of an internal QtObject, it can simply be defined as a child of the top-level item with an id, which you would refer to in the assignment of leadingActions.

In BookmarksView.qml, I don’t think we need to expose the entire settings object as a top-level property. Given that only the homepage setting is used (and in a read-only manner), can you expose just that one?

   property bool isHomeBookmark: folder === "" && index === 0
should reuse isAllBookmarksFolder instead of checking whether folder is empty again.

In general, please try and mark properties that won’t ever be written to with the 'readonly' keyword (such as isAllBookmarksFolder or isHomeBookmark.

And while we’re at it, in BookmarksFoldersView.qml the isHomeBookmark property is referenced only once, so its usefulness as a standalone property is questionable, you could simply transfer the condition to the assignment of 'removable'.

In test_set_bookmark_title, the need for an 'index' variable disappeared, you can simply hardcode the index in the assignment of the 'bookmark' variable.

review: Needs Fixing

« Back to merge proposal