[Merge] lp:~phablet-team/messaging-app/refactor_fallback into lp:messaging-app/staging

Tiago Salem Herrmann tiago.herrmann at canonical.com
Wed Oct 19 10:08:26 UTC 2016


Review: Needs Fixing



Diff comments:

> 
> === modified file 'src/qml/GroupChatInfoPage.qml'
> --- src/qml/GroupChatInfoPage.qml	2016-10-13 18:15:58 +0000
> +++ src/qml/GroupChatInfoPage.qml	2016-10-13 18:15:59 +0000
> @@ -376,7 +376,7 @@
>                          onTriggered: {
>                              // in case account is of type Multimedia, alert if the group is going to have no active participants that the group could

please update this comment accordingly

>                              // be dissolved by the server
> -                            if (mainView.multimediaAccount !== null && chatEntry.participants.length === 1 /*the active participant to remove now*/) {
> +                            if (chatEntry.chatType == ChatEntry.ChatTypeRoom && chatEntry.participants.length === 1 /*the active participant to remove now*/) {
>                                  var properties = {}
>                                  properties["selectedIndex"] = value
>                                  properties["groupName"] = groupName.text
> 
> === modified file 'src/qml/Messages.qml'
> --- src/qml/Messages.qml	2016-10-13 18:15:58 +0000
> +++ src/qml/Messages.qml	2016-10-13 18:15:59 +0000
> @@ -107,51 +106,27 @@
>  
>      function restoreBindings() {
>          messages.account = Qt.binding(getCurrentAccount)
> -        messages.phoneAccount = Qt.binding(isPhoneAccount)
>          headerSections.selectedIndex = Qt.binding(getSelectedIndex)
>      }
>  
>      function getAccountsModel() {
> -        var accounts = []
> -        // on new chat dialogs display all possible accounts
> -        if (newMessage) {
> -            for (var i in telepathyHelper.activeAccounts) {
> -                accounts.push(telepathyHelper.activeAccounts[i])
> -            }
> -            return accounts
> -        }
> - 
> +        // on chat rooms we don't give the option to switch to another account
>          var tmpAccount = telepathyHelper.accountForId(messages.accountId)
> -        // on generic accounts we don't give the option to switch to another account
> -        if (tmpAccount && (tmpAccount.type == AccountEntry.GenericAccount ||
> -                           (tmpAccount.type == AccountEntry.MultimediaAccount && messages.chatType == HistoryThreadModel.ChatTypeRoom))) {
> +        if (!newMessage && tmpAccount && tmpAccount.type != AccountEntry.PhoneAccount && messages.chatType == HistoryThreadModel.ChatTypeRoom) {

please fix this logic.
broadcast messages on generic accounts will fail here.

>              return [tmpAccount]
>          }
>  
> -        // if we get here, this is a regular sms conversation. just
> -        // add the available phone accounts next
> -        for (var i in telepathyHelper.activeAccounts) {
> -            var account = telepathyHelper.activeAccounts[i]
> -            if (account.type == AccountEntry.PhoneAccount) {
> -                accounts.push(account)
> -            }
> -        }
> -
> -        return accounts
> +        // show only the text accounts meant to be displayed
> +        return telepathyHelper.textAccounts.displayed
>      }
>  
>      function getSectionsModel() {
>          var accountNames = []
> -        // suru divider must be empty if there is only one sim card
> -        if (messages.accountsModel.length == 1 &&
> -                messages.accountsModel[0].type == AccountEntry.PhoneAccount) {
> -            return []
> -        }
> - 
> +        // suru divider must be empty if there is only one account
>          for (var i in messages.accountsModel) {
>              accountNames.push(messages.accountsModel[i].displayName)
>          }
> -        return accountNames.length > 0 ? accountNames : []
> +        return accountNames.length > 1 ? accountNames : []
>      }
>  
>      function getSelectedIndex() {
> @@ -929,34 +887,64 @@
>      ActionSelectionPopover {
>          id: contextMenu
>          z: 100
> +
>          delegate: ListItem.Standard {
>              text: action.text
>          }
>          actions: ActionList {
> -            Action {
> -                text: i18n.tr("Create MMS Group...")
> -                onTriggered: {
> -                    if (!telepathyHelper.mmsGroupChat) {
> -                        var properties = {}
> -                        properties["title"] = i18n.tr("MMS group chat is disabled")
> -                        properties["text"] = i18n.tr("You need to enable MMS group chat in the app settings")
> -                        PopupUtils.open(Qt.createComponent("Dialogs/InformationDialog.qml").createObject(messages), messages, properties)
> -                        return
> -                    }
> -                    mainStack.addPageToCurrentColumn(messages, Qt.resolvedUrl("NewGroupPage.qml"), {"participants": multiRecipient.participants, "account": messages.account})
> +            id: actionList
> +        }
> +
> +        Action {
> +            id: mmsGroupAction
> +            text: i18n.tr("Create MMS Group...")
> +            onTriggered: {
> +                // FIXME: remove that, there is no need to have a MMS group chat option anymore
> +                if (!telepathyHelper.mmsGroupChat) {
> +                    var properties = {}
> +                    properties["title"] = i18n.tr("MMS group chat is disabled")
> +                    properties["text"] = i18n.tr("You need to enable MMS group chat in the app settings")
> +                    PopupUtils.open(Qt.createComponent("Dialogs/InformationDialog.qml").createObject(messages), messages, properties)
> +                    return
>                  }
> +                mainStack.addPageToCurrentColumn(messages, Qt.resolvedUrl("NewGroupPage.qml"), {"participants": multiRecipient.participants, "account": messages.account})
>              }
> +        }
> +
> +        Component {
> +            id: customGroupChatActionComponent
>              Action {
> +                property var participants: null
> +                property var account: null
>                  text: {
> -                    var protocolDisplayName = mainView.multimediaAccount.protocolInfo.serviceDisplayName;
> +                    var protocolDisplayName = account.protocolInfo.serviceDisplayName;
>                      if (protocolDisplayName === "") {
> -                       protocolDisplayName = mainView.multimediaAccount.protocolInfo.serviceName;
> +                       protocolDisplayName = account.protocolInfo.serviceName;
>                      }
>                      return i18n.tr("Create %1 Group...").arg(protocolDisplayName);
>                  }
> -                enabled: mainView.multimediaAccount != null
> -                onTriggered: mainStack.addPageToCurrentColumn(messages, Qt.resolvedUrl("NewGroupPage.qml"), {"multimedia": true, "participants": multiRecipient.participants, "account": messages.account})
> -                visible: mainView.multimediaAccount.connected
> +                // FIXME: this multimedia: true property needs to be replaced by the accountId

I think this fixme is obsolete

> +                onTriggered: mainStack.addPageToCurrentColumn(messages, Qt.resolvedUrl("NewGroupPage.qml"), {"mmsGroup": false, "participants": participants, "account": account})
> +            }
> +        }
> +
> +        function updateGroupTypes() {
> +            // remove the previous actions
> +            actionList.removeAction(mmsGroupAction)
> +            for (var i in actionList.actions) {
> +                actionList.actions[i].destroy()
> +            }
> +            actionList.actions = []
> +
> +            actionList.addAction(mmsGroupAction)
> +
> +            for (var i in telepathyHelper.textAccounts.active) {
> +                var account = telepathyHelper.textAccounts.active[i]
> +                if (account.type == AccountEntry.PhoneAccount) {
> +                    continue
> +                }
> +                var action = customGroupChatActionComponent.createObject(actionList, {"account": account, "participants": multiRecipient.participants})
> +                actionList.addAction(action)
>              }
>          }
>      }
> @@ -1099,10 +1084,13 @@
>              if (!account || chatType != HistoryThreadModel.ChatTypeContact) {
>                  return ""
>              }
> +            // FIXME: for accounts that we don't want to show the online status, we have to check if the overloaded account
> +            // might be available for that. If that account should not use the

this comment seems to be incomplete

>              if (account.type == AccountEntry.PhoneAccount) {
> -                for (var i in telepathyHelper.accounts) {
> -                    var tmpAccount = telepathyHelper.accounts[i]
> -                    if (tmpAccount.type == AccountEntry.MultimediaAccount) {
> +                var accounts = telepathyHelper.accountOverload(account)
> +                for (var i in accounts) {
> +                    var tmpAccount = accounts[i]
> +                    if (tmpAccount.active) {
>                          return tmpAccount.accountId
>                      }
>                  }


-- 
https://code.launchpad.net/~phablet-team/messaging-app/refactor_fallback/+merge/308425
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/messaging-app/add-participant-info-screen.



More information about the Ubuntu-reviews mailing list