GCI [KDE Connect] Start New Conversation in Messaging App
Needs ReviewPublic

Authored by turx on Wed, Nov 28, 1:41 PM.

Details

Reviewers
albertvaka
nicolasfella
sredman
Group Reviewers
KDE Connect
Summary
  • A contact with whom we are not already in a conversation
  • An arbitrary phone number

Diff Detail

Repository
R224 KDE Connect
Lint
Lint Skipped
Unit
Unit Tests Skipped
turx created this revision.Wed, Nov 28, 1:41 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptWed, Nov 28, 1:41 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
turx requested review of this revision.Wed, Nov 28, 1:41 PM
turx added reviewers: nicolasfella, Simon.
nicolasfella edited reviewers, added: sredman; removed: Simon.Wed, Nov 28, 1:44 PM
nicolasfella requested changes to this revision.Wed, Nov 28, 1:47 PM

There are some changes in this patch that don't belong here. You probably need to do git rebase master to fix it

plugins/systemvolume/systemvolumeplugin-pulse.cpp
77 ↗(On Diff #46401)

This slipped in by accident. Please remove it

This revision now requires changes to proceed.Wed, Nov 28, 1:47 PM

Let us know if you have any trouble with the rebase. If you did your work on the master branch, you should be able to just do git pull --rebase. If you did your work on a different branch, first change to master, then do git pull, then go back to your branch and do git rebase master

turx updated this revision to Diff 46420.Wed, Nov 28, 4:46 PM
turx marked an inline comment as done.

Removes something about PulseAudio which was misadded to the repo.

sredman added a subscriber: apol.Wed, Nov 28, 9:21 PM

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

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.

plugins/sms/conversationsdbusinterface.cpp
125

No need to keep commented-out code

127

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()?

smsapp/conversationlistmodel.cpp
170–185

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

183

@apol How do we make this translate-able?

184

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

smsapp/qml/ConversationList.qml
68

Is this used for anything?

85

This should be set to 0 instead of 1

There is also an error when you type the phone number for a contact with whom there is already a conversation

Something that might help when considering address vs thread_id, is to notice that if you receive an SMS on Android from any contact (ie. Mom/Dad) who share a phone number, Google/Android will just pick one and the messages will go there.

In other words, it may no even be possible to have two different thread_id's for the same address, so the thread_id is really only necessary for fetching messages. In the GSConnect UI, new incoming messages (whether for a new or existing conversation) are "routed" by address, not by thread_id, and I haven't found a situation where that breaks yet.

There is also an error when you type the phone number for a contact with whom there is already a conversation

Something that might help when considering address vs thread_id, is to notice that if you receive an SMS on Android from any contact (ie. Mom/Dad) who share a phone number, Google/Android will just pick one and the messages will go there.

In other words, it may no even be possible to have two different thread_id's for the same address, so the thread_id is really only necessary for fetching messages. In the GSConnect UI, new incoming messages (whether for a new or existing conversation) are "routed" by address, not by thread_id, and I haven't found a situation where that breaks yet.

Oh. Hm. All of our SMS app works on thread_id, with the assumption that they would be unique, or at least the desktop GUI would match the phone GUI. If I'm understanding what you're saying correctly, we're likely going to hit a wall at some point. In any case, fixing that is outside of the scope of this current patch.

nicolasfella added inline comments.Wed, Nov 28, 9:40 PM
smsapp/conversationlistmodel.cpp
183

i18n("Text")

All of our SMS app works on thread_id, with the assumption that they would be unique...

Oh, I believe they are unique, but they are *also* unique to phone number. So there will never be a address with more than one thread_id; any messages received from or sent to that address will always appear in the same thread_id.

So what I mean is, when starting a "new" conversation, you can watch for new messages from the target address and catch the thread_id (probably even from your "sent confirmation" packet).

Oh, I believe they are unique, but they are *also* unique to phone number. So there will never be a address with more than one thread_id; any messages received from or sent to that address will always appear in the same thread_id.

So what I mean is, when starting a "new" conversation, you can watch for new messages from the target address and catch the thread_id (probably even from your "sent confirmation" packet).

Ah. Now I understand. That is the behavior I was hoping for :)