Merge lp:~rpadovani/webbrowser-app/no-duplicates-address-bar into lp:webbrowser-app

[Riccardo Padovani]

Avoid to have duplicate entries in the address bar. Fix #1467078

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1072
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1914/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3269
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/671
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/671
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/671/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/671
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/2730
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3267
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3267/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21359

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

review: Needs Fixing (continuous-integration)

[Olivier Tilloy (osomon)]

Thanks for the bug report, and for working on a fix, Riccardo!

While the patch kind of works, the approach isn’t right, as it special-cases bookmarks in a component (Suggestions.qml) that is generic (it doesn’t know anything about the models populating it).

The approach to remove doublons is ok, but the code should remain generic. Not sure what the best approach for that would be. Maybe add a "displayLimit" parameter to the models, so that e.g. a LimitProxyModel would fetch 4 (or 5, or 10) items, but then only the first 2 that are not doublons would be displayed?

review: Needs Fixing

[Riccardo Padovani (rpadovani)]

Sorry, I'm not sure I understand how to create a generic approach. We have 3 model to merge here: bookmarks, history, and suggestions on google. We want to limit bookmarks and history to two elements, suggestions on google to 5. I need to pass more elements to be sure the aren't doublons (4 is perfect for history, because at least two are different from the two of bookmarks), but how can I limit how many of them I display without counting how many I've already added in that section?

Maybe I've to do a more generic approach creating an array of how many of each section I added? But again, how could I choose if I have to display 5 or 2 without verifing which model it is?

[Olivier Tilloy (osomon)]

Counting how many items you’ve added from one given model is okay. What’s not ok is to check that with hardcoded assertions such as:

    model.icon !== 'non-starred'

because you make assumptions on the models used to populate the suggestions.

[lp:~rpadovani/webbrowser-app/no-duplicates-address-bar]

1073. By Riccardo Padovani on 2015-08-31

Merge with trunk and resolve conflicts

1074. By Riccardo Padovani on 2015-08-31

Avoid hardcoding model name

[Riccardo Padovani (rpadovani)]

Fixed, thanks for the explanation

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1074
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2144/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3906
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/898
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/898
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/898/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/898
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3201
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3903
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3903/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Olivier Tilloy (osomon)]

I’m reliably getting 3 autopilot failures when running the tests on my desktop:

  webbrowser_app.tests.test_suggestions.TestSuggestions.test_keyboard_navigation
  webbrowser_app.tests.test_suggestions.TestSuggestions.test_list_of_suggestions_order
  webbrowser_app.tests.test_suggestions.TestSuggestions.test_list_of_suggestions_bookmark_limits

review: Needs Fixing

[Riccardo Padovani (rpadovani)]

I don't understand the failures: the first and the third are the same: it says there are 4 suggestions, but it expects only 2 suggestions. The strange is there are only two suggestions, so I don't understand why it fails.

The second one expects 5 results, but I think it's right it shows 4, because the first and the second are the same, am I right?

[Olivier Tilloy (osomon)]

Without looking further into the failures, my guess is that with your changes the suggestions are instantiated but not displayed. Autopilot, if not explicitly instructed to do so, will return all matching objects, regardless of whether they are visible or not.

If my guess is correct, there are two possible approaches here:
 - ensure that only exactly the required number of suggestions is instantiated
 - modify the autopilot emulator to query for only *visible* suggestions (have a look at tests/autopilot/webbrowser_app/emulators/browser.py around line 350)

[lp:~rpadovani/webbrowser-app/no-duplicates-address-bar]

1075. By Riccardo Padovani on 2015-09-11

Merge with upstream

1076. By Riccardo Padovani on 2015-09-23

Update test to don't expect duplicate suggestion in the address bar

1077. By Riccardo Padovani on 2015-10-19

Fix test_list_of_suggestions_order

1078. By Riccardo Padovani on 2015-10-19

Fix all the tests

[Riccardo Padovani (rpadovani)]

Actually, the problem was in the way Suggestions count property was calculated, I fixed it.
I ran all the suggestion's tests and they still work, so I think my fix is ok.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1078
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2372/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/4697
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1126
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1126
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1126/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1126
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3790
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4694
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4694/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24328

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

review: Needs Fixing (continuous-integration)

[Olivier Tilloy (osomon)]

In Suggestions.qml:

 - Function countItems() isn’t used any longer, please remove it.

 - For consistency with the rest of the codebase, please remove trailing semi-colons in JS code

 - The forEach branch of the conditional on the type of model needs to be updated to filter out duplicates as well. This works correctly as it is, but it will break if we reshuffle the order of the models, or add new models to the suggestions.

 - Instead of hardcoding the upper limit for j to 2, you should add a 'displayLimit' property on the models.

Also, would you mind taking advantage of this change to add QML unit tests for the Suggestions.qml component? That would allow verifying that your implementation works well in all cases.

review: Needs Fixing

Unmerged revisions

1078. By Riccardo Padovani on 2015-10-19

Fix all the tests

1077. By Riccardo Padovani on 2015-10-19

Fix test_list_of_suggestions_order

1076. By Riccardo Padovani on 2015-09-23

Update test to don't expect duplicate suggestion in the address bar

1075. By Riccardo Padovani on 2015-09-11

Merge with upstream

1074. By Riccardo Padovani on 2015-08-31

Avoid hardcoding model name

1073. By Riccardo Padovani on 2015-08-31

Merge with trunk and resolve conflicts

1072. By Riccardo Padovani on 2015-06-22

Avoid to have duplicate entries in the address bar. Fix #1467078

=== modified file 'src/app/webbrowser/Browser.qml'
--- src/app/webbrowser/Browser.qml	2015-10-13 12:57:33 +0000
+++ src/app/webbrowser/Browser.qml	2015-10-19 08:33:27 +0000
@@ -496,7 +496,7 @@
 
             LimitProxyModel {
                 id: bookmarksSuggestions
-                limit: 2
+                limit: 4 // To be sure they aren't the same of historySuggestions
                 readonly property string icon: "non-starred"
                 readonly property bool displayUrl: true
                 sourceModel: TextSearchFilterModel {

=== modified file 'src/app/webbrowser/Suggestions.qml'
--- src/app/webbrowser/Suggestions.qml	2015-08-24 16:51:47 +0000
+++ src/app/webbrowser/Suggestions.qml	2015-10-19 08:33:27 +0000
@@ -26,7 +26,7 @@
     property var searchTerms
     property var models
 
-    readonly property int count: models.reduce(internal.countItems, 0)
+    readonly property int count: suggestionsList.model.length
     readonly property alias contentHeight: suggestionsList.contentHeight
 
     signal activated(url url)
@@ -56,7 +56,13 @@
             if (model.forEach) {
                 model.forEach(function(item) { modelItems.push(item) })
             } else {
-                for (var i = 0; i < model.count; i++) modelItems.push(model.get(i))
+                for (var i = 0, j = 0; i < model.count && j < 2; i++) {
+                    var item = model.get(i);
+                    if (!list.some(function(elem){return elem["url"] === item.url})) {
+                        modelItems.push(model.get(i));
+                        j++;
+                    }
+                }
             }
 
             modelItems.forEach(function(item) {

=== modified file 'tests/autopilot/webbrowser_app/tests/test_suggestions.py'
--- tests/autopilot/webbrowser_app/tests/test_suggestions.py	2015-07-09 07:44:48 +0000
+++ tests/autopilot/webbrowser_app/tests/test_suggestions.py	2015-10-19 08:33:27 +0000
@@ -213,12 +213,11 @@
         suggestions = self.main_window.get_suggestions()
         self.address_bar.write('QML')
         self.assert_suggestions_eventually_shown()
-        self.assertThat(suggestions.count, Eventually(Equals(5)))
+        self.assertThat(suggestions.count, Eventually(Equals(4)))
         entries = suggestions.get_ordered_entries()
-        self.assertThat(len(entries), Equals(5))
+        self.assertThat(len(entries), Equals(4))
         self.assertThat(entries[0].icon, Equals("history"))
-        self.assertThat(entries[1].icon, Equals("non-starred"))
-        self.assertThat(entries[2].icon, Equals("search"))
+        self.assertThat(entries[1].icon, Equals("search"))
 
     def test_clear_address_bar_dismisses_suggestions(self):
         self.address_bar.focus()