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

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

I tested again modifying a bookmark’s title on krillin, and while the experience is poor with the OSK moving the popover out of the way, I think it’s acceptable (as long as it’s only very temporary, and because the title field remains partly visible). No need to disable the title field.

A handful of minor remarks, and we’ll be good to land the feature I think:

 - When the bookmark options popover is displayed, pressing Return should dismiss it and save the bookmark (so that Ctrl+D followed by Return allows bookmarking quickly with the keyboard only).

 - In BookmarkOptions.qml, the color of the "OK" and "Save" buttons should be [UbuntuColors.green] (not exactly #3fb24f, but this will be consistent with all other OK buttons across the app).

 - In TestBookmarkOptions, is populate_config() really useful? The tests operates only on bookmarks, so I don’t think explicitly writing a config file is needed. Or are the tests relying on the fact that the homepage is always shown as a bookmark in the new tab view (if so, a comment in populate_config would be welcome to make that explicit) ?

 - Looking at BookmarkOptions.get_dismiss_button(), it seems this emulator method is only used to retrieve the button to then click it. So maybe a better method would be click_dismiss_button (with the appropriate @autopilot.logging.log_action decorator, see other click_*_button methods in emulators). The same goes for BookmarkOptions.get_new_folder_button().

 - In all new autopilot tests, after clicking the dismiss button, you should call bookmark_options.wait_until_destroyed().

 - tst_BookmarksFolderModelTests.cpp should #include <QtCore/QObject>, and the corresponding CMakeLists.txt should qt5_use_modules(Core) too. Same for tst_BookmarksFolderListModelTests.cpp.

review: Needs Fixing

« Back to merge proposal