Ubuntu UI Toolkit

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

listItemRightClick
Merge into staging

Proposed by Cris Dywan on 2015-06-17

Status:

Merged

Approved by:

Zsombor Egri on 2015-07-20

Approved revision:

1549

Merged at revision:

1570

Proposed branch:

lp://staging/~ubuntu-sdk-team/ubuntu-ui-toolkit/listItemRightClick

Merge into:

lp://staging/ubuntu-ui-toolkit/staging

Diff against target:

337 lines (+173/-8)

7 files modified

modules/Ubuntu/Components/1.3/ListItemPopover.qml (+70/-0)
modules/Ubuntu/Components/Popups/1.3/Popover.qml (+2/-2)
modules/Ubuntu/Components/plugin/uclistitem.cpp (+59/-1)
modules/Ubuntu/Components/plugin/uclistitem.h (+7/-0)
modules/Ubuntu/Components/plugin/uctheme.cpp (+4/-0)
modules/Ubuntu/Components/plugin/uctheme.h (+2/-1)
tests/unit_x11/tst_components/tst_listitem.qml (+29/-4)

To merge this branch:

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

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Zsombor Egri			Approve on 2015-07-20
PS Jenkins bot	continuous-integration		Approve on 2015-07-20
Tim Peeters		2015-06-17	Needs Fixing on 2015-07-01
Review via email: mp+262177@code.staging.launchpad.net

Commit message

Implement ListItemPopover on right-click

Revision history for this message

Andrea Bernabei (faenil) wrote on 2015-06-17:

I left a few comments, just out of curiosity

Revision history for this message

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

FAILED: Continuous integration, rev:1536
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1919/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3249/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/647/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/649/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/646/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3247/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) on 2015-06-18:

Revision history for this message

Andrea Bernabei (faenil) on 2015-06-18:

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/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1924/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-23:

When I open our gallery.sh and go to the new list items and right-click them, I am getting a bunch of warnings:

file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/examples/ubuntu-ui-toolkit-gallery/ListItems.qml:244:13: QML ListView: Binding loop detected for property "height"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"

review: Needs Fixing

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-23:

I get the same^ warnings with tst_listitem.qml:

tim@ubuntu:~/dev/ubuntu-ui-toolkit/r/listItemRightClick/tests/unit_x11/tst_components$ ../../launcher/launcher tst_listitem.qml
file:///usr/lib/x86_64-linux-gnu/qt5/qml/QtTest/TestCase.qml:337:32: Unable to assign [undefined] to bool
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-23:

I agree with Andrea that searching or the path of the qml file is not very pretty.

Also, even when using Ubuntu.Components 1.2, the ListItem will show the popover for 1.3. We don't want API or visual changes in 1.2 any more, but with this approach you will get the changes in ListItem popover that are made for 1.3 even when importing 1.2.

Perhaps we should show the popover only for 1.3?

review: Needs Information

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-23:

A screenshot of how it looks now: https://www.dropbox.com/s/v4cxv2vaqogqvjo/Screenshot%202015-06-23%2016.04.03.png?dl=0

Design input is needed.

Revision history for this message

Cris Dywan (kalikiana) wrote on 2015-06-23:

FTR I'm going to update the design.

The searching for the QML path can't be avoided as far as I'm aware because there's no path in the component, but I will look if I can find the version that is being used.

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-23:

> FTR I'm going to update the design.
>
> The searching for the QML path can't be avoided as far as I'm aware because
> there's no path in the component, but I will look if I can find the version
> that is being used.

If the version is not >= 1.3, the right-click should not even open the popover.

Revision history for this message

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

FAILED: Continuous integration, rev:1541
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1938/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/3302/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/666/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/668/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/665/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/3300/console

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

review: Needs Fixing (continuous-integration)

lp://staging/~ubuntu-sdk-team/ubuntu-ui-toolkit/listItemRightClick updated on 2015-06-24

1542. By Cris Dywan on 2015-06-24: Update components.api

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-29:

57 + MouseArea {
58 + anchors.fill: parent
59 + onReleased: {
60 + popover.hide();
61 + action.trigger();
62 + }
63 + }

This seems weird/hackish to me. Why do you need this?

review: Needs Information

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-29:

Test Result (3 failures / -1)
components.ListItemAPI::test_1_warn_missing_dragUpdated_signal_handler
components.ListItemAPI::test_dragmode_availability
components.ListItemAPI::test_warn_model

^the failures from jenkins seem related to the changes you made. Can you check them?

review: Needs Fixing

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-29:

I'm getting these failures locally too, see http://pastebin.ubuntu.com/11792405/

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-06-29:

Should we have autopilot CPOs specifically for using the popover to trigger an action?

Revision history for this message

Tim Peeters (tpeeters) wrote on 2015-07-01:

The list-item that was right-clicked to show the popover must be highlighted while the popover is open. I don't know if that was in the specifications, but we just agreed on it in the design sync meeting.

review: Needs Fixing

lp://staging/~ubuntu-sdk-team/ubuntu-ui-toolkit/listItemRightClick updated on 2015-07-01

1543. By Cris Dywan on 2015-07-01: Use onClicked instead of custom MouseArea in ListItem
1544. By Cris Dywan on 2015-07-01: Highlight ListItem while context menu is open

Revision history for this message

Cris Dywan (kalikiana) wrote on 2015-07-01:

> Should we have autopilot CPOs specifically for using the popover to trigger an
> action?

All icons are available via swiping. Currently I don't see any need since it would do exactly the same.

Revision history for this message

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

FAILED: Continuous integration, rev:1544
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/1956/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/686
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/686/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/683

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Zsombor Egri (zsombi) wrote on 2015-07-08:

What is the reason you registered a 1.3 specific ListItem? You wanted to guard the availability of the popup menu in 1.2? You can do that by not having the Popup present in 1.2 directory, and check where the ListItem is used based on the theme.toolkitVersion used.

You can look after the QML file you need based on the plugin's baseUrl(). See UbuntuComponentsPlugin::pluginUrl().

review: Needs Fixing

Revision history for this message

Zsombor Egri (zsombi) wrote on 2015-07-08:

Another idea on resolving the context menu: Introduce the menu property in the ListItem (which will be done anyway), yet undocumented. Then set that property in the 1.3 Ambiance style with a Popover from the theme. In the code you look after the menu property, if it is set, you can open it. So no need for 1.3 specific subclassing anymore.

lp://staging/~ubuntu-sdk-team/ubuntu-ui-toolkit/listItemRightClick updated on 2015-07-19

1545. By Cris Dywan on 2015-07-19: Use UbuntuComponentsPlugin::pluginUrl to locale ListItemPopover
1546. By Cris Dywan on 2015-07-19: Update line numbers in ignoreWarning calls in listitem test
1547. By Cris Dywan on 2015-07-19: Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/staging
1548. By Cris Dywan on 2015-07-19: Check popover.parent before adjusting y in updatePosition