ubuntu-touch-coreapps-reviewers team mailing list archive

[Roman Shchekin <mrqtros@xxxxxxxxx>]

Review: Needs Fixing

See inline comments.

Overall feeling - so much logic is located in headers. I know that you only rewriting some staff to UITK 1.3, just wanted to note that. 
Can't w8 that time when I will be able to refactor some QML code ;)

Diff comments:

> 
> === modified file 'src/app/qml/documentPage/DocumentPageSearchHeader.qml'
> --- src/app/qml/documentPage/DocumentPageSearchHeader.qml	2015-10-19 13:00:11 +0000
> +++ src/app/qml/documentPage/DocumentPageSearchHeader.qml	2016-03-26 18:49:31 +0000
> @@ -54,4 +61,33 @@
>          // show OSK if appropriate.
>          onVisibleChanged: forceActiveFocus()
>      }
> +

Should you extract this component to separate file since it is declared twice?

> +    Component {
> +        id: textualButton
> +        AbstractButton {
> +            id: button
> +            action: modelData
> +            width: label.width + units.gu(4)
> +            height: parent.height
> +            Rectangle {
> +                color: UbuntuColors.slate
> +                opacity: 0.1
> +                anchors.fill: parent
> +                visible: button.pressed
> +            }
> +            Label {
> +                anchors.centerIn: parent
> +                id: label
> +                text: action.text
> +                font.weight: text === i18n.tr("Pick") ? Font.Normal : Font.Light
> +                color: {
> +                    if (button.enabled)
> +                        return text === i18n.tr("Pick") ? theme.palette.selected.backgroundText : theme.palette.normal.backgroundText
> +
> +                    return theme.palette.disabled.backgroundText
> +                }
> +            }
> +        }
> +    }
> +
>  }
> 
> === modified file 'src/app/qml/documentPage/DocumentPageSelectionModeHeader.qml'
> --- src/app/qml/documentPage/DocumentPageSelectionModeHeader.qml	2015-11-01 16:50:23 +0000
> +++ src/app/qml/documentPage/DocumentPageSelectionModeHeader.qml	2016-03-26 18:49:31 +0000
> @@ -18,55 +18,77 @@
>  import Ubuntu.Components 1.3
>  import Ubuntu.Components.Popups 1.3
>  
> -PageHeadState {
> -    id: rootItem
> -
> -    property Page targetPage
> -    head: targetPage.head
> -
> -    property bool selectedAll: viewLoader.item.ViewItems.selectedIndices.length == viewLoader.item.count
> -
> -    backAction: Action {
> +PageHeader {
> +    id: selectionHeader
> +
> +    property var view: parent.view
> +    property Page parentPage: parent
> +
> +    property bool selectedAll: view.ViewItems.selectedIndices.length == view.count
> +
> +    leadingActionBar.actions: Action {
>          iconName: "close"
>          text: i18n.tr("Close")
>          onTriggered: {
> -            viewLoader.item.cancelSelection()
> -        }
> -    }
> -
> -    actions: [
> -        Action {
> -            iconName: selectedAll ? "select-none" :  "select"
> -            text: selectedAll ? i18n.tr("Select None") : i18n.tr("Select All")
> -
> -            onTriggered: {
> -                if (selectedAll) {
> -                    viewLoader.item.clearSelection()
> -                } else {
> -                    viewLoader.item.selectAll()
> -                }
> -            }
> -        },
> -
> -        Action {
> -            iconName: "delete"
> -            text: i18n.tr("Delete")
> -            enabled: viewLoader.item.ViewItems.selectedIndices.length !== 0
> -
> -            onTriggered: PopupUtils.open(Qt.resolvedUrl("DeleteFileDialog.qml"), documentPage)
> -        }
> -    ]
> -
> -    // WORKAROUND: "preset" property of PageHeadConfiguration is still not
> -    // exposed in PageHeadState.
> -    contents: Item {
> -        Connections {
> -            target: targetPage
> -            onStateChanged: {
> -                if (targetPage.state === "selection")
> -                    head.preset = "select"
> -                else
> -                    head.preset = ""
> +            view.cancelSelection()
> +        }
> +    }
> +
> +    trailingActionBar {
> +        anchors.rightMargin: 0
> +        delegate: textualButtonWithIcon
> +
> +        actions: [
> +            Action {
> +                iconName: selectedAll ? "select-none" :  "select"
> +                text: selectedAll ? i18n.tr("Select None") : i18n.tr("Select All")
> +
> +                onTriggered: {
> +                    if (selectedAll)
> +                        view.clearSelection()
> +                    else
> +                        view.selectAll()
> +                }
> +            },
> +
> +            Action {
> +                iconName: "delete"
> +                text: i18n.tr("Delete")
> +                enabled: view.ViewItems.selectedIndices.length !== 0
> +
> +                onTriggered: PopupUtils.open(Qt.resolvedUrl("DeleteFileDialog.qml"), documentPage)
> +            }
> +        ]
> +    }
> +

Third similar component!

> +    Component {
> +        id: textualButtonWithIcon
> +        AbstractButton {
> +            id: button
> +            action: modelData
> +            width: layout.width + units.gu(4)
> +            height: parent.height
> +            Rectangle {
> +                color: UbuntuColors.slate
> +                opacity: 0.1
> +                anchors.fill: parent
> +                visible: button.pressed
> +            }
> +            Row {
> +                id: layout
> +                anchors.centerIn: parent
> +                spacing: units.gu(1)
> +                Icon {
> +                    anchors.verticalCenter: parent.verticalCenter
> +                    width: units.gu(2); height: width
> +                    name: action.iconName
> +                    source: action.iconSource
> +                }
> +                Label {
> +                    anchors.verticalCenter: parent.verticalCenter
> +                    text: action.text
> +                    font.weight: text === i18n.tr("Pick") ? Font.Normal : Font.Light
> +                }
>              }
>          }
>      }


-- 
https://code.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/uitk13-documents-page/+merge/290170
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app.