ubuntu-touch-coreapps-reviewers team mailing list archive

[Victor Thompson <victor.thompson@xxxxxxxxx>]

Review: Needs Fixing

I have some inline comments.

Diff comments:

> 
> === added file 'app/components/Delegates/ActionDelegate.qml'
> --- app/components/Delegates/ActionDelegate.qml	1970-01-01 00:00:00 +0000
> +++ app/components/Delegates/ActionDelegate.qml	2015-10-28 01:05:47 +0000
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (C) 2015
> + *      Andrew Hayzen <ahayzen@xxxxxxxxx>
> + *      Victor Thompson <victor.thompson@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +import QtQuick 2.4
> +import Ubuntu.Components 1.2

This should be UC1.3

> +
> +Rectangle {
> +    color: currentColor
> +    width: height
> +
> +    Icon {
> +        anchors {
> +            centerIn: parent
> +        }
> +        objectName: action.objectName
> +        color: pressed ? UbuntuColors.blue : styleMusic.common.white
> +        name: action.iconName
> +        height: units.gu(3)
> +        width: units.gu(3)
> +    }
> +
> +    Rectangle {  // FIXME: pad.lv/1507339 Workaround for gap between end of listitem and first action
> +        anchors {
> +            bottom: parent.bottom
> +            right: parent.left
> +            top: parent.top
> +        }
> +        color: currentColor
> +        width: units.gu(0.5)
> +    }
> +}
> 
> === modified file 'app/components/Delegates/MusicListItem.qml'
> --- app/components/Delegates/MusicListItem.qml	2015-10-28 01:05:46 +0000
> +++ app/components/Delegates/MusicListItem.qml	2015-10-28 01:05:47 +0000
> @@ -21,131 +21,66 @@
>  import Ubuntu.Components 1.3
>  import "../"
>  
> +ListItem {
> +    color: styleMusic.mainView.backgroundColor
> +    highlightColor: Qt.lighter(color, 1.2)
>  
> -ListItemWithActions {
> -    id: root
> +    // Store the currentColor so that actions can bind to it
> +    property var currentColor: highlighted ? highlightColor : color
>  
>      property alias column: musicRow.column
>      property alias imageSource: musicRow.imageSource
>  
> -    property int listItemIndex: index
>      property bool multiselectable: false
> -    property int previousListItemIndex: -1
>      property bool reorderable: false
>  
> -    signal reorder(int from, int to)
> -
> -    onItemPressAndHold: {
> +    signal itemClicked()
> +
> +    onClicked: {
> +        if (selectMode) {
> +            selected = !selected;
> +        } else {
> +            itemClicked()
> +        }
> +    }
> +
> +    onPressAndHold: {
> +        // FIXME: pad.lv/1468100 drag a listitem with no leadingActions to right, then press and hold causes no signal

This bug has been marked fixed. Is it still an issue?

> +
> +        if (reorderable) {
> +            ListView.view.ViewItems.dragMode = !ListView.view.ViewItems.dragMode
> +        }
> +
>          if (multiselectable) {
> -            selectionMode = true
> -        }
> -    }
> -
> -    onListItemIndexChanged: {
> -        var i = parent.parent.selectedItems.lastIndexOf(previousListItemIndex)
> -
> -        if (i !== -1) {
> -            parent.parent.selectedItems[i] = listItemIndex
> -        }
> -
> -        previousListItemIndex = listItemIndex
> -    }
> -
> -    onSelectedChanged: {
> -        if (selectionMode) {
> -            var tmp = parent.parent.selectedItems
> -
> -            if (selected) {
> -                if (parent.parent.selectedItems.indexOf(listItemIndex) === -1) {
> -                    tmp.push(listItemIndex)
> -                    parent.parent.selectedItems = tmp
> -                }
> -            } else {
> -                tmp.splice(parent.parent.selectedItems.indexOf(listItemIndex), 1)
> -                parent.parent.selectedItems = tmp
> -            }
> -        }
> -    }
> -
> -    onSelectionModeChanged: {
> -        if (reorderable && selectionMode) {
> -            resetSwipe()
> -        }
> -
> -        for (var j=0; j < _main.children.length; j++) {
> -            if (_main.children[j] !== actionReorderLoader) {
> -                _main.children[j].anchors.rightMargin = reorderable && selectionMode ? actionReorderLoader.width + units.gu(2) : 0
> -            }
> -        }
> -
> -        parent.parent.state = selectionMode ? "multiselectable" : "normal"
> -
> -        if (!selectionMode) {
> -            selected = false
> -        }
> -    }
> -
> -    /* Highlight the listitem on press */
> -    Rectangle {
> -        id: listItemBrighten
> -        color: root.pressed ? styleMusic.common.white : "transparent"
> -        opacity: 0.1
> -        height: root.height
> -        x: root.x - parent.x  // -parent.x due to selectionIcon in ListItemWithActions
> -        width: root.width
> -    }
> -
> -    /* Reorder Component */
> -    Loader {
> -        id: actionReorderLoader
> -        active: reorderable && selectionMode && root.parent.parent.selectedItems.length === 0
> -        anchors {
> -            bottom: parent.bottom
> -            right: parent.right
> -            rightMargin: units.gu(1)
> -            top: parent.top
> -        }
> -        asynchronous: true
> -        source: "../ListItemReorderComponent.qml"
> -    }
> -
> -    Item {
> -        Connections {  // Only allow one ListItem to be swiping at any time
> -            target: mainView
> -            onListItemSwiping: {
> -                if (i !== index) {
> -                    root.resetSwipe();
> -                }
> -            }
> -        }
> -
> -        Connections {  // Connections from signals in the ListView
> -            target: root.parent.parent
> -            onClearSelection: selected = false
> -            onFlickingChanged: {
> -                if (root.parent.parent.flicking) {
> -                    root.resetSwipe()
> -                }
> -            }
> -            onSelectAll: selected = true
> -            onStateChanged: selectionMode = root.parent.parent.state === "multiselectable"
> -        }
> -    }
> -
> +            ListView.view.ViewItems.selectMode = !ListView.view.ViewItems.selectMode
> +        }
> +    }
> +
> +    divider {
> +        visible: false
> +    }
>  
>      MusicRow {
>          id: musicRow
>          anchors {
> -            verticalCenter: parent.verticalCenter
> -        }
> -        height: parent.height
> -    }
> -
> -    Component.onCompleted: {  // reload settings as delegates are destroyed
> -        if (parent.parent.selectedItems.indexOf(index) !== -1) {
> -            selected = true
> -        }
> -
> -        selectionMode = root.parent.parent.state === "multiselectable"
> +            fill: parent
> +            // When not in selectMode we want a margin between the Image and the left edge
> +            // when in selectMode the checkbox has its own margin so we don't want a double margin
> +            leftMargin: selectMode ? 0 : units.gu(2)
> +            rightMargin: selectMode ? 0 : units.gu(2)
> +        }
> +
> +        // Animate margin changes so it isn't noticible
> +        Behavior on anchors.leftMargin {
> +            NumberAnimation {
> +
> +            }
> +        }
> +
> +        Behavior on anchors.rightMargin {
> +            NumberAnimation {
> +
> +            }
> +        }
>      }
>  }
> 
> === added file 'app/components/ListItemActions/AddToQueueAndPlaylist.qml'
> --- app/components/ListItemActions/AddToQueueAndPlaylist.qml	1970-01-01 00:00:00 +0000
> +++ app/components/ListItemActions/AddToQueueAndPlaylist.qml	2015-10-28 01:05:47 +0000
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (C) 2015
> + *      Andrew Hayzen <ahayzen@xxxxxxxxx>
> + *      Victor Thompson <victor.thompson@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +import QtQuick 2.4
> +import Ubuntu.Components 1.2

Change to UC1.3

> +import "../Delegates"
> +
> +ListItemActions {
> +    actions: [
> +        AddToQueue {
> +
> +        },
> +        AddToPlaylist {
> +        }
> +    ]
> +    delegate: ActionDelegate {
> +
> +    }
> +}
> 
> === modified file 'debian/changelog'
> --- debian/changelog	2015-10-28 01:05:46 +0000
> +++ debian/changelog	2015-10-28 01:05:47 +0000
> @@ -7,6 +7,9 @@
>    * Remove some deprecated code for the UbuntuShape image property.
>    * Update app to use the Ubuntu Components version 1.3 (LP: #1508363)
>  
> +  [ Andrew Hayzen ]
> +  * Switch to using the new listitems within the SDK 

Perhaps mention UC1.3 as the version.

> +
>   -- Bartosz Kosiorek <gang65@xxxxxxxxxxxxxx>  Tue, 08 Sep 2015 10:08:49 +0200
>  
>  music-app (2.2ubuntu1) vivid; urgency=medium


-- 
https://code.launchpad.net/~ahayzen/music-app/refactor-use-sdk-listitems/+merge/274829
Your team Music App Developers is subscribed to branch lp:music-app.