ubuntu-touch-coreapps-reviewers team mailing list archive

[Roman Shchekin <mrqtros@xxxxxxxxx>]

Review: Needs Fixing

See inline comments. I will continue my review in IDE.

Diff comments:

> 
> === modified file 'src/app/qml/common/DetailsPage.qml'
> --- src/app/qml/common/DetailsPage.qml	2015-04-15 14:21:48 +0000
> +++ src/app/qml/common/DetailsPage.qml	2015-10-19 11:44:23 +0000
> @@ -14,45 +14,127 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -import QtQuick 2.3
> -import Ubuntu.Components 1.1
> -import Ubuntu.Components.ListItems 1.0 as ListItem
> +import QtQuick 2.4
> +import Ubuntu.Components 1.2
> +import Ubuntu.Components.ListItems 1.0 as ListItems
>  
>  import "utils.js" as Utils
>  
> +// FIXME: Autopilot tests
> +
>  Page {
>      id: detailsPage
>      objectName: "detailsPage"
> -
>      title: i18n.tr("Details")
>  
>      Column {
> -        width: parent.width
> -
> -        ListItem.Subtitled {
> -            text: i18n.tr("Location")
> -            subText: file.path
> -        }
> -        ListItem.Subtitled {
> -            text: i18n.tr("Size")
> -            subText: Utils.printSize(i18n, file.info.size)
> -        }
> -
> -        ListItem.Subtitled {
> -            text: i18n.tr("Created")
> -            subText: file.info.creationTime.toLocaleString(Qt.locale())
> -        }
> -
> -        ListItem.Subtitled {
> -            text: i18n.tr("Last modified")
> -            subText: file.info.lastModified.toLocaleString(Qt.locale())
> -        }
> -
> -        ListItem.Subtitled {
> -            id: mimetypeItem
> +        width: Math.min(units.gu(80), parent.width)
> +        anchors {
> +            top: parent.top
> +            bottom: parent.bottom
> +            horizontalCenter: parent.horizontalCenter
> +        }
> +

I think that you should introduce new QML class for this, something like "SubtitledListItem". Don't repeat yourself - great rule!

> +        ListItem {
> +            Column {
> +                anchors {
> +                    left: parent.left; right: parent.right
> +                    margins: units.gu(2)
> +                    verticalCenter: parent.verticalCenter
> +                }
> +
> +                Label {
> +                    text: i18n.tr("Location")
> +                    color: UbuntuColors.midAubergine
> +                }
> +                Label {
> +                    text: file.path
> +                    fontSize: "small"
> +                }
> +            }
> +        }
> +
> +        ListItem {
> +            Column {
> +                anchors {
> +                    left: parent.left; right: parent.right
> +                    margins: units.gu(2)
> +                    verticalCenter: parent.verticalCenter
> +                }
> +
> +                Label {
> +                    text: i18n.tr("Size")
> +                    color: UbuntuColors.midAubergine
> +                }
> +                Label {
> +                    text: Utils.printSize(i18n, file.info.size)
> +                    fontSize: "small"
> +                }
> +            }
> +        }
> +
> +        ListItem {
> +            Column {
> +                anchors {
> +                    left: parent.left; right: parent.right
> +                    margins: units.gu(2)
> +                    verticalCenter: parent.verticalCenter
> +                }
> +
> +                Label {
> +                    text: i18n.tr("Created")
> +                    color: UbuntuColors.midAubergine
> +                }
> +                Label {
> +                    text: file.info.creationTime.toLocaleString(Qt.locale())
> +                    fontSize: "small"
> +                }
> +            }
> +        }
> +
> +        ListItem {
> +            Column {
> +                anchors {
> +                    left: parent.left; right: parent.right
> +                    margins: units.gu(2)
> +                    verticalCenter: parent.verticalCenter
> +                }
> +
> +                Label {
> +                    text: i18n.tr("Last modified")
> +                    color: UbuntuColors.midAubergine
> +                }
> +                Label {
> +                    text: file.info.lastModified.toLocaleString(Qt.locale())
> +                    fontSize: "small"
> +                }
> +            }
> +        }
> +
> +        ListItem {
> +            // Used by Autopilot tests
>              objectName: "mimetypeItem"
> -            text: i18n.tr("MIME type")
> -            subText: file.mimetype.name
> +            property alias text: mimetypeText.text
> +            property alias subText: mimetypeSubText.text
> +
> +            Column {
> +                anchors {
> +                    left: parent.left; right: parent.right
> +                    margins: units.gu(2)
> +                    verticalCenter: parent.verticalCenter
> +                }
> +
> +                Label {
> +                    id: mimetypeText
> +                    text: i18n.tr("MIME type")
> +                    color: UbuntuColors.midAubergine
> +                }
> +                Label {
> +                    id: mimetypeSubText
> +                    text: file.mimetype.name
> +                    fontSize: "small"
> +                }
> +            }
>          }
>      }
>  }
> 
> === modified file 'src/app/qml/loView/PartsView.qml'
> --- src/app/qml/loView/PartsView.qml	2015-10-11 11:27:29 +0000
> +++ src/app/qml/loView/PartsView.qml	2015-10-19 11:44:23 +0000
> @@ -74,13 +76,13 @@
>                  wrapMode: Text.WordWrap
>                  text: model.name
>                  color: (loView.document.currentPart === model.index) ? UbuntuColors.orange
> -                                                                         : Theme.palette.selected.backgroundText

"Theme.palette.selected.backgroundText" such things are deprecated, aren't them?

> +                                                                     : Theme.palette.selected.backgroundText
>              }
>  
>              Label {
>                  text: model.index + 1
>                  color: (loView.document.currentPart === model.index) ? UbuntuColors.orange
> -                                                                         : Theme.palette.selected.backgroundText
> +                                                                     : Theme.palette.selected.backgroundText
>              }
>          }
>      }


-- 
https://code.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/reboot-uitk12/+merge/274051
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app/reboot.