Review Request: Make Contactable Objects implement the Contactable interface

[Dario Freddi]

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


Overall pretty good, some stuff to fix. Btw, the reduceToContactSet stuff apparently applies to Contactable's API - I'd like to see that changed indeed.


src/core/contact-set.h
<http://git.reviewboard.kde.org/r/102247/#comment5022>

    same



src/core/contact-set.cpp
<http://git.reviewboard.kde.org/r/102247/#comment5023>

    I'd name that firstLocalAccount instead



src/core/contact-set.cpp
<http://git.reviewboard.kde.org/r/102247/#comment5024>

    This check might be quite ugly IMHO. Ideally, all the contacts in the list should have the same localAccount, so it would be great if that check were to happen outside the loop - it might be confusing otherwise



src/core/contact.h
<http://git.reviewboard.kde.org/r/102247/#comment5021>

    toContactSet() would be actually more consistent here



src/core/person-set.cpp
<http://git.reviewboard.kde.org/r/102247/#comment5025>

    First of all, all of your foreach cycle miss constness, so please Q_FOREACH(const PersonPtr &person, people()) around (including previous occurrrences).
    
    Besides that, you can actually make this code less ugly by:
    
    QHash<Nepomuk::Resource, SimpleContactSetPtr> stuff;
    
    Then just do the first iteration and add stuff to the hash, and iterate over it in the end. This spares quite some cycles and memory.


- Dario


On Aug. 7, 2011, 3:11 p.m., George Goldberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102247/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2011, 3:11 p.m.)
> 
> 
> Review request for Telepathy and Dario Freddi.
> 
> 
> Summary
> -------
> 
> The patch I meant to submit last week - this actually implements the Contactable interface for all our Contactable classes. It doesn't do any complicated logic yet when dealing with Person or PersonSet. That's something we need to put a bit more thought into, but for now just a basic approach is good enough just to get things moving.
> 
> 
> Diffs
> -----
> 
>   src/core/contact-set.h 56cef034fcf990bb1a6d4c6f0e7fb117531de7e2 
>   src/core/contact-set.cpp b47465b974358a0b55630ac66c7c1e6688e50c02 
>   src/core/contact.h 23cb0a9100cea45e0d80653c3ded8b3630544713 
>   src/core/contact.cpp 19172f02ee22c0489dfd6a84a75ed7026227a2e5 
>   src/core/contactable.h 8266d9a3b5cc9e49bde6db7ac2fb7e386734e19e 
>   src/core/contactable.cpp ec01d440c73d26adcdb851a6997dea59af00d007 
>   src/core/person-set.h ab52d367698c1e94d6c5442dbe538b8250bfe3db 
>   src/core/person-set.cpp 4d4a945e6f3e15b0bd029e924b9269546cb158a7 
>   src/core/person.h 7a262dac126f24e28266e723f17ea8221bf7d623 
>   src/core/person.cpp 8c0b622490819c3acc2d830dbbdfca61fc120d95 
> 
> Diff: http://git.reviewboard.kde.org/r/102247/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> George
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20110811/aa54b37b/attachment-0001.html>