D17214: GCI [KDE Connect] Start New Conversation in Messaging App

[Simon Redman]

sredman added a subscriber: apol.
sredman added a comment.


  This is good so far. Thank you!
  
  Two small bugs which come from the same source:
  
  If I type the phone number of a contact with whom I do not have a conversation, that gets matched to the contact (nice job!)
  
  Since the filter box has their phone number typed in, the contact is not visible, so you get the error: "TypeError: Cannot call method 'startChat' of null"
  
  There is also an error when you type the phone number for a contact with whom there is already a conversation F6445105: DoubleContact.png <https://phabricator.kde.org/F6445105>
  
  However, I think that is more of a bug that was already in the search box. The search box should allow filtering by address as well as by contact name. That way, these situations could never occur.

INLINE COMMENTS

> conversationsdbusinterface.cpp:125
>          // Since there are no messages in the conversation, we can't do anything sensible
> -        qCWarning(KDECONNECT_CONVERSATIONS) << "Got a conversationID for a conversation with no messages!";
> +        // qCDebug(KDECONNECT_CONVERSATIONS) << "Got a conversationID for a conversation with no messages!";
> +        qCDebug(KDECONNECT_CONVERSATIONS) << "Trying to send a new message to new conversation with address " << conversationID << endl;

No need to keep commented-out code

> conversationsdbusinterface.cpp:127
> +        qCDebug(KDECONNECT_CONVERSATIONS) << "Trying to send a new message to new conversation with address " << conversationID << endl;
> +        createNewConversation(conversationID, message);
>          return;

You pass the conversationID here, which you have hard-coded (in the QML application) to 0. That means that the phone tries to send a message to the phone number "0" which is probably not what was intended. Did you mean message.address()?
F6445122: MessageNullTarget.jpg <https://phabricator.kde.org/F6445122>

> conversationlistmodel.cpp:170-185
> +    QScopedPointer<KPeople::PersonData> personData(lookupPersonByAddress(address));
> +    QStandardItem* item = new QStandardItem();
> +    if (personData) {
> +        item->setText(personData->name());
> +        item->setIcon(QIcon(personData->photo()));
> +        item->setData(personData->personUri(), PersonUriRole);
> +    } else {

This is a good idea! However, since it is code that was copy-pasted from another method ("createRowFromMessage") could you make a helper function which can be used by both of those? For instance, maybe something like "createItemFromMessage" which returns QStandardItem*. That method gets used directly by "createRowFromMessage". This method ("createRowFromAddress") would construct the dummy message with the dummy "[New Conversation]" body and timestamp, then call the same helper

> conversationlistmodel.cpp:183
> +    item->setData(ConversationMessage::MessageTypeSent, FromMeRole);
> +    item->setData("[New conversation]", Qt::ToolTipRole);
> +    item->setData(1, DateRole);

@apol How do we make this translate-able?

> conversationlistmodel.cpp:184
> +    item->setData("[New conversation]", Qt::ToolTipRole);
> +    item->setData(1, DateRole);
> +    appendRow(item);

Do you need to specify this? What happens if you leave it out?

> ConversationList.qml:68
>                  deviceId: device ? device.id() : ""
> +                id: address
>              }

Is this used for anything?

> ConversationList.qml:85
> +                    address.createRowFromAddress(filter.text)
> +                    view.currentIndex = 1 // the first possible seleciton
> +                }

This should be set to 0 instead of 1

REPOSITORY
  R224 KDE Connect

REVISION DETAIL
  https://phabricator.kde.org/D17214

To: turx, albertvaka, nicolasfella, sredman, #kde_connect
Cc: apol, kdeconnect, varunp, shivanshukantprasad, skymoore, brute4s99, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, mikesomov, tctara
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20181128/b7efb5c4/attachment-0001.html>