Review Request: Add a class providing a GlobalContact list throughout all accounts

[Daniele Elmo Domenichelli]

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


Just a quick review of the API...
This looks like a very useful addition, perhaps it's even worth to have it in tp-qt


KTp/global-contact-manager.h
<http://git.reviewboard.kde.org/r/106404/#comment14895>

    Perhaps it could derive from QPair<Tp::AccountPtr, Tp::ContactPtr> to get some operators for free?
    
    By the way since you are exporting this class and I believe that it could be used in several other places, is it worth to have it in a separate file? (Or perhaps we can just add the a pretty header for this class that include this file)



KTp/global-contact-manager.h
<http://git.reviewboard.kde.org/r/106404/#comment14900>

    A constructor that takes a Tp::AccountManagerPtr could be useful? Or perhaps even a constructor that takes the same arguments as  Tp::AccountManager::create?



KTp/global-contact-manager.h
<http://git.reviewboard.kde.org/r/106404/#comment14896>

    white spaces



KTp/global-contact-manager.h
<http://git.reviewboard.kde.org/r/106404/#comment14899>

    To avoid ABI problems in the future I think that these should be in the private class and slots declared using Q_PRIVATE_SLOT



KTp/global-contact-manager.h
<http://git.reviewboard.kde.org/r/106404/#comment14897>

    white spaces


- Daniele Elmo Domenichelli


On Sept. 9, 2012, 4:55 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106404/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2012, 4:55 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Add a class providing a GlobalContact list throughout all accounts.
> 
> So far 3 people have managed to get this wrong when implementing this, including Rohan, Me, George G and Dario so it would be good to have one class that does it right, also I don't like duplicate code.
> 
> This is to fix Rohan's patch in the KDED module for monitoring account presences as well as removing all the duplciate code that will be in contact-request-handler. This code will also be useful for the nepomuk-service which also has an implementation of this.
> 
> This will also fix the contact-request-handler not being able to display which account a new contact request comes from.
> 
> 
> Diffs
> -----
> 
>   KTp/CMakeLists.txt fa0c741564e09c020fbe3d3e8b3d375fe9c4c3df 
>   KTp/global-contact-manager.h PRE-CREATION 
>   KTp/global-contact-manager.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106404/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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