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

[Simon Redman]

sredman marked 2 inline comments as done.
sredman added inline comments.

INLINE COMMENTS

> nicolasfella wrote in telephonyMessage.h:27
> 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

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}

REPOSITORY
  R224 KDE Connect

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

To: sredman, #kde_connect, nicolasfella
Cc: nicolasfella, #kde_connect, Danial0_0, krantideep95, Pitel, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180401/b5ce140d/attachment.html>