[Merge] lp:~renatofilho/address-book-app/choose-default-address-book into lp:address-book-app

Gustavo Pichorim Boiko gustavo.boiko at canonical.com
Wed Dec 2 21:15:33 UTC 2015


Review: Needs Fixing

Just a couple remarks.

Diff comments:

> 
> === modified file 'src/imports/Ubuntu/AddressBook/ContactEditor/ContactDetailSyncTargetEditor.qml'
> --- src/imports/Ubuntu/AddressBook/ContactEditor/ContactDetailSyncTargetEditor.qml	2015-10-26 13:18:11 +0000
> +++ src/imports/Ubuntu/AddressBook/ContactEditor/ContactDetailSyncTargetEditor.qml	2015-12-01 16:47:06 +0000
> @@ -27,6 +27,9 @@
>      id: root
>  
>      property alias active: sourceModel.autoUpdate
> +    property bool isNewContact: contact && contact.contactId === "qtcontacts:::"
> +    property real myHeight: sources.currentlyExpanded ? sources.containerHeight + units.gu(6) + label.height : sources.itemHeight + units.gu(6) + label.height

maybe it is more readable to have it as:
property real myHeight: label.height + units.gu(6) + (sources.currentlyExpanded ? sources.containerHeight :
                                                                                  sources.itemHeight)

> +
>      signal changed()
>  
>      function save() {
> @@ -164,27 +207,17 @@
>  
>          delegate: OptionSelectorDelegate {
>              text: {
> -                if ((contact.guid.guid != "system-address-book") &&
> +                if ((sourceId != "system-address-book") &&
>                      (iconSource == "image://theme/address-book-app-symbolic")) {
> -                    return i18n.dtr("address-book-app", "Personal - %1").arg(contact.displayLabel.label)
> +                    return i18n.dtr("address-book-app", "Personal - %1").arg(sourceName)

Can you just add a TRANSLATORS comment explaining what the %1 is (just to make sure it gets translated correctly)?

>                  } else {
> -                    return contact.displayLabel.label
> +                    return sourceName
>                  }
>              }
>              constrainImage: true
> -            iconSource: {
> -                var details = contact.details(ContactDetail.ExtendedDetail)
> -                for(var i in details) {
> -                    if (details[i].name === "PROVIDER") {
> -                        if (details[i].data === "") {
> -                            return "image://theme/address-book-app-symbolic"
> -                        } else {
> -                            return "image://theme/online-accounts-%1".arg(details[i].data)
> -                        }
> -                    }
> -                }
> -                return "image://theme/address-book-app-symbolic"
> -            }
> +            iconSource: accountProvider == "" ?
> +                            "image://theme/address-book-app-symbolic" :
> +                            "image://theme/online-accounts-%1".arg(accountProvider)
>              height: units.gu(4)
>          }
>  


-- 
https://code.launchpad.net/~renatofilho/address-book-app/choose-default-address-book/+merge/279135
Your team Ubuntu Phablet Team is subscribed to branch lp:address-book-app.



More information about the Ubuntu-reviews mailing list