Merge lp://qastaging/~tpeeters/ubuntu-ui-toolkit/freeze into lp://qastaging/ubuntu-ui-toolkit/staging

Proposed by Tim Peeters
Status: Merged
Approved by: Cris Dywan
Approved revision: 1958
Merged at revision: 1945
Proposed branch: lp://qastaging/~tpeeters/ubuntu-ui-toolkit/freeze
Merge into: lp://qastaging/ubuntu-ui-toolkit/staging
Diff against target: 250 lines (+208/-4)
3 files modified
src/Ubuntu/Components/Popups/1.3/PopupBase.qml (+3/-3)
src/Ubuntu/Components/Popups/1.3/popupUtils.js (+6/-1)
tests/unit_x11/tst_components/tst_popups_pagestack.qml (+199/-0)
To merge this branch: bzr merge lp://qastaging/~tpeeters/ubuntu-ui-toolkit/freeze
Reviewer Review Type Date Requested Status
ubuntu-sdk-build-bot continuous-integration Approve
Cris Dywan Approve
Review via email: mp+291814@code.qastaging.launchpad.net

Commit message

Fix app freeze after closing Dialog/Popover.

Description of the change

Fix app freeze after closing Dialog/Popover.

To post a comment you must log in.
1945. By Tim Peeters

clean

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1944
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/684/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2618/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1944
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/465/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2619/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1944
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/651/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2620/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1944
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/439/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2621/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1945
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-stable/685/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2623/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1945
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-amd64-devel/466/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2624/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1945
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-armhf-stable/652/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2625/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :

FAILED: Continuous integration, rev:1945
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/freeze/+merge/291814/+edit-commit-message

https://jenkins.ubuntu.com/ubuntu-sdk/job/ubuntu-ui-toolkit-ci-i386-gles-stable/440/
Executed test runs:
    None: https://jenkins.ubuntu.com/ubuntu-sdk/job/generic-update-mp/2626/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1946. By Tim Peeters

improve test

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1947. By Tim Peeters

void

1948. By Tim Peeters

wait for clicked signal

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1949. By Tim Peeters

click button in the middle

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1950. By Tim Peeters

try CI again

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
1951. By Tim Peeters

kick CI again to make sure we didn't just get one lucky pass for a flaky test

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1952. By Tim Peeters

wait more

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
1953. By Tim Peeters

try CI again to be sure

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
1954. By Tim Peeters

one more time ;)

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
1955. By Tim Peeters

comment

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

I think the unit test should also cover a popover explicitly - it's shared code but the differences between the components are extensive.

review: Needs Fixing
1956. By Tim Peeters

add popover unit test

1957. By Tim Peeters

sync staging

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

> property alias popoverComponent: popoverComponent
> [...]
> if (data.tag === "Dialog component") {
> popup = PopupUtils.open(pageStack.currentPage.dialogComponent);
The property should have a name that is the same for both tests, for instance "property alias popupComponent" could be popoverComponent or dialogComponent respectively, instead of if-deffing based on the tag. And note how I'm explicitly suggesting to have unique names to make it less confusing and error-prone.

review: Needs Fixing
Revision history for this message
Tim Peeters (tpeeters) wrote :

An alias only points to one object, you cannot just set it to another.

The only thing I can do is create an additional property Component popupComponent, and then in the test do "if-deffing" to set popupComponent = (for example)dialogComponent. But with that we don't win anything, and it will only make the test code longer. So, what do you propose?

Revision history for this message
Tim Peeters (tpeeters) wrote :

The CI failure above looks like a flaky tst_slotslayout which causes a segfault. I reported a bug for that here https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1571426

Revision history for this message
Cris Dywan (kalikiana) wrote :

> An alias only points to one object, you cannot just set it to another.
>
> The only thing I can do is create an additional property Component
> popupComponent, and then in the test do "if-deffing" to set popupComponent =
> (for example)dialogComponent. But with that we don't win anything, and it will
> only make the test code longer. So, what do you propose?

My bad, I didn't explain it properly. You have to put either the buttons or the components into the data function.

Using the buttons you could do it like test_popover_follows_pointerTarget_bug1199502 in tst_popover(13).qml which only clicks the button from the data function which in turn opens the popover (so manual testing behavior is identical to the unit test).
Or you can simply have popupComponent in the data function and use that. No need for an if.

1958. By Tim Peeters

click buttons in test

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

Lovely, thanks for getting rid of the if!

review: Approve
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches