[Desktop] Make message syncronization interface capable of handling future changes
ClosedPublic

Authored by sredman on Nov 1 2018, 8:31 PM.

Details

Summary

Add "event" field to ConversationMessage

Update packet type with proper field names and commenting

Future "proof" SmsPlugin against new event types

Test Plan
  • Install the corresponding Android-side patch D16600
  • Verify that it is possible to synchronize messages the same as it was before

Diff Detail

Repository
R224 KDE Connect
Branch
messaging-future-view
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4870
Build 4888: arc lint + arc unit
sredman created this revision.Nov 1 2018, 8:31 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptNov 1 2018, 8:31 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
sredman requested review of this revision.Nov 1 2018, 8:31 PM
apol added a subscriber: apol.Nov 2 2018, 11:27 PM
apol added inline comments.
interfaces/conversationmessage.h
95

How about isTextMessage?
Pure reads weird.

sredman added inline comments.Nov 5 2018, 2:54 AM
interfaces/conversationmessage.h
95

The reason for not "isTextMessage" is because you could have a picture message which also contains text, so saying "isTextMessage" would technically be not true. The message I am trying to get across with "containsPureText" is that the message contains something which can be directly displayed, as opposed to HTML or some other weirdness. I could change it to something like "containsTextBody" or something if you like that better

apol added a comment.Nov 12 2018, 3:53 PM

Other than that, let's get this in.

interfaces/conversationmessage.h
95

I like containsTextBody more, yes.

sredman updated this revision to Diff 45366.Nov 12 2018, 5:07 PM
  • Rename containsPureText to containsTextBody
sredman marked 3 inline comments as done.Nov 12 2018, 5:08 PM
In D16599#358423, @apol wrote:

Other than that, let's get this in.

I will need to upload the Android-side patch too, since without that change this plugin will break. Could you accept that one too? :)

apol accepted this revision.Nov 13 2018, 2:18 AM

I know it sucks reading this, but I don't feel qualified to accept a patch in android.

This revision is now accepted and ready to land.Nov 13 2018, 2:18 AM
sredman closed this revision.Nov 16 2018, 12:04 AM