Split SMS and Telephony plugin on desktop
ClosedPublic

Authored by nicolasfella on Jun 18 2018, 11:16 PM.

Details

Summary

Telephony and SMS handling are quite distinct so they should be in separate plugins for better maintainability, given that @sredman has big plans with SMS.
This diff should be fully backwards compatible, but whether we really want to do that is up to discussion

Test Plan

Only supeficially tested. Receive an SMS (old way), Notification is shown

Diff Detail

Repository
R224 KDE Connect
Branch
smsrefactor
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 141
Build 141: arc lint + arc unit
nicolasfella created this revision.Jun 18 2018, 11:16 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptJun 18 2018, 11:16 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
nicolasfella requested review of this revision.Jun 18 2018, 11:16 PM
sredman added inline comments.Jun 19 2018, 2:24 AM
plugins/sms/conversationsdbusinterface.h
94

We should probably change the name of this variable too (for instance, to m_smsInterface)

plugins/telephony/telephonyplugin.cpp
48–52

Needless to say, all of this can be removed since the telephony plugin no longer has a conversation interface

apol added a subscriber: apol.Jun 20 2018, 12:25 AM

LGTM, I'm not sure that it's required but it certainly doesn't hurt.

  • Adapt DBus path in CLI
apol added a comment.Jul 26 2018, 5:36 PM

Wouldn't this collide with @sredman though?

In D13594#298619, @apol wrote:

Wouldn't this collide with @sredman though?

Yes, in a sense, but this branch is the next thing I intend to work on... Once the code is cleaned up, the path forward for improvements should be clearer

  • Remove leftover comment
  • Rename variable
nicolasfella marked 2 inline comments as done.Aug 4 2018, 7:47 PM

Is this still WIP?

Is this still WIP?

I suppose it is complete, but it need some thorough testing

nicolasfella retitled this revision from [WIP] Split SMS and Telephony plugin on desktop to Split SMS and Telephony plugin on desktop.Aug 13 2018, 10:01 PM

I am getting lots of printed warnings that the Telepathy interfaces does not seem to be working. I don't have telepathy set up right now so this is not a big surprise. The cause appears to be a missing check for whether the interface is valid (see inline comments)

Also, after rebasing or merging master the logic for connecting message addresses with KPeople seems to be broken so we don't get names or icons in the SMS gui list. I have not looked into why this is.

plugins/sms/smsplugin.cpp
72

In the master branch, this check has been brought within the forwardToTelepathy method

185

This line is causing an invalid call to the telepathy dbus interface since we have not asked if m_telepathyInterface.isValie()

plugins/telephony/telephonyplugin.cpp
211

This check should either be brought back within this method or bumped outside in all uses

Also, after rebasing or merging master the logic for connecting message addresses with KPeople seems to be broken so we don't get names or icons in the SMS gui list. I have not looked into why this is.

This does not appear to be due to this patch. My contacts are not synchronizing for some reason.

sredman updated this revision to Diff 41226.Sep 8 2018, 9:01 PM
  • Check if telepathy interface is valid before calling it
sredman accepted this revision as: sredman.Sep 8 2018, 9:08 PM

This has been working for me for a couple of weeks, so I think we're good. The tiny change I just pushed - to check the Telepathy interface before calling it - avoids an error and doesn't change any functionality (since it didn't work before anyway!)

plugins/sms/kdeconnect_sms.json
12

Also perhaps we should find a new icon for this. How do I look at the list of available icons?

This revision is now accepted and ready to land.Sep 8 2018, 9:08 PM

I also rebased onto master, but this is actually how I have been testing it :)

nicolasfella added inline comments.Sep 10 2018, 9:10 AM
plugins/sms/kdeconnect_sms.json
12

cuttlefish is a tool for that

This revision was automatically updated to reflect the committed changes.