Review Request: Port Presence Data Engine to tp-qt4 0.5, and add Services and Properties

[David Edmundson]

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


A few comments below.

I would suggest using an AccountFactory (a Tp-Qt 0.5 features) when you create the accountManager, it simplifies the code quite a lot, and stops you retrieving data before it's ready by accident.



presence.h
<http://git.reviewboard.kde.org/r/100968/#comment1846>

    suggest onAccountAdded() for consistency
    
    up to you, as you do also call it like a conventional method as well.



presence.cpp
<http://git.reviewboard.kde.org/r/100968/#comment1847>

    If you're using and AccountSet of valid accounts you shouldn't be monitoring the accounts for accountValidityChanged, nor for accountManager fo newAccount / accountRemoved.
    
    An AccountSet is a lot more than just a simple typedef, you haven't made the full use of it.
    
    You should watch the AccountSet for accountAdded and accountRemoved, which is doing all 3 of the above for you whilst applying the filter. (at least that's how I understand it)
    
    (currently yours doesn't list invalid accounts at the start, but will list any newly added invalid accounts, this will fix that)
    
    



presence.operations
<http://git.reviewboard.kde.org/r/100968/#comment1845>

    should this not say setAvatar?



presencesource.cpp
<http://git.reviewboard.kde.org/r/100968/#comment1843>

    qDebug -> kDebug



presencesource.cpp
<http://git.reviewboard.kde.org/r/100968/#comment1844>

    qDebug -> kDebug


- David


On March 29, 2011, 12:17 p.m., Dario Freddi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100968/
> -----------------------------------------------------------
> 
> (Updated March 29, 2011, 12:17 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This patch ports the data engine to Telepathy Qt4 0.5.x, and adds new Service operations for setting avatars and nicknames. It also enhances data retrieval and fixes some small bugs here and there.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 409c6135e53ecee63f02a756a9c0d8c5c399fe4e 
>   plasma-dataengine-presence.desktop 7a850af570113882b1d8c3509e3c260a2fc77353 
>   presence.h eb5d6dac20a9e7469508d4b87bb7fd02a5624c24 
>   presence.cpp 179c8055bbf29bd17eb80faf8fd14a8549a453a9 
>   presence.operations 0aed15758af9cab6780bd8b31822ce991d045983 
>   presenceservice.cpp d7a9a64d3b04acbb5ae19c9b7194c2f47efae74c 
>   presencesource.h 586b70f258d7b9bd0bc239aba25b599a038cfd4a 
>   presencesource.cpp fdab2c400e8833844c7feed71d77927acd9160cf 
>   setavatarjob.h PRE-CREATION 
>   setavatarjob.cpp PRE-CREATION 
>   setnicknamejob.h PRE-CREATION 
>   setnicknamejob.cpp PRE-CREATION 
>   setrequestedpresencejob.h 40c2ca1f5abff5d82c49f9ac1019b162e9249128 
>   setrequestedpresencejob.cpp 0a77a48941ab41cebe59b6646ba5bbde796ad7a4 
> 
> Diff: http://git.reviewboard.kde.org/r/100968/diff
> 
> 
> Testing
> -------
> 
> Engine explorer works flawlessly
> 
> 
> Thanks,
> 
> Dario
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110329/df00a7b0/attachment-0001.htm