-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126834/#review93493
-----------------------------------------------------------



General approach seems like a good idea.

You have one big bug in here (comment #2).


KTp/Models/presence-model.cpp (line 85)
<https://git.reviewboard.kde.org/r/126834/#comment63745>

    you should be able to have 
    presenceModelChanged(KTp::Presence, bool)
    
    and have it automatically cast as long as you have registered KTp::Presence 
as a DBus type



KTp/Models/presence-model.cpp (line 184)
<https://git.reviewboard.kde.org/r/126834/#comment63747>

    on the initial load we call addPresence; addPresence is emitting DBus 
signals to all the other clients when it's not actually adding anything, but 
just loading the existing ones which all the other clients should already have.
    
    It won't break as the other clients won't do anything, but it's rather 
wasteful.



KTp/Models/presence-model.cpp (line 230)
<https://git.reviewboard.kde.org/r/126834/#comment63746>

    Personally I'd use two different DBus signals for add/remove.


- David Edmundson


On Feb. 23, 2016, 7:16 a.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126834/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2016, 7:16 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> -------
> 
> New features:
> -Simplified API, sends and collects changes over DBus.
> 
> 
> Diffs
> -----
> 
>   KTp/Models/presence-model.h 8f206b880f48640626322269a14956f105482f69 
>   KTp/Models/presence-model.cpp ddc1a7c75f1a452bf3ac2db1aecbd88a5d1ce519 
> 
> Diff: https://git.reviewboard.kde.org/r/126834/diff/
> 
> 
> Testing
> -------
> 
> Compile, run.
> 
> 
> Thanks,
> 
> James Smith
> 
>

_______________________________________________
KDE-Telepathy mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-telepathy

Reply via email to