Review Request 113222: Handle message delivery reports in MessagesModel

[David Edmundson]

> On Oct. 13, 2013, 5:34 a.m., Lasath Fernando wrote:
> > Hey,
> > 
> > This is a nice clean patch, and it's great that someone got around to handling delivery messages in MessagesModel.
> > 
> > However, I don't understand why you bothered creating a MessagePrivate class instead of just adding new attributes to KTp::Message. There's no real reason not to, since the reason we created KTp::Message was so we can arbitrarily add properties like this. 
> > 
> > I think you'd be much better off doing it that way, and the code would be cleaner :-)

Actually I said not to do that.

It's really only the message model that should be altering these things, therefore the data structure should be private to it. 
If it was external the model would need some way to know the message has changed - which would mean making Message inherit from QObject to emit signals and it all becomes rather messy.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113222/#review41624
-----------------------------------------------------------


On Oct. 12, 2013, 8:26 p.m., Leon Handreke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113222/
> -----------------------------------------------------------
> 
> (Updated Oct. 12, 2013, 8:26 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> -------
> 
> Handle message delivery reports and update original message object with the the new status and possibly an error message.
> 
> Introduce a new internal MessagePrivate class to hold extra properties for MessagesModel to avoid having to extend KTp::Message.
> 
> 
> Diffs
> -----
> 
>   KTp/Declarative/messages-model.cpp 9cf1606555edc7ed9f36970a957a854caf3010a0 
>   KTp/Declarative/messages-model.h a0f15653c3644f26dc768fe7ac882b4ac3c91367 
> 
> Diff: http://git.reviewboard.kde.org/r/113222/diff/
> 
> 
> Testing
> -------
> 
> Only tested receiving a Tp::DeliveryStatusDelivered. Not sure how to test the other cases.
> 
> 
> Thanks,
> 
> Leon Handreke
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20131013/56124e88/attachment.html>