<table><tr><td style="">sredman marked 2 inline comments as done.<br />sredman added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D11854">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11854#inline-59180">View Inline</a><span style="color: #4b4d51; font-weight: bold;">nicolasfella</span> wrote in <span style="color: #4b4d51; font-weight: bold;">telephonyMessage.h:27</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">In general you should favor composition over inheritance, you should read about that a bit.<br />
Meaning: TelephonyMessage should <em>have</em> a QMap to store the information, not <em>be</em> one. The public interface to TelephonyMessage should be independent of the underlying data structure, because our needs could change.</p>

<p style="padding: 0; margin: 8px;">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</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>

<p style="padding: 0; margin: 8px;">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.</p>

<p style="padding: 0; margin: 8px;">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}</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R224 KDE Connect</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11854">https://phabricator.kde.org/D11854</a></div></div><br /><div><strong>To: </strong>sredman, KDE Connect, nicolasfella<br /><strong>Cc: </strong>nicolasfella, KDE Connect, Danial0_0, krantideep95, Pitel, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol<br /></div>