(WIP) Upgrade Telephony plugin to read SMS history (KDE side)
ClosedPublic

Authored by sredman on Apr 1 2018, 1:11 AM.

Details

Summary

For real usecases of SMS support, we will almost always need access to the message history in some way

Specifically resolve T8338

Incidentally resolve T6651 since Telephony shall no longer create a notification

Test Plan

Setup:

  • Build corresponding Android-side diff (D11698)
  • Build this diff

Step 1: Does anything at all work?

  • Put a breakpoint in the handleBatchMessages method of the telephony plugin, ideally after constructing a Message object
  • Use DBus to poke /modules/kdeconnect/devices/<deviceID>/telephony.requestAllConversations()
  • Verify that the constructed Message is one you sent or received and that it is the most recent in the corresponding conversation

Step 2: DBus

  • Open the Interface org.kde.kdeconnect.device.conversations of /modules/kdeconnect/devices/<deviceId>
  • Poke activeConversations and verify an empty array is returned
  • Poke requestAllConversationThreads
  • Poke activeConversations and verify that a list of numbers has been returned. These are conversationIds
  • Use a conversationId to call getFirstFromConversation
    • Verify that the returned Message object is one which you recognize
    • Note that if the message is an MMS it will be blank and meaningless. Try a different conversationId. MMS support "coming soon!"

Step 3: SMS App

  • Use ccmake (or similar) to set SMSAPP_ENABLE to ON
  • Build the project
  • Run ./bin/kdeconnect-sms
  • Verify that the app shows a list of everyone you have an SMS conversation with (MMS messages are stripped out)
    • If you have the Contacts plugin working, verify that most contacts have their name and photo instead of their phone number

Diff Detail

Repository
R224 KDE Connect
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sredman created this revision.Apr 1 2018, 1:11 AM
Restricted Application added a project: KDE Connect. · View Herald TranscriptApr 1 2018, 1:11 AM
sredman requested review of this revision.Apr 1 2018, 1:11 AM
sredman edited reviewers, added: KDE Connect; removed: kdeconnect.Apr 1 2018, 1:14 AM
sredman edited subscribers, added: KDE Connect; removed: kdeconnect.
sredman updated this revision to Diff 31082.Apr 1 2018, 4:12 AM

Add basic dbus interface for viewing messages

nicolasfella added inline comments.
plugins/telephony/telephonyMessage.h
27 ↗(On Diff #31082)

Uppercase for all class names, please

nicolasfella added inline comments.Apr 1 2018, 12:58 PM
plugins/telephony/telephonyMessage.h
27 ↗(On Diff #31082)

In general you should favor composition over inheritance, you should read about that a bit.
Meaning: TelephonyMessage should have a QMap to store the information, not be one. The public interface to TelephonyMessage should be independent of the underlying data structure, because our needs could change.

That said, I don't quite get it's purpose. Does it store a single message like the name suggests? In that case a QMap doesn't make any sense to me. A class with QStrings address and body should be enough. If it is intended to store multiple messages the naming should reflect that. Also a QList or QMap in the TelephonyPlugin should be enough

nicolasfella requested changes to this revision.Apr 1 2018, 1:00 PM
This revision now requires changes to proceed.Apr 1 2018, 1:00 PM
sredman updated this revision to Diff 31106.Apr 1 2018, 5:01 PM

Rename class, restructure to container, and add interface

  • Rename telephonyMessage to Message
  • Change Message from extending QMap to instead be a container class
  • Add interface for messages
sredman retitled this revision from Upgrade Telephony plugin to read SMS history (KDE side) to (WIP) Upgrade Telephony plugin to read SMS history (KDE side).Apr 1 2018, 5:02 PM
sredman marked 2 inline comments as done.Apr 1 2018, 5:10 PM
sredman added inline comments.
plugins/telephony/telephonyMessage.h
27 ↗(On Diff #31082)

A Message (was) a mapping of field names to field values. This makes it very extensible since anybody who wants to add more data just has to add another key/value. However, you are right that this is not a very readable way to implement such a container.

The other advantage of extending QMap is that you get all the DBus marshalling/demarshalling for free. However, the model framework we have in place takes care of this, I think, using properties. I am still working out how that interface needs to work.

It does store a single message, but in order to work with the model framework (or DBus) the model being represented needs to be represented by its own class. For a similar example, see the Notification class in plugins/notifications/notification.{cpp,h}

Looking much better, but there is still room for improvement. Passing a QVariantMap in the constructor is IMHO a violation of the law of demeter. The constructor should not make assumptions/rely on the content of args. It should ask directly for what it wants, that is a String body and a String address. I assume you want to be extensible this way, but adding a third property, say an image that is being sent would still need changes in the class. That would be a violation of the open-closed principle, that states that a class should be closed for modification but open for extension. Taking the example of an image message: Instead of cramming all the logic into Message one should create a class ImageMessage that extends Message and takes the image as a third constructor argument.

nicolasfella added inline comments.Apr 1 2018, 5:48 PM
plugins/telephony/message.cpp
28 ↗(On Diff #31106)

You can use initializer list here

sredman marked an inline comment as done.Apr 1 2018, 5:51 PM

Looking much better, but there is still room for improvement. Passing a QVariantMap in the constructor is IMHO a violation of the law of demeter. The constructor should not make assumptions/rely on the content of args. It should ask directly for what it wants, that is a String body and a String address. I assume you want to be extensible this way, but adding a third property, say an image that is being sent would still need changes in the class. That would be a violation of the open-closed principle, that states that a class should be closed for modification but open for extension. Taking the example of an image message: Instead of cramming all the logic into Message one should create a class ImageMessage that extends Message and takes the image as a third constructor argument.

I don't disagree, but here is my thinking: Making a lot of individual arguments (this Message classs will need a few more, "type" at a minimum, and I like the concept of an ImageMessage) and passing them to the constructor offloads the logic of how a Message needs to be constructed from Message to the user. In other words, every place I want to construct a Message will have duplicate code. You're right that there is some worry about fragility by passing an unconstrained map of arguments.

sredman updated this revision to Diff 31113.Apr 1 2018, 5:54 PM

Support bulk message packets

sredman marked an inline comment as done.Apr 1 2018, 5:57 PM
nicolasfella added inline comments.Apr 8 2018, 6:22 PM
plugins/telephony/message.h
33 ↗(On Diff #31113)

How is the type going to be used? Is it constant through the lifecycle of the message object or does a message object has evolving states?

sredman added inline comments.Apr 8 2018, 7:03 PM
plugins/telephony/message.h
33 ↗(On Diff #31113)

It is constant through the life of the message. The Android documentation is not good, so I don't know what all the possible types actually mean, but the most important is that TYPE_SENT means the message was sent by us and TYPE_INBOX means the message was received by us

This revision was not accepted when it landed; it landed in state Needs Review.Apr 8 2018, 7:09 PM
This revision was automatically updated to reflect the committed changes.
nicolasfella reopened this revision.Apr 17 2018, 7:52 PM

Looks like your diff got lost

Not sure if I'm overly nitpicky here 😆

Will a message be always constructed from a QVariantMap? If not it would make more sense to ask for the actual parameters. To avoid code duplication one could make a helper method like

Message Message::fromQVariant(QVariant args) {}
sredman updated this revision to Diff 32504.Apr 18 2018, 7:14 PM

Reupload entire diff, not single file

Looks like your diff got lost

Hopefully one day I will make arc do what I want on the first try :)

Not sure if I'm overly nitpicky here 😆

Will a message be always constructed from a QVariantMap? If not it would make more sense to ask for the actual parameters. To avoid code duplication one could make a helper method like

Message Message::fromQVariant(QVariant args) {}

I think that is a fair suggestion, but what does such a method offer vs. a constructor which takes a QVariantMap?

At the moment, the only way I see a Message being created is from a QVariantMap, since the only way to get a message is from a network packet, and network packets contain QVariantMaps

Test plan should include some steps to reproduce the desired behavior and see what is already implemented and what not

plugins/telephony/telephonyplugin.cpp
146

"->this" is weird, never seen it in a KDE Project

149

Else if?

205

Use new syntax if possible

213

returning false here is not necessary

apol requested changes to this revision.Apr 18 2018, 11:20 PM
apol added a subscriber: apol.
apol added inline comments.
plugins/telephony/message.h
33 ↗(On Diff #32504)

Usually enums start with a capital letter and values are camelCased. Also no need to specify the value for each.
Specify it a Q_ENUM(types) too, it's better for debugging at least.

59 ↗(On Diff #32504)

const? and the m_address too?

plugins/telephony/telephonyplugin.cpp
207

Shouldn't initialize if it's empty.

210

return reply?
Actually nobody seems to be reading the result of forwardToTelepathy, you can probablty just omit returning anything and call asynchronously.

222

const

224

const QVariant &

plugins/telephony/telephonyplugin.h
110

requestAllConversations()?

This revision now requires changes to proceed.Apr 18 2018, 11:20 PM
sredman marked 10 inline comments as done.May 15 2018, 10:24 PM
sredman added inline comments.
plugins/telephony/message.h
33 ↗(On Diff #32504)

I have specified the values since they come from Android's values for the same field, just to be clear that they are the same (even though I know they would automatically be). Do you think this is a good idea?
Done on the rest of the suggestions. Thanks!

plugins/telephony/telephonyplugin.cpp
149

At this revision I hadn't figured out what to do with old-style packets. I have decided to support them fully (meaning old Android apps would continue to function as they currently do) and return at the end of the if block. Does this work for you?

205

The messageReceived method being used as the sender of this connection is on the other side of the DBus interface (telepathy-kdeconnect/connection.h line 55). I don't know how or if the new syntax can be used in this case

sredman updated this revision to Diff 34236.May 15 2018, 10:24 PM

Update based on reviews

  • Add remaining Telephony Message fields
  • Address review suggestions for const data
  • Correct naming convention of message.Type enum
  • Clean up forwardToTelepathy method
  • Rename sendAllConversationsRequest to requestAllConversations
  • Remove this->
  • Re-add createNotification method
  • Reenable support for old-style packets which generate notificatinos (SMS, incoming call, missed call)
  • Remove extra redundant telepathy code from notifications handling
  • Add header comment to createNotification
  • Add header comment for PACKET_TYPE_TELEPHONY
sredman edited the test plan for this revision. (Show Details)May 15 2018, 10:27 PM

Would you consider (when you're ready) updating the README to be a little more complete, since you're probably quite familiar with the expected packet formats?

sredman updated this revision to Diff 34254.May 16 2018, 4:11 AM

Correct field types based on reading Android docs

apol added inline comments.May 16 2018, 3:28 PM
plugins/telephony/message.h
28 ↗(On Diff #34254)

Mark CONSTANT if it's not going to change.

40 ↗(On Diff #34254)

Enums usually start with capital letter: MessageTypeAll

58 ↗(On Diff #34254)

getters in Qt usually don't have a get prefix: getBody() -> body()

plugins/telephony/telephonyplugin.cpp
61

Instead of passing "" or QLatin1String(""), pass QString() or even {}.

64–65

Shouldn't this be !m_telepathyInterface.isValid()?

This revision was not accepted when it landed; it landed in state Needs Review.May 16 2018, 7:24 PM
This revision was automatically updated to reflect the committed changes.
sredman reopened this revision.May 17 2018, 3:25 PM
sredman marked 5 inline comments as done.

Would you consider (when you're ready) updating the README to be a little more complete, since you're probably quite familiar with the expected packet formats?

Sure. Putting it somewhere centralized is probably a good idea. If you want to look at it now, currently all of that documentation lives in header comments on the various PACKET_TYPE_* fields

plugins/telephony/telephonyplugin.cpp
64–65

No, this is handling the case where the telepathy interface was valid. If the message has been passed to telepathy, it does not keep trying to process it using a notification

sredman updated this revision to Diff 34376.May 17 2018, 3:25 PM

Lots of Model work. SMS app is now able to display a list of convesations (which is far more exciting than it sounds)

sredman edited the test plan for this revision. (Show Details)May 17 2018, 3:36 PM
sredman marked 7 inline comments as done.May 17 2018, 4:58 PM
sredman added inline comments.
plugins/telephony/message.h
28 ↗(On Diff #34254)

Annoyingly this class has become mutable because I am required (I think) for the DBus support to implement operator=, so the properties are no longer constant

nicolasfella accepted this revision.Jun 6 2018, 10:01 PM

Let's merge it and refactor then

apol added a comment.Jun 6 2018, 10:13 PM

Hey careful there. You merged master instead of rebasing somewhere in between. Can someone clean the branch before merging?

sredman marked an inline comment as done.Jun 8 2018, 2:59 AM
In D11854#275000, @apol wrote:

Hey careful there. You merged master instead of rebasing somewhere in between. Can someone clean the branch before merging?

Are you sure? I rebased the branch onto master before using arc to push it here (but the branch is still in crazy land). How would I tell if it's messed up?

In D11854#275000, @apol wrote:

Hey careful there. You merged master instead of rebasing somewhere in between. Can someone clean the branch before merging?

Are you sure? I rebased the branch onto master before using arc to push it here (but the branch is still in crazy land). How would I tell if it's messed up?

Okay, having just had my first experience landing a commit from the Contacts plugin, I now see what you're saying. Are you happy with what you're seeing here on Phabricator? I can land exactly this with no trouble.

apol accepted this revision.Jun 8 2018, 12:15 PM

Yes, let's just put it as a big patch.

This revision is now accepted and ready to land.Jun 8 2018, 12:15 PM
This revision was automatically updated to reflect the committed changes.