Ubuntu UI Toolkit

Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/paperChase into lp:ubuntu-ui-toolkit/staging

paperChase
Merge into staging

Proposed by Christian Dywan on 2015-02-26

Status:

Merged

Approved by:

Tim Peeters on 2015-03-03

Approved revision:

1424

Merged at revision:

1425

Proposed branch:

lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/paperChase

Merge into:

lp:ubuntu-ui-toolkit/staging

Diff against target:

137 lines (+79/-14)

2 files modified

modules/Ubuntu/Components/Popups/Popover.qml (+8/-0)
tests/unit_x11/tst_components/tst_popover.qml (+71/-14)

To merge this branch:

bzr merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/paperChase

Related bugs:

Bug #1199502: [popover] Popover does not follow caller if it moves	High	Fix Released
Bug #1415532: copy/paste popover stays still when scrolling page	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-03-03
Tim Peeters		2015-02-26	Approve on 2015-03-03
Review via email: mp+251038@code.launchpad.net

Commit Message

Update popover position upon pointer movement

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1483/rebuild

review: Needs Fixing (continuous-integration)

Tim Peeters (tpeeters) wrote on 2015-02-26:

8 +
9 + Connections {
10 + target: pointerTarget
11 + onXChanged: internal.updatePosition()
12 + onYChanged: internal.updatePosition()
13 + }

The pointer points to the middle of the target in either x or y direction, and it aligns with the edge of it in the other direction. So changes in width or height of the target need to trigger an update as well.

review: Needs Fixing

Tim Peeters (tpeeters) wrote on 2015-02-26:

23 + useDeprecatedToolbar: false

I propose to update the import versions (and (C)-year) in tst_popover.qml. If you decide to go for Ubuntu.Components version 1.2, useDeprecatedToolbar: false is not needed.

Tim Peeters (tpeeters) wrote on 2015-02-26:

Can we have a regression test that is clearly marked as such (with the bug number in the function name), which fails without the proposed updates to Popover.qml?

lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/paperChase updated on 2015-02-26

1420. By Christian Dywan on 2015-02-26: Take also width and height changes into account
1421. By Christian Dywan on 2015-02-26: Bump imports for Test and Popups inx11…tst_popover

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1486/rebuild

review: Needs Fixing (continuous-integration)

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1487/rebuild

review: Needs Fixing (continuous-integration)

Tim Peeters (tpeeters) wrote on 2015-02-26:

I created a little test program, see http://paste.ubuntu.com/10437208/
I would like to see this program (or something similar) added somewhere in tests/resources.

Also, the results with the test program are not as I expected. When you click the button to open the popover, and then drag the dragging area to move the button, I expect the popover to follow the button. This does not happen.

review: Needs Fixing

Tim Peeters (tpeeters) wrote on 2015-02-26:

tiny update to the test program: http://paste.ubuntu.com/10437251/

Tim Peeters (tpeeters) wrote on 2015-02-26:

Note that it works perfect if the position of the pointer target updates, so for example:
PopupUtils.open(popoverComponent, dragRectangle);

with dragRectangle instead of the popButton.

Tim Peeters (tpeeters) wrote on 2015-02-27:

> I would like to see this program (or something similar) added somewhere in
> tests/resources.

I missed that we already have a resources/popover/ directory. The test program can be added there, as it tests moving of the pointer target, which the other popover tests do not test.

lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/paperChase updated on 2015-03-02

1422. By Christian Dywan on 2015-03-02: Unit test popover arrow direction and parent displacement

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1501/rebuild

review: Needs Fixing (continuous-integration)

lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/paperChase updated on 2015-03-03

1423. By Christian Dywan on 2015-03-03: Uncomment Moving parent tag with a bug link
1424. By Christian Dywan on 2015-03-03: Forgotten comment markers

Christian Dywan (kalikiana) wrote on 2015-03-03:

I filed bug 1427557 for the parent movement case, as I don't think it's feasible to fix it here since it will need a hierarchy monitoring feature that we don't have right now (it's not possible to track global coordinates).

Tim Peeters (tpeeters) wrote on 2015-03-03:

Ok, that's acceptable :)

Thanks.

review: Approve

PS Jenkins bot (ps-jenkins) on 2015-03-03:

review: Approve (continuous-integration)

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1505/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Dan Chapman 

Tom Mirmar

Ubuntu SDK team

 === modified file 'modules/Ubuntu/Components/Popups/Popover.qml'
 --- modules/Ubuntu/Components/Popups/Popover.qml	2014-11-25 15:39:38 +0000
 +++ modules/Ubuntu/Components/Popups/Popover.qml	2015-03-03 08:49:03 +0000
@@ -289,4 +289,12 @@
      onHeightChanged: internal.updatePosition()
      /*! \internal */
      onRotatingChanged: hide()
++
++    Connections {
++        target: pointerTarget
++        onXChanged: internal.updatePosition()
++        onYChanged: internal.updatePosition()
++        onWidthChanged: internal.updatePosition()
++        onHeightChanged: internal.updatePosition()
++    }
+ }
 === modified file 'tests/unit_x11/tst_components/tst_popover.qml'
 --- tests/unit_x11/tst_components/tst_popover.qml	2014-06-09 08:26:24 +0000
 +++ tests/unit_x11/tst_components/tst_popover.qml	2015-03-03 08:49:03 +0000
@@ -15,24 +15,52 @@
   */
  import QtQuick 2.0
  import QtTest 1.0
--import Ubuntu.Test 0.1
--import Ubuntu.Components 0.1
--import Ubuntu.Components.Popups 0.1
++import Ubuntu.Test 1.0
++import Ubuntu.Components 1.2
++import Ubuntu.Components.Popups 1.0
  MainView {
      id: main
--    width: units.gu(40)
++    width: units.gu(50)
      height: units.gu(71)
--    Button {
--        id: caller
++    Rectangle {
++        id: rect
++        y: main.height / 2
++        height: units.gu(10)
++        width: height
++
++        Button {
++            id: pressMe
++            anchors.top: parent.top
++            text: "Press me"
++            onClicked: {
++                testCase.resetPositions();
++                var popover = PopupUtils.open(popoverComponent, pressMe);
++                popoverSpy.target = testCase.findChild(popover, "popover_foreground");
++                popoverSpy.clear();
++                pressMe.parent.height = units.gu(25)
++                pressMe.anchors.top = parent.bottom
++            }
++        }
++
++        Button {
++            id: pushMe
++            anchors.bottom: parent.bottom
++            text: "Push me"
++            onClicked: {
++                testCase.resetPositions();
++                var popover = PopupUtils.open(popoverComponent, pushMe);
++                popoverSpy.target = testCase.findChild(popover, "popover_foreground");
++                popoverSpy.clear();
++                rect.y = main.height / 10
++            }
++        }
++    }
++    Label {
++        id: other
++        text: "Ignore me"
          anchors.centerIn: parent
--        text: "Press me"
--        onClicked: {
--            var popover = PopupUtils.open(popoverComponent, caller);
--            popoverSpy.target = testCase.findChild(popover, "popover_foreground");
--            popoverSpy.clear();
--        }
+     }
      // spy to listen on the popover foreground's hideCompleted() signal
@@ -58,7 +86,13 @@
          name: "PopoverTests"
          when: windowShown
++        function resetPositions() {
++            pressMe.parent.height = units.gu(10)
++            rect.y = main.height / 2
++        }
++
          function cleanup() {
++            resetPositions()
              popoverSpy.target = null;
              popoverSpy.clear();
              waitForRendering(main, 500);
@@ -73,12 +107,35 @@
+         }
          function test_dismiss_on_click(data) {
--            mouseClick(caller, caller.width / 2, caller.height / 2);
--            waitForRendering(caller);
++            mouseClick(pressMe, pressMe.width / 2, pressMe.height / 2);
++            waitForRendering(pressMe);
              verify(popoverSpy.target !== null, "The popover did not open");
++
              // dismiss
              mouseClick(main, 10, 10, data.button);
              popoverSpy.wait();
+         }
++
++        function test_popover_follows_pointerTarget_bug1199502_data() {
++            return [
++                { tag: "Moving pointerTarget", button: pressMe, dir: "down", y: 318 },
++                // FIXME: { tag: "Moving parent", button: pushMe, dir: "up", y: 142.8 },
++                // https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1427557
++            ]
++        }
++        function test_popover_follows_pointerTarget_bug1199502(data) {
++            mouseClick(data.button, data.button.width / 2, data.button.height / 2);
++            waitForRendering(data.button);
++            var dir = popoverSpy.target.direction
++            var popoverY = popoverSpy.target.y
++
++            // dismiss
++            mouseClick(main, 10, 10, Qt.LeftButton);
++            popoverSpy.wait();
++
++            // ensure popover was next to caller
++            compare(dir, data.dir, "Popover arrow is wrong")
++            compare(popoverY, data.y, "Popover isn't pointing at the caller")
++        }
+     }
+ }

Ubuntu UI Toolkit

Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/paperChase into lp:ubuntu-ui-toolkit/staging

Commit Message

Description of the Change

Preview Diff

Subscribers