ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #08597
Re: [Merge] lp:~ahayzen/music-app/convergence-tabs-with-sidebar-01 into lp:music-app
Review: Needs Fixing
Added some inline code issues/questions. Will try to do some functional testing as well tonight/tomorrow.
Diff comments:
> === modified file 'app/components/BlurredBackground.qml'
> --- app/components/BlurredBackground.qml 2016-01-16 01:30:31 +0000
> +++ app/components/BlurredBackground.qml 2016-03-04 04:41:28 +0000
> @@ -26,13 +26,14 @@
> width: parent.width
>
> property string art
> + property string color: "black"
Was there a specific reason to do this? Did you plan on referencing it elsewhere?
>
> // dark layer
> Rectangle {
> anchors {
> fill: parent
> }
> - color: "black"
> + color: parent.color
> }
>
> // the album art
>
> === modified file 'app/components/HeadState/MultiSelectHeadState.qml'
> --- app/components/HeadState/MultiSelectHeadState.qml 2016-01-16 01:30:31 +0000
> +++ app/components/HeadState/MultiSelectHeadState.qml 2016-03-04 04:41:28 +0000
> @@ -20,89 +20,102 @@
> import Ubuntu.Components 1.3
> import "../Flickables"
>
> -PageHeadState {
> - id: selectionState
> - actions: [
> - Action {
> - iconName: "select"
> - text: i18n.tr("Select All")
> - onTriggered: {
> - if (listview.getSelectedIndices().length === listview.model.count) {
> - listview.clearSelection()
> - } else {
> - listview.selectAll()
> - }
> - }
> - },
> - Action {
> - enabled: listview !== null ? listview.getSelectedIndices().length > 0 : false
> - iconName: "add-to-playlist"
> - text: i18n.tr("Add to playlist")
> - onTriggered: {
> - var items = []
> - var indicies = listview.getSelectedIndices();
> -
> - for (var i=0; i < indicies.length; i++) {
> - items.push(makeDict(listview.model.get(indicies[i], listview.model.RoleModelData)));
> - }
> -
> - mainPageStack.push(Qt.resolvedUrl("../../ui/AddToPlaylist.qml"),
> - {"chosenElements": items})
> -
> - listview.closeSelection()
> - }
> - },
> - Action {
> - enabled: listview !== null ? listview.getSelectedIndices().length > 0 : false
> - iconName: "add"
> - text: i18n.tr("Add to queue")
> - visible: addToQueue
> -
> - onTriggered: {
> - var items = [];
> - var indicies = listview.getSelectedIndices();
> -
> - for (var i=0; i < indicies.length; i++) {
> - items.push(Qt.resolvedUrl(listview.model.get(indicies[i], listview.model.RoleModelData).filename));
> - }
> -
> - player.mediaPlayer.playlist.addItems(items);
> -
> - listview.closeSelection()
> - }
> - },
> - Action {
> - enabled: listview !== null ? listview.getSelectedIndices().length > 0 : false
> - iconName: "delete"
> - text: i18n.tr("Delete")
> - visible: removable
> -
> - onTriggered: {
> - removed(listview.getSelectedIndices())
> -
> - listview.closeSelection()
> - }
> - }
> -
> - ]
> - backAction: Action {
> - text: i18n.tr("Cancel selection")
> - iconName: "back"
> - onTriggered: listview.closeSelection()
> - }
> - head: thisPage.head
> +State {
> name: "selection"
>
> - PropertyChanges {
> - target: thisPage.head
> - backAction: selectionState.backAction
> - actions: selectionState.actions
> - }
> -
> property bool addToQueue: true
> property MultiSelectListView listview
> property bool removable: false
> - property Page thisPage
> + property PageHeader thisHeader: PageHeader {
> + id: selectionState
> + flickable: thisPage.flickable
> + leadingActionBar {
> + actions: [
> + Action {
> + text: i18n.tr("Cancel selection")
> + iconName: "back"
> + onTriggered: listview.closeSelection()
> + }
> + ]
> + }
> + title: thisPage.title
> + trailingActionBar {
> + actions: [
> + Action {
> + iconName: "select"
> + text: i18n.tr("Select All")
> + onTriggered: {
> + if (listview.getSelectedIndices().length === listview.model.count) {
> + listview.clearSelection()
> + } else {
> + listview.selectAll()
> + }
> + }
> + },
> + Action {
> + enabled: listview !== null ? listview.getSelectedIndices().length > 0 : false
> + iconName: "add-to-playlist"
> + text: i18n.tr("Add to playlist")
> + onTriggered: {
> + var items = []
> + var indicies = listview.getSelectedIndices();
> +
> + for (var i=0; i < indicies.length; i++) {
> + items.push(makeDict(listview.model.get(indicies[i], listview.model.RoleModelData)));
> + }
> +
> + mainPageStack.push(Qt.resolvedUrl("../../ui/AddToPlaylist.qml"),
> + {"chosenElements": items})
> +
> + listview.closeSelection()
> + }
> + },
> + Action {
> + enabled: listview !== null ? listview.getSelectedIndices().length > 0 : false
> + iconName: "add"
> + text: i18n.tr("Add to queue")
> + visible: addToQueue
> +
> + onTriggered: {
> + var items = [];
> + var indicies = listview.getSelectedIndices();
> +
> + for (var i=0; i < indicies.length; i++) {
> + items.push(Qt.resolvedUrl(listview.model.get(indicies[i], listview.model.RoleModelData).filename));
> + }
> +
> + player.mediaPlayer.playlist.addItems(items);
> +
> + listview.closeSelection()
> + }
> + },
> + Action {
> + enabled: listview !== null ? listview.getSelectedIndices().length > 0 : false
> + iconName: "delete"
> + text: i18n.tr("Delete")
> + visible: removable
> +
> + onTriggered: {
> + removed(listview.getSelectedIndices())
> +
> + listview.closeSelection()
> + }
> + }
> + ]
> + }
> + visible: thisPage.state === "selection"
> +
> + StyleHints {
> + backgroundColor: mainView.headerColor
> + dividerColor: Qt.darker(mainView.headerColor, 1.1)
It'd be tiny, but since we replicate this all over the place it would be nice to make a MusicStyleHints or perhaps put the "1.1" value in the Style.qml file. Actually, maybe create a base MusicState for all these to implement?
> + }
> + }
> + property Item thisPage
>
> signal removed(var selectedIndices)
> +
> + PropertyChanges {
> + target: thisPage
> + header: thisHeader
> + }
> }
>
> === modified file 'app/components/HeadState/SearchHeadState.qml'
> --- app/components/HeadState/SearchHeadState.qml 2015-11-17 01:59:32 +0000
> +++ app/components/HeadState/SearchHeadState.qml 2016-03-04 04:41:28 +0000
> @@ -19,61 +19,79 @@
> import QtQuick 2.4
> import Ubuntu.Components 1.3
>
> -PageHeadState {
> - id: headerState
> +State {
> name: "search"
> - head: thisPage.head
> - backAction: Action {
> - id: leaveSearchAction
> - text: "back"
> - iconName: "back"
> - onTriggered: thisPage.state = "default"
> - }
> - contents: TextField {
> - id: searchField
> - anchors {
> - left: parent ? parent.left : undefined
> - right: parent ? parent.right : undefined
> - rightMargin: units.gu(2)
> - }
> - color: styleMusic.common.black
> - hasClearButton: true
> - inputMethodHints: Qt.ImhNoPredictiveText
> - placeholderText: i18n.tr("Search music")
> +
> + property PageHeader thisHeader: PageHeader {
> + id: headerState
> + contents: TextField {
> + id: searchField
> + anchors {
> + left: parent ? parent.left : undefined
> + right: parent ? parent.right : undefined
> + verticalCenter: parent ? parent.verticalCenter : undefined
> + }
> + color: styleMusic.common.black
> + focus: true
> + hasClearButton: true
> + inputMethodHints: Qt.ImhNoPredictiveText
> + placeholderText: i18n.tr("Search music")
> +
> + // Use the page onVisible as the text field goes visible=false when switching states
> + // This is used when popping from the pageStack and returning back to a page with search
> + Connections {
> + target: thisPage
> +
> + onStateChanged: { // ensure the search is reset (eg pressing Esc)
> + if (state === "default") {
> + searchField.text = ""
> + }
> +
> + // FIXME: Workaround for pad.lv/1514143 (keyboard show/hide on view moving)
This bug was fixed in OTA9, you can leave the workaround in if you want, but otherwise we should put together an MP to remove it.
> + // by locking the header and forcing a topMargin of page to the header height
> + thisPage.head.locked = state === headerState.name;
Move this commented out code? Or will it be needed when bug is resolved?
> + //thisPage.anchors.topMargin = state === headerState.name ? units.gu(6.125) : 0 // FIXME: 6.125 is header.height
> + }
> +
> + onVisibleChanged: {
> + // clear when the page becomes visible not invisible
> + // if invisible is used the delegates can be destroyed which
> + // have created the pushed component
> + if (visible) {
> + thisPage.state = "default"
> + }
> + }
> + }
> + }
> + flickable: thisPage.flickable
> + leadingActionBar {
> + actions: [
> + Action {
> + id: leaveSearchAction
> + text: "back"
> + iconName: "back"
> + onTriggered: thisPage.state = "default"
> + }
> + ]
> + }
> + visible: thisPage.state === "search"
>
> onVisibleChanged: {
> if (visible) {
> - forceActiveFocus()
> + searchField.forceActiveFocus()
> }
> }
>
> - // Use the page onVisible as the text field goes visible=false when switching states
> - // This is used when popping from the pageStack and returning back to a page with search
> - Connections {
> - target: thisPage
> -
> - onStateChanged: { // ensure the search is reset (eg pressing Esc)
> - if (state === "default") {
> - searchField.text = ""
> - }
> -
> - // FIXME: Workaround for pad.lv/1514143 (keyboard show/hide on view moving)
> - // by locking the header and forcing a topMargin of page to the header height
> - headerState.head.locked = state === headerState.name;
> - thisPage.anchors.topMargin = state === headerState.name ? units.gu(6.125) : 0 // FIXME: 6.125 is header.height
> - }
> -
> - onVisibleChanged: {
> - // clear when the page becomes visible not invisible
> - // if invisible is used the delegates can be destroyed which
> - // have created the pushed component
> - if (visible) {
> - thisPage.state = "default"
> - }
> - }
> + StyleHints {
> + backgroundColor: mainView.headerColor
> + dividerColor: Qt.darker(mainView.headerColor, 1.1)
> }
> }
> -
> - property Page thisPage
> + property Item thisPage
> property alias query: searchField.text
> +
> + PropertyChanges {
> + target: thisPage
> + header: thisHeader
> + }
> }
>
> === added file 'app/components/NowPlayingSidebar.qml'
> --- app/components/NowPlayingSidebar.qml 1970-01-01 00:00:00 +0000
> +++ app/components/NowPlayingSidebar.qml 2016-03-04 04:41:28 +0000
> @@ -0,0 +1,134 @@
> +/*
> + * Copyright (C) 2013, 2014, 2015, 2016
This is a nitpick, but when we have consecutive updates, let's do 2013-2016, etc. You can leave it as-is for this MP though if you'd like.
> + * Andrew Hayzen <ahayzen@xxxxxxxxx>
> + * Daniel Holm <d.holmen@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.3
> +
> +import "HeadState"
> +
> +Rectangle {
> + id: nowPlayingSidebar
> + anchors {
> + fill: parent
> + }
> + color: "#2c2c34"
I know we haven't been consistent, but we should avoid hardcoding this value all the time. Consider moving it to Style.qml
> + state: queue.state === "multiselectable" ? "selection" : "default"
> + states: [
> + QueueHeadState {
> + thisHeader {
> + leadingActionBar {
> + actions: [] // hide tab bar
> + }
> + z: 100 // put on top of content
> + }
> + thisPage: nowPlayingSidebar
> + },
> + MultiSelectHeadState {
> + addToQueue: false
> + listview: queue
> + removable: true
> + thisHeader {
> + z: 100 // put on top of content
> + }
> + thisPage: nowPlayingSidebar
> +
> + onRemoved: {
> + // Remove the tracks from the queue
> + // Use slice() to copy the list
> + // so that the indexes don't change as they are removed
> + player.mediaPlayer.playlist.removeItemsWrapper(selectedIndices.slice());
> + }
> + }
> + ]
> + property alias flickable: queue // fake normal Page
> + property Item header: PageHeader {
> + id: pageHeader
> + leadingActionBar {
> + actions: nowPlayingSidebar.head.backAction
> + }
> + flickable: queue
> + trailingActionBar {
> + actions: nowPlayingSidebar.head.actions
> + }
> + z: 100 // put on top of content
> +
> + StyleHints {
> + backgroundColor: mainView.headerColor
> + }
> + }
> + property Item previousHeader: null
> + property string title: "" // fake normal Page
> +
> + onHeaderChanged: { // Copy what SDK does to parent header correctly
> + if (previousHeader) {
> + previousHeader.parent = null
> + }
> +
> + header.parent = nowPlayingSidebar
> + previousHeader = header;
> + }
> +
> + Loader {
> + anchors {
> + left: parent.left
> + right: parent.right
> + top: parent.top
> + }
> + height: units.gu(6.125)
> + sourceComponent: header
> + }
> +
> + Queue {
> + id: queue
> + anchors {
> + bottomMargin: 0
> + topMargin: 0
> + }
> + clip: true
> + isSidebar: true
> + header: Column {
> + id: sidebarColumn
> + anchors {
> + left: parent.left
> + right: parent.right
> + }
> +
> + NowPlayingFullView {
> + anchors {
> + fill: undefined
> + }
> + backgroundColor: "#2c2c34"
> + clip: true
> + height: units.gu(47)
> + sidebar: true
> + width: parent.width
> + }
> +
> + NowPlayingToolbar {
> + anchors {
> + fill: undefined
> + }
> + bottomProgressHint: false
> + color: "#2c2c34"
> + height: itemSize + 2 * spacing + units.gu(2)
> + width: parent.width
> + }
> + }
> + }
> +}
>
> === modified file 'app/components/NowPlayingToolbar.qml'
> --- app/components/NowPlayingToolbar.qml 2016-01-16 01:30:31 +0000
> +++ app/components/NowPlayingToolbar.qml 2016-03-04 04:41:28 +0000
> @@ -81,13 +85,13 @@
> MouseArea {
> id: nowPlayingPlayButton
> anchors.centerIn: parent
> - height: units.gu(10)
> + height: itemSize + 2 * spacing
Please add parens to clarify the order of operations.
> width: height
> onClicked: player.mediaPlayer.toggle()
>
> Icon {
> id: nowPlayingPlayIndicator
> - height: units.gu(6)
> + height: itemSize
> width: height
> anchors.verticalCenter: parent.verticalCenter
> anchors.horizontalCenter: parent.horizontalCenter
>
> === modified file 'app/music-app.qml'
> --- app/music-app.qml 2016-01-28 01:43:54 +0000
> +++ app/music-app.qml 2016-03-04 04:41:28 +0000
> @@ -644,6 +637,39 @@
> }
>
> property Tab lastTab: selectedTab
> + property list<Action> tabActions: [
Does this deserve it's own component at all? I'd kind of argue that it's not necessary, but having a MusicTabActions component might be "nice" since this logic is rather simple.
> + Action {
> + enabled: recentTabRepeater.count > 0
> + text: enabled ? recentTabRepeater.itemAt(0).title : ""
> + visible: enabled
> +
> + onTriggered: {
> + if (enabled) {
> + tabs.selectedTabIndex = recentTabRepeater.itemAt(0).index
> + }
> + }
> + },
> + Action {
> + text: artistsTab.title
> + onTriggered: tabs.selectedTabIndex = artistsTab.index
> + },
> + Action {
> + text: albumsTab.title
> + onTriggered: tabs.selectedTabIndex = albumsTab.index
> + },
> + Action {
> + text: genresTab.title
> + onTriggered: tabs.selectedTabIndex = genresTab.index
> + },
> + Action {
> + text: songsTab.title
> + onTriggered: tabs.selectedTabIndex = songsTab.index
> + },
> + Action {
> + text: playlistsTab.title
> + onTriggered: tabs.selectedTabIndex = playlistsTab.index
> + }
> + ]
>
> onSelectedTabChanged: {
> // pause loading of the models in the old tab
>
> === modified file 'app/ui/AddToPlaylist.qml'
> --- app/ui/AddToPlaylist.qml 2016-02-28 01:51:17 +0000
> +++ app/ui/AddToPlaylist.qml 2016-03-04 04:41:28 +0000
> @@ -59,10 +59,13 @@
>
> // FIXME: workaround for pad.lv/1531016 (gridview juddery)
> anchors {
> + bottom: parent.bottom
> + left: parent.left
Move below fill? Occurs a few times in various files.
> fill: undefined
> + top: parent.top
> }
> - height: mainView.height + musicToolbar.height
> - width: mainView.width
> + height: mainPageStack.height + musicToolbar.height
> + width: mainPageStack.width
>
> property var chosenElements: []
>
--
https://code.launchpad.net/~ahayzen/music-app/convergence-tabs-with-sidebar-01/+merge/286127
Your team Music App Developers is subscribed to branch lp:music-app.
References
