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

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

> The "Save in" text is inside the OptionSelector component and it seems
> it does not provide anyway to set the fontSize for the text.

Have you asked the UITK folks about that? What about the other widgets in the popover?

When creating a new folder from the popover, the new folder is selected as expected, but the dropdown remains expanded. I think it should be collapsed.

The __bookmarkToggle property is not a good idea, it breaks encapsulation. Instead, can you add an empty item that is anchored to the bookmark toggle, and have that item exposed as a top-level property? Properties whose names start with a double underscore "__" are not meant to be used in QML, they exist only for testing purposes.

Why does BookmarksModel::update() move the bookmark in the model? The "created" timestamp doesn’t need to be updated (it represents the bookmark creation time, not its last modification time), so the bookmark doesn’t need to be moved.

If BookmarksModel::update() is used only to change the folder, then it should be renamed to updateFolder() or setFolder(), and it should accept only a URL and folder name as parameter (it shouldn’t change the title and icon, unless there is a concrete use case for that).

Can you please rename BookmarksFolderListModel::getIndex() to BookmarksFolderListModel::indexOf() ?

review: Needs Fixing

« Back to merge proposal