Merge into trunk : webbrowser-app-bookmark_folders : Code : webbrowser-app

Reviewer	Review Type	Date Requested	Status
Olivier Tilloy		2015-05-28	Approve on 2015-07-03
PS Jenkins bot	continuous-integration		Approve on 2015-07-02
Bill Filler (community)			Needs Fixing on 2015-06-17
Review via email: mp+260410@code.staging.launchpad.net

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-05-28

1032. By Arthur Mello on 2015-05-28: Remove empty line

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-28:

#

FAILED: Continuous integration, rev:1032
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1832/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/2999/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/589/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/589/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/589/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/2997/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1832/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-04

1033. By Arthur Mello on 2015-06-02: Merge with trunk
1034. By Arthur Mello on 2015-06-04: Move the Folder data to a specific table in Database
1035. By Arthur Mello on 2015-06-04: Update models to handle the new bookmarks folders table
1036. By Arthur Mello on 2015-06-04: Add support to the default empty folder
1037. By Arthur Mello on 2015-06-04: Fix QML issues
1038. By Arthur Mello on 2015-06-04: Revert chnages in NewTabView
1039. By Arthur Mello on 2015-06-04: Merge with trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-04:

#

FAILED: Continuous integration, rev:1039
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1850/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3080/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/607/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/607/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/607/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3078/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1850/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-04:

#

Merged in the latest trunk, and tst_BookmarksModelTests.cpp fails to build.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-04:

#

After resolving the trivial build issue pointed out above, I’m doing some quick functional testing, and creating a new bookmark folder doesn’t seem to work: I’m bookmarking a page that wasn’t previously bookmarked, the popover appears, I click on "New Folder", enter a new name (btw pressing the Enter key should validate), click "Choose Folder", but the dropdown list still has only "All Bookmarks".

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-04:

#

"CREATE TABLE IF NOT EXISTS bookmarks_folders" …

Can the table be named "folders", please? The "bookmarks_" prefix is redundant, given that the database name is "bookmarks" already.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-04:

#

33 + if (state == "exisitngFolder") {

typo

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-04:

#

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.

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-04

1040. By Arthur Mello on 2015-06-04: Merge with trunk
1041. By Arthur Mello on 2015-06-04: Fix build issue
1042. By Arthur Mello on 2015-06-04: Change the bookmarks folders table name
1043. By Arthur Mello on 2015-06-04: Fix typo
1044. By Arthur Mello on 2015-06-04: Remove unnecessary show function
1045. By Arthur Mello on 2015-06-04: Remove both the lastAddition property and the folder list chronological model
1046. By Arthur Mello on 2015-06-04: Remove empty property
1047. By Arthur Mello on 2015-06-04: Use Q_EMIT instead of emit
Rename methods names

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-04:

#

FAILED: Continuous integration, rev:1047
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1854/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3092/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/611/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/611/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/611/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3090/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1854/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-04

1048. By Arthur Mello on 2015-06-04: Use the Popover component from the SDK for the BookmarkOptions

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-04:

#

FAILED: Continuous integration, rev:1048
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1855/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3093/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/612/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/612/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/612/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3091/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1855/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-04

1049. By Arthur Mello on 2015-06-04: Add a new Dialog to add a new folder in BookmarkOptions
1050. By Arthur Mello on 2015-06-04: Create API in the foldersListModel to create a new folder
1051. By Arthur Mello on 2015-06-04: Add new unittest for the createNewFolder function

Revision history for this message

Arthur Mello (artmello) wrote on 2015-06-04:

#

> The SQL "bookmarks" table should probably have a foreign key constraint on
> folderId. See https://www.sqlite.org/foreignkeys.html.

It seems that Foreign Key support is disabled by default and need to be enabled setting the PRAGMA foreign_keys. But it is not possible to enable it in a multi-statement transaction so we would need to set it before each relevant transaction in the bookmarks-model. I think that could be error prone but can be done for sure. What do you think? Or maybe there is another way to do it that I am missing.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-04:

#

FAILED: Continuous integration, rev:1051
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1856/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3094/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/613/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/613/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/613/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3092/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1856/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Arthur Mello (artmello) wrote on 2015-06-04:

#

> After resolving the trivial build issue pointed out above, I’m doing some
> quick functional testing, and creating a new bookmark folder doesn’t seem to
> work: I’m bookmarking a page that wasn’t previously bookmarked, the popover
> appears, I click on "New Folder", enter a new name (btw pressing the Enter key
> should validate), click "Choose Folder", but the dropdown list still has only
> "All Bookmarks".

The BookmarkOptions component had 2 states: choosing an existing folder and creating a new folder. We would only create a new folder if the user dismiss the component in the "creating a new folder" state. So when you clicked "Choose Folder" you cancelled the creation of the new folder. But, as we can see, that is not user friendly. I changed to show a dialog when you click "New Folder" and when it is confirmed we will create the folder and it will show up in the dropdown. Please, let me know what you think about it.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-05:

#

> > The SQL "bookmarks" table should probably have a foreign key constraint on
> > folderId. See https://www.sqlite.org/foreignkeys.html.
>
> It seems that Foreign Key support is disabled by default and need to be
> enabled setting the PRAGMA foreign_keys. But it is not possible to enable it
> in a multi-statement transaction so we would need to set it before each
> relevant transaction in the bookmarks-model. I think that could be error prone
> but can be done for sure. What do you think? Or maybe there is another way to
> do it that I am missing.

Looks like too much hassle for little benefit. Let’s forget about it.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-05:

#

> > After resolving the trivial build issue pointed out above, I’m doing some
> > quick functional testing, and creating a new bookmark folder doesn’t seem to
> > work: I’m bookmarking a page that wasn’t previously bookmarked, the popover
> > appears, I click on "New Folder", enter a new name (btw pressing the Enter
> key
> > should validate), click "Choose Folder", but the dropdown list still has
> only
> > "All Bookmarks".
>
> The BookmarkOptions component had 2 states: choosing an existing folder and
> creating a new folder. We would only create a new folder if the user dismiss
> the component in the "creating a new folder" state. So when you clicked
> "Choose Folder" you cancelled the creation of the new folder. But, as we can
> see, that is not user friendly. I changed to show a dialog when you click "New
> Folder" and when it is confirmed we will create the folder and it will show up
> in the dropdown. Please, let me know what you think about it.

This looks better, although I would prefer not to have an extra button. Maybe a special entry in the dropdown to create a new folder? In any case design will have to comment on this, so let’s leave it as is for now, it works.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-05:

#

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

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-08

1052. By Arthur Mello on 2015-06-08: Set the optionSelector to the new created folder
1053. By Arthur Mello on 2015-06-08: Point the arrow of the bookmark dialog popover should at the star
1054. By Arthur Mello on 2015-06-08: Add the bookmark as soon as the toggle is clicked and update the data after
1055. By Arthur Mello on 2015-06-08: Add a top and a bottom margin to the bookmarkOptions
1056. By Arthur Mello on 2015-06-08: Remove unnecessary include
1057. By Arthur Mello on 2015-06-08: Stop connecting to unnecessary signal
1058. By Arthur Mello on 2015-06-08: Merge with trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-08:

#

FAILED: Continuous integration, rev:1058
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1859/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3113/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/616/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/616/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/616/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3111/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1859/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Arthur Mello (artmello) wrote on 2015-06-08:

#

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

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

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-09:

#

> 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

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-09

1059. By Arthur Mello on 2015-06-09: Do not change the icon and the created roles when updating bookmark
1060. By Arthur Mello on 2015-06-09: Rename method

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-09:

#

FAILED: Continuous integration, rev:1060
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1861/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3117/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/618/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/618/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/618/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3115/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1861/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-09

1061. By Arthur Mello on 2015-06-09: Expose an item to work as a place holder for the bookmark toggle
1062. By Arthur Mello on 2015-06-09: Colapse the OptionSelector after we add a new folder
1063. By Arthur Mello on 2015-06-09: Reduce font size of tex

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-09:

#

FAILED: Continuous integration, rev:1061
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1863/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3128/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/620/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/620/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/620/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3126/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1863/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-09:

#

FAILED: Continuous integration, rev:1063
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1864/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3129/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/621/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/621/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/621/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3127/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1864/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-10:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1868/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-12

1064. By Arthur Mello on 2015-06-11: Merge with trunk
1065. By Arthur Mello on 2015-06-11: Limit the size of the OptionSelector when expanded
1066. By Arthur Mello on 2015-06-12: Show the bookmarks folders when the user click in see more bookmarks
1067. By Arthur Mello on 2015-06-12: Merge with trunk

Revision history for this message

Arthur Mello (artmello) wrote on 2015-06-12:

#

Following some Bill's suggestions, I am showing the bookmark folders when the user click in the "see more" bookmarks button. Now, when the user click in the "see more" we show all the bookmarks separated by folder. All the folders are expanded and we can collapse them if we click in the folder name. Since that was not specified by the design team any feedback about the UX will be really appreciated

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-12:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1880/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-15:

#

Revision 1066 breaks autopilot tests. Please run the tests locally before pushing new revisions.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-15:

#

When expanding the list of favorites to display the bookmarks grouped by folder, empty folders should probably not be displayed (including "All Bookmarks" if it’s empty).

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-15:

#

238 + text: {
239 + var ret
240 + if (bookmarksFolderColumn.expanded) {
241 + ret = "- "
242 + } else {
243 + ret = "+ "
244 + }
245 +
246 + if (folder) {
247 + return ret + folder
248 + } else {
249 + return ret + i18n.tr("All Bookmarks")
250 + }
251 + }

This should be properly internationalized: exotic languages might have different ways of signifying that a section is expanded/collapsed than a minus/plus sign (not to mention RTL languages where the sign would probably be incorrectly placed).

review: Needs Fixing

Revision history for this message

Bill Filler (bfiller) wrote on 2015-06-15:

#

Some comments that need to be fixed (some dupes of Olivier's). You should run on the device to see some of these.

1) turn off predictive text hint for entry field in Save dialog
2) pressing the Save button in the dialog doesn't close it on the device, it only dismisses the keyboard but doesn't close the dialog
3) I think we really need a "Dismiss" button in the initial popover. Not clear how to close it, especially on device
4) On device, if the list of bookmark folders is too big it pushes the popover above the url field
5) On the bookmarks page, the "More" button is too big. I thought this was supposed to be a "See more.." link at the bottom of the bookmarks list, not a button to the right? If we leave it a button, please reduce the size. Same with less button.
6) Hide folders that are empty in the more view
7) Instead of using the + -, use right arrow and down arrow icons if we have them, if not don't show anything. Just allow clicking the section to expand/collapse.

review: Needs Fixing

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-17

1068. By Arthur Mello on 2015-06-15: Merge with trunk
1069. By Arthur Mello on 2015-06-16: Merge with trunk
1070. By Arthur Mello on 2015-06-17: Do not display empty folders
Show icons instead of +/- as symbols for folders names
1071. By Arthur Mello on 2015-06-17: Turn off predictive text hint for entry field in Save dialog
1072. By Arthur Mello on 2015-06-17: Add a dismiss button to the BookmarkOptions dialog

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-17:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1897/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-17

1073. By Arthur Mello on 2015-06-17: Change how the top and the bottom margin is handled in the BookmarkOptions so it will not push above the url field
1074. By Arthur Mello on 2015-06-17: Reduce the overall size of the BookmarkOptions component so it will not override url bar

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-17:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1898/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-17

1075. By Arthur Mello on 2015-06-17: Dismiss BookmarkOptions during AP tests
1076. By Arthur Mello on 2015-06-17: Get delegates from the "All Bookmarks" folder to fix AP test
1077. By Arthur Mello on 2015-06-17: Fix AP test to work with new bookmarks folder list view

Revision history for this message

Arthur Mello (artmello) wrote on 2015-06-17:

#

> 5) On the bookmarks page, the "More" button is too big. I thought this was
> supposed to be a "See more.." link at the bottom of the bookmarks list, not a
> button to the right? If we leave it a button, please reduce the size. Same
> with less button.

Thanks a lot for reviewing this. If you agree, I think it would be better handle this in a different MR. The button was not changed by this, but was applied during the changes for the new tab view. I will talk with the design team about it.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-17:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1899/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-17:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1900/rebuild

review: Approve (continuous-integration)

Revision history for this message

Bill Filler (bfiller) wrote on 2015-06-17:

#

Good fixes, still seeing 2 problems:
1) pressing the Save button in the dialog doesn't close it on the device, it only dismisses the keyboard but doesn't close the dialog. You have to press it twice to make it go away

2) after pressing it a second time, the popover is shown but you see the selection in the list switching to the new folder name and it takes a while. I would expect the new folder name to already be selected in the list before the popover becomes visible

review: Needs Fixing

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-17

1078. By Arthur Mello on 2015-06-17: Make sure that the clicked signal is triggered in the new folder dialog
1079. By Arthur Mello on 2015-06-17: Reduce BookmarkOptions space

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-17:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1905/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-17:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1906/rebuild

review: Approve (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-19

1080. By Arthur Mello on 2015-06-19: Merge with trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-19:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1910/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-22:

#

124 + text: i18n.tr("Ok")

Spelling is incorrect, this should be "OK" (see https://code.launchpad.net/~mterry/webbrowser-app/Ok/+merge/260630).

review: Needs Fixing

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-22

1081. By Arthur Mello on 2015-06-22: Fix text
1082. By Arthur Mello on 2015-06-22: Merge with trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-22:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1915/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-22:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1917/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-06-24

1083. By Arthur Mello on 2015-06-23: Merge with trunk
1084. By Arthur Mello on 2015-06-23: Fix AP test and Ctrl-D call to bookmark URL
1085. By Arthur Mello on 2015-06-23: Fix creation of bookmark folder in suggestions AP test
1086. By Arthur Mello on 2015-06-23: Add AP tests to the NewTabView
1087. By Arthur Mello on 2015-06-24: Add Bookmark Options AP tests
1088. By Arthur Mello on 2015-06-24: Merge with trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-24:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1926/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-06-30:

#

Download full text (3.6 KiB)

Test failure:

- webbrowser_app.tests.test_bookmark_options.TestBookmarkOptions.test_set_bookmark_title reliably fails when run on krillin (with the latest vivid image). In fact the this is a functional issue, as the popover is partially scrolled out of view when the OSK slides in.

Functional issues:

- Pressing Ctrl+D to bookmark a page should bring up the popover too.

- When the popover is visible, pressing ESC should dismiss it and remove the bookmark, Return should dismiss it and save the bookmark (similar to desktop chromium’s behaviour).

UX issue, which I discussed with James earlier today:

- Clicking the star or pressing Ctrl+D for a page that is already bookmarked should allow editing the bookmark, instead of removing it. This requires further design work though, so let’s keep it as it is for now.

Tests coverage:

- bookmarks-folderlist-model.cpp has only 70.6% coverage, this should be improved.
- bookmarks-model.cpp has 93.8% coverage, which is ok, but we’re not testing initially populating a model with existing folders. This should be tested.

Code remarks:

- In BookmarkOptions.qml:
   * It doesn’t look like selectorDelegate is used in several places, perhaps it doesn’t need to be enclosed in a separate component, but rather be set directly as folderOptionSelector.delegate?
   * Can you add translator comments for the "Name" and "Save in" strings? Without context it’s not necessarily obvious how to translate those.
   * In titleTextField (and other places), please define anchors instead of setting width to parent.width.
   * Not sure why newFolderButton and okButton are inside a Row, but if you want to ensure that one is anchored to the left and the other one to the right, that’s not the right solution.
   * In newFolderDialog.saveButton, is there a bug report we could link to for the issue with onClicked not being triggered?

- In BookmarksFolderDelegate.qml:
* objectName doesn’t need to include the folderName. Instead, in autopilot tests, you can filter both on 'objectName' ("bookmarkFolderDelegate") and on the 'folderName' property.
* There are two references to bookmarksFolderListViewItem, which isn’t defined in the scope of this component, this breaks encapsulation. In fact it doesn’t look like BookmarksFolderDelegate really needs to be a separate QML file (it’s not re-used anywhere else, is it?), so I think it would be easier to just inline it in BookmarksFolderListView.

- In BookmarksFolderListView.qml:
* The BookmarksFolderDelegate instance that serves as a delegate doesn’t need to be explicitly enclosed in a Component, QML does that automatically.

- In Browser.qml:
* In the BookmarkOptions instance, the binding of bookmarkTitle to browser.currentWebview.title has undesired side effects if the page updates the title periodically (test e.g. with http://htmlmarquee.com/title.html). Instead, the title needs to be retrieved on the current page and set in Component.onCompleted.

- In bookmarks-folderlist-model.cpp:
* The documentation says that each item has three roles, but they have two.
* BookmarksFolderListModel::data() should probably call checkValidFolderIndex(index.row()) to ensure ...

Test failure:

- webbrowser_app.tests.test_bookmark_options.TestBookmarkOptions.test_set_bookmark_title reliably fails when run on krillin (with the latest vivid image). In fact the this is a functional issue, as the popover is partially scrolled out of view when the OSK slides in.

Functional issues:

- Pressing Ctrl+D to bookmark a page should bring up the popover too.

- When the popover is visible, pressing ESC should dismiss it and remove the bookmark, Return should dismiss it and save the bookmark (similar to desktop chromium’s behaviour).

UX issue, which I discussed with James earlier today:

- Clicking the star or pressing Ctrl+D for a page that is already bookmarked should allow editing the bookmark, instead of removing it. This requires further design work though, so let’s keep it as it is for now.

Tests coverage:

- bookmarks-folderlist-model.cpp has only 70.6% coverage, this should be improved.
 - bookmarks-model.cpp has 93.8% coverage, which is ok, but we’re not testing initially populating a model with existing folders. This should be tested.

Code remarks:

- In BookmarkOptions.qml:
   * It doesn’t look like selectorDelegate is used in several places, perhaps it doesn’t need to be enclosed in a separate component, but rather be set directly as folderOptionSelector.delegate?
   * Can you add translator comments for the "Name" and "Save in" strings? Without context it’s not necessarily obvious how to translate those.
   * In titleTextField (and other places), please define anchors instead of setting width to parent.width.
   * Not sure why newFolderButton and okButton are inside a Row, but if you want to ensure that one is anchored to the left and the other one to the right, that’s not the right solution.
   * In newFolderDialog.saveButton, is there a bug report we could link to for the issue with onClicked not being triggered?

- In BookmarksFolderDelegate.qml:
   * objectName doesn’t need to include the folderName. Instead, in autopilot tests, you can filter both on 'objectName' ("bookmarkFolderDelegate") and on the 'folderName' property.
   * There are two references to bookmarksFolderListViewItem, which isn’t defined in the scope of this component, this breaks encapsulation. In fact it doesn’t look like BookmarksFolderDelegate really needs to be a separate QML file (it’s not re-used anywhere else, is it?), so I think it would be easier to just inline it in BookmarksFolderListView.

- In BookmarksFolderListView.qml:
   * The BookmarksFolderDelegate instance that serves as a delegate doesn’t need to be explicitly enclosed in a Component, QML does that automatically.

- In Browser.qml:
   * In the BookmarkOptions instance, the binding of bookmarkTitle to browser.currentWebview.title has undesired side effects if the page updates the title periodically (test e.g. with http://htmlmarquee.com/title.html). Instead, the title needs to be retrieved on the current page and set in Component.onCompleted.

- In bookmarks-folderlist-model.cpp:
   * The documentation says that each item has three roles, but they have two.
   * BookmarksFolderListModel::data() should probably call checkValidFolderIndex(index.row()) to ensure that the the row is valid.

- In bookmarks-model.cpp:
   * In the while loop in BookmarksModel::populateFromDatabase(), you could use a local variable to store the value of populateFolderQuery.value(1).toString(), instead of querying it twice.
   * In BookmarksModel::addFolder(), what does "if (newFolderId)" check? Can the boolean test be made explicit, i.e. "if (newFolderId != 0)" ?
   * The above also applies to the if statement in BookmarksModel::updateExistingEntryInDatabase().

review: Needs Fixing

Revision history for this message

Arthur Mello (artmello) wrote on 2015-07-01:

#

> * In newFolderDialog.saveButton, is there a bug report we could link to for
> the issue with onClicked not being triggered?

I was not able to find one. I will talk with Sheldon and check if there is one, since he was the one that pointed out the issue. If there is not I will create one

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-07-01

1089. By Arthur Mello on 2015-06-30: Merge with trunk
1090. By Arthur Mello on 2015-06-30: Show the Bookmark Options popup when using Ctrl + D shortcut
1091. By Arthur Mello on 2015-06-30: Make the ESC key to close the BookmarkOptions popup
1092. By Arthur Mello on 2015-06-30: Improve unit test coverage for BookmarksFolderListModel
1093. By Arthur Mello on 2015-07-01: Add unittest to verify that bookmark model is populated with existing folders
1094. By Arthur Mello on 2015-07-01: Do not make OptionSelectorDelegate be inside another component
1095. By Arthur Mello on 2015-07-01: Add translator comments
1096. By Arthur Mello on 2015-07-01: Use anchors instead of setting width
1097. By Arthur Mello on 2015-07-01: Remove unnecessary Row
1098. By Arthur Mello on 2015-07-01: Does not set the folderName to the objectName
1099. By Arthur Mello on 2015-07-01: Remove BookmarksFolderDelegate and move code to inside BookmarksFolderListView
1100. By Arthur Mello on 2015-07-01: Does not enclose the delegate inside a Component
1101. By Arthur Mello on 2015-07-01: Do not bind the Bookmark Title in Bookmark Options to the currentWebView.title
1102. By Arthur Mello on 2015-07-01: Update doc for BookmarksFolderListModel
1103. By Arthur Mello on 2015-07-01: Call checkValidFolderIndex to ensure that the row is valid in BookmarksFolderListModel
1104. By Arthur Mello on 2015-07-01: Stop querying the value of populateFolderQuery more than once
1105. By Arthur Mello on 2015-07-01: Make the if statement more explicit
1106. By Arthur Mello on 2015-07-01: Make the if statement more explicit
1107. By Arthur Mello on 2015-07-01: Fix AP tests
1108. By Arthur Mello on 2015-07-01: Add AP test to check if "ESC" will unbookmark the URL
1109. By Arthur Mello on 2015-07-01: Skip test based in bug opened against SDK

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1938/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-07-01:

#

There are two remaining places in Browser.qml where a bookmark is created without the popover being displayed: HUD actions (which is defunct code, but until we remove it it should be kept up-to-date) and contextual actions.

The BookmarkOptions component should gain an additional 'url' property, whose value would be set to browser.currentWebview.url at construction time. This would ensure that the url doesn’t change while it’s shown. If the webview’s current URL changes while the popover is shown, the user can potentially try to unbookmark a different URL than the one that was bookmarked.

It’s wrong to skip test_set_bookmark_title. It’s not just the test that fails, it’s the actual functionality that’s broken. So we can’t land this branch until the UITK fix lands. By the way, have you tested the UITK branch (the fix appears to be merged in staging) to confirm that it fixes our issue?

If I press Ctrl+D to create a bookmark, then Ctrl+D again to remove it, the BookmarkOptions popover doesn’t disappear. If I then press Ctrl+D again to re-create the bookmark, I end up with two instances of the same popover.

review: Needs Fixing

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-07-01

1110. By Arthur Mello on 2015-07-01: Add bug report for the not emitted clicked signal issue
1111. By Arthur Mello on 2015-07-01: Call the BookmarkOptions popover for all the actions that add a bookmark
1112. By Arthur Mello on 2015-07-01: Add a bookmarkUrl property to BookmarkOptions set to the bookmarked url in construction time
1113. By Arthur Mello on 2015-07-01: Hide the BookmarkOptions if the Ctrl+D is pressed again

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1939/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1940/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-07-01

1114. By Arthur Mello on 2015-07-01: Remove unecessary code
1115. By Arthur Mello on 2015-07-01: Use a single function to add bookmark and show popover

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1941/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-07-01

1116. By Arthur Mello on 2015-07-01: Bookmark name field should have predictive text turned off

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1942/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-07-01

1117. By Arthur Mello on 2015-07-01: Do not show BookmarkOptions for the contextual action
1118. By Arthur Mello on 2015-07-01: Improve skip message for AP test

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1945/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-07-01

1119. By Arthur Mello on 2015-07-01: Fix add to bookmark call

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1946/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1947/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-07-02:

#

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

lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders updated on 2015-07-02

1120. By Arthur Mello on 2015-07-02: Add missing include to unittests
1121. By Arthur Mello on 2015-07-02: Add check to make sure that bookmark options is closed
1122. By Arthur Mello on 2015-07-02: Change get buttons methods click buttons
1123. By Arthur Mello on 2015-07-02: Remove populate_config call
1124. By Arthur Mello on 2015-07-02: Change icon color
1125. By Arthur Mello on 2015-07-02: Accept return key to close bookmark options

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-07-02:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1950/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-07-03:

#

This looks good now.

review: Approve

webbrowser-app

Merge lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders into lp://staging/webbrowser-app

Commit message

Description of the change

Preview Diff

Subscribers