[Merge] lp:~artmello/webbrowser-app/webbrowser-app-bookmark_folders into lp:webbrowser-app
Olivier Tilloy
olivier.tilloy at canonical.com
Thu Jul 2 08:44:02 UTC 2015
Review: Needs Fixing
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.
--
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.
More information about the Ubuntu-reviews
mailing list