Merge lp:~zsombi/ubuntu-ui-toolkit/listViewProxyBug into lp:ubuntu-ui-toolkit/staging

[Zsombor Egri]

ListView proxy should not consume up/down key events.

[Olivier Tilloy (osomon)]

LGTM.

review: Approve

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1887
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/447/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1532/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/447/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1887
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/224/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1533/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/224/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1887
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/201/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1534/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/201/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1887
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/457/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1535/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/457/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1887
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/121/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/447
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/457
        deb: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/457/artifact/artifacts/*zip*/artifacts.zip
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/224
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/201
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1536/console
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1532
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1535
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1533
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1534

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/121/rebuild

review: Approve (continuous-integration)

[Tim Peeters (tpeeters)]

The code looks good, but can you add some comments to make it easier to understand?

For example:
// override up/down key presses for ListView
bool ListViewProxy::keyPressEvent(QKeyEvent *event)

suggests that this overrides the keyPressEvent() function of ListView(), which inherits that function from QQuickItem, but that function has a void return value. Mentioning the value of the return value, and that the returned value will be returned by eventFilter() is probably enough.

review: Needs Fixing

[lp:~zsombi/ubuntu-ui-toolkit/listViewProxyBug]

1888. By Zsombor Egri on 2016-03-14

add more comments

1889. By Zsombor Egri on 2016-03-14

staging sync

[Zsombor Egri (zsombi)]

> The code looks good, but can you add some comments to make it easier to
> understand?
>
> For example:
> // override up/down key presses for ListView
> bool ListViewProxy::keyPressEvent(QKeyEvent *event)
>
> suggests that this overrides the keyPressEvent() function of ListView(), which
> inherits that function from QQuickItem, but that function has a void return
> value. Mentioning the value of the return value, and that the returned value
> will be returned by eventFilter() is probably enough.
Updated.

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

FAILED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/488/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1671/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/488/rebuild

review: Needs Fixing (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/253/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1680/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/253/rebuild

review: Approve (continuous-integration)

[Tim Peeters (tpeeters)]

Looks good, thanks.

The jenkins failure above looks unrelated.

review: Approve

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/476/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1681/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/476/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/230/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1684/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/230/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

FAILED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/146/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/476
    FAILURE: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/488/console
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/253
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/230
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1685/console
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1681
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1671
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1680
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1684

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/146/rebuild

review: Needs Fixing (continuous-integration)

[Tim Peeters (tpeeters)]

E: failed to extract /home/phablet/pbuilder/vivid-armhf.tgz to /home/phablet/pbuilder/tmp/23637
1

Storage full, or /home failed to mount?

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/261/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1719/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/261/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/484/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1720/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/484/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/238/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1721/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/238/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/496/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1722/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/496/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/152/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/484
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/496
        deb: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/496/artifact/artifacts/*zip*/artifacts.zip
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/261
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/238
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1723/console
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1720
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1722
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1719
    SUCCESS: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1721

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci/152/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/486/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1726/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/486/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/264/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1727/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/264/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/240/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1728/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/240/rebuild

review: Approve (continuous-integration)

[ubuntu-sdk-build-bot (ubuntu-sdk-build-bot)]

PASSED: Continuous integration, rev:1889
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/499/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/1731/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/499/rebuild

review: Approve (continuous-integration)

=== modified file 'src/Ubuntu/Components/plugin/privates/listviewextensions.cpp'
--- src/Ubuntu/Components/plugin/privates/listviewextensions.cpp	2016-03-01 15:08:08 +0000
+++ src/Ubuntu/Components/plugin/privates/listviewextensions.cpp	2016-03-14 17:30:57 +0000
@@ -125,7 +125,8 @@
     return false;
 }
 
-// override up/down key presses for ListView
+// override up/down key presses for ListView; returns true if the key event is consumed
+// in which case ListView won't get it.
 bool ListViewProxy::keyPressEvent(QKeyEvent *event)
 {
     int key = event->key();
@@ -138,6 +139,7 @@
     // for horizontal moves take into account the layout mirroring
     bool isRtl = QQuickItemPrivate::get(listView)->effectiveLayoutMirror;
     bool forwards = (key == Qt::Key_Up || (isRtl ? key == Qt::Key_Left : key == Qt::Key_Right));
+    int oldIndex = this->currentIndex();
     int currentIndex = this->currentIndex();
     int count = this->count();
 
@@ -147,5 +149,5 @@
         setKeyNavigationForListView(true);
     }
 
-    return true;
+    return (oldIndex != currentIndex);
 }

=== modified file 'tests/unit_x11/tst_components/tst_listitem_focus.qml'
--- tests/unit_x11/tst_components/tst_listitem_focus.qml	2016-03-01 14:53:44 +0000
+++ tests/unit_x11/tst_components/tst_listitem_focus.qml	2016-03-14 17:30:57 +0000
@@ -412,6 +412,42 @@
                 tryCompare(main.activeFocusItem, "objectName", data.focus[i]);
             }
         }
+
+        SignalSpy {
+            id: upKeySpy
+            signalName: "upPressed"
+        }
+        SignalSpy {
+            id: downKeySpy
+            signalName: "downPressed"
+        }
+        function test_do_not_eat_up_down_key_events_in_listview_bug1554447() {
+            var test = loadTest(listView);
+            test.delegate = listItemWithContent;
+            waitForRendering(test, 500);
+            upKeySpy.target = test.Keys;
+            downKeySpy.target = test.Keys;
+            test.forceActiveFocus();
+
+            // test up
+            test.positionViewAtBeginning();
+            keyClick(Qt.Key_Up);
+
+            upKeySpy.wait(500);
+
+            // test down
+            test.positionViewAtEnd();
+            keyClick(Qt.Key_Down);
+            downKeySpy.wait(500);
+
+            // test both up and down in the middle
+            upKeySpy.clear(); downKeySpy.clear();
+            test.positionViewAtIndex(test.count / 2, ListView.Center);
+            keyClick(Qt.Key_Up);
+            upKeySpy.wait(500);
+            keyClick(Qt.Key_Down);
+            downKeySpy.wait(500);
+        }
     }
 }