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

[Olivier Tilloy]

A few remarks/suggestions, from a very quick read through the diff, in no particular order (and not a complete review yet):

189	+            function show() {
190	+                sourceComponent = bookmarkOptionsDialog
191	+            }

This is unnecessary: instead of a custom show() function, where you would call it just do "bookmarkOptionsLoader.active = true", and have sourceComponent always set to bookmarkOptionsDialog and active set to false by default.

For BookmarkOptions.qml, should we use a Popover component from the UITK? It already handles the automatic dismissal when clicking outside of it, IIRC.

376 +    Q_PROPERTY(QDateTime lastAddition READ lastAddition NOTIFY lastAdditionChanged)

Do we really need this "lastAddition" property (and role)? I was under the impression that we would always display the list of bookmarks sorted alphabetically, not by recency. BookmarksFolderListChronologicalModel would become unnecessary too.

790	+        Empty

I’m not sure this role is very useful. Since there is an "Entries" role already, it’s easy to check whether the list of entries is empty. I’d remove it.

903	+    emit folderInserted("");

Can you please use Q_EMIT everywhere instead of emit?
Can you rename the "folderInserted" signal to "folderAdded"? Similarly, "insertNewFolder" should be renamed to "addFolder".

The SQL "bookmarks" table should probably have a foreign key constraint on folderId. See https://www.sqlite.org/foreignkeys.html.
-- 
https://code.launchpad.net/~artmello/webbrowser-app/webbrowser-app-bookmark_folders/+merge/260410
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.