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

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

A few more comments/remarks from a functional review and partial code review:

 - When adding a bookmark, creating a new folder should auto-select it (supposedly when creating a new folder the user wants to save the current bookmark in it).

 - The arrow of the bookmark dialog popover should point at the star, not at the center of the chrome.

 - The dialog title says "bookmark added", however the bookmark is not actually added until after the dialog is closed. If the app crashes or is killed for whatever reason while the dialog is shown, the bookmark will be lost, which is unexpected. Can we add the bookmark right away (by default without a folder, and when changing the folder from the dropdown update it)?

 - The font size of all elements in the bookmark dialog should probably decreased, except maybe that of the title.

 - There should probably be top and bottom margins for the contents of the bookmark dialog.

 - The inclusion of QDateTime in bookmarks-folder-model.h is useless.

 - In BookmarksFolderListModel::setSourceModel(), why do you need to listen to layoutChanged()?

review: Needs Fixing

« Back to merge proposal