Review Request 115328: KTp-KDED: Save all persistent information on a contact to a database

[David Edmundson]

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



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34206>

    You can delete this.



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34198>

    and we need an index on the first two columns



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34201>

    Just a reminder for me.
    
    We must update the rest of the kded module to use KTp::accountManager otherwise we will have 2. Which means twice as much DBus traffic.



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34199>

    I can imagine this being a problem.
    
    .bind automatically escapes for us.
    
    which means we end up saying
    
    WHERE accountID NOT IN ('myAccount1,myAccount2')
    
    instead of
    
    ('myAccount1','myAccount2')
    



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34204>

    I'm surprised this works.
    
    At no point do we need to call 
    connection->becomeReady(FeatureRoster);
    
    at which point it's not trying to load the whole roster (contact list).
    
    We have this off by default in the ConnectionFactory to make loading faster.



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34200>

    coding style {}



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34202>

    we need to use onConnectionChanged 
    
    On the account DBus object we have 2 parameters; connectionStatus and a path to the connection DBus object.
    
    isOnline is a wrapper round the connectionState.
    
    At the time this happens, we have the path to the new DBus connection object, but we have not finished loading our Tp::Connection object yet. This will happen nanoseconds afterwards. Tp::Account::connection() is only set once Tp::Connection has loaded. Basically you end up in a race condition.
    
    Your code does exit out gracefully in the error checks, but we will miss a lot of updates.



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34203>

    why have an if on the connect()?



contact-cache.cpp
<https://git.reviewboard.kde.org/r/115328/#comment34205>

    Is this actually needed?
    
    My KTp::Account sets a property on the connection object. 
    
    At the time a contactManager state changes you will always have a connection object.
    
    at which point we can do
    
    if (contactManager->connection()) {
      const QString accountPath = contactManager->connection->property("accountUID").toString();
      Tp::AccountPtr account = KTp::accountManager()->accountForPath(accountPath));
    }
    
    The whole connection->account mapping always results in a bit of a hack, I don't want to have two hacks.


- David Edmundson


On Jan. 27, 2014, 11:52 a.m., Alexandr Akulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115328/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2014, 11:52 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-kded-module
> 
> 
> Description
> -------
> 
> Was: https://git.reviewboard.kde.org/r/115060/
> 
> Draft version. Not proposed to be merged as is.
> 
> TODO:
> * Fix code
> * Add groups info
> * Performance testing.
> 
> 
> Diffs
> -----
> 
>   telepathy-module.cpp a754283 
>   contactnotify.cpp 314d48a 
>   contact-cache.h PRE-CREATION 
>   contact-cache.cpp PRE-CREATION 
>   CMakeLists.txt a31245b 
> 
> Diff: https://git.reviewboard.kde.org/r/115328/diff/
> 
> 
> Testing
> -------
> 
> Tested accounts contact refreshing in follow action:
> 1 Does connect via Telepathy.
> 2 Checked that contacts added to db (via sqlite db viewer).
> 3 Disconnected in telepathy.
> 4 Connect via another IM software.
> 5 Removed few contacts.
> 6 Does connect via Telepathy.
> 7 Checked that there is only actual contacts in db (via sqlite db viewer).
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20140127/9a9181ca/attachment.html>