Review Request: Removed accounts not updated in Nepomuk

[Dario Freddi]

> On 2010-04-27 12:57:23, Daniele E. Domenichelli wrote:
> > /trunk/playground/network/telepathy-integration-daemon/telepathyaccountmonitor.cpp, line 114
> > <http://reviewboard.kde.org/r/3784/diff/1/?file=24360#file24360line114>
> >
> >     This queries returns all IMAccount that the "me" PersonContact has.
> >     Actually also all contacts of removed accounts and removed contacts are not updated and status will always be the last status.
> >     So I believe that a better query could be:
> >     QString query = QString("select distinct ?a where { ?a a %1 }")                            .arg(Soprano::Node::resourceToN3(Nepomuk::Vocabulary::NCO::IMAccount()));
> >     to unset the status for all contacts before creating accounts

Not really: otherwise the query would get all of the existing IMAccounts, which include the IMAccounts of each and every contact, whereas we're interested in "real" accounts here.

To do what you want to achieve (if I understood you correctly), you would need to make an additional query finding out all the PersonContact/IMAccounts which isBuddyOf the account which is being removed.


> On 2010-04-27 12:57:23, Daniele E. Domenichelli wrote:
> > /trunk/playground/network/telepathy-integration-daemon/telepathyaccountmonitor.cpp, line 129
> > <http://reviewboard.kde.org/r/3784/diff/1/?file=24360#file24360line129>
> >
> >     I'm not sure if imStatusMessage should be cleaned, maybe it should represent last status when the user was online...

I don't think so. More generally, I don't really get why you're doing such a thing in the constructor (iterating over accounts and clearing their properties).


> On 2010-04-27 12:57:23, Daniele E. Domenichelli wrote:
> > /trunk/playground/network/telepathy-integration-daemon/telepathyaccountmonitor.cpp, line 150
> > <http://reviewboard.kde.org/r/3784/diff/1/?file=24360#file24360line150>
> >
> >     Pointer should be saved, to be able to call delete later (see below)
> >     Probably something like this is needed:
> >     (in .h)
> >     QHash<QString, TelepathyAccount*> m_accounts;
> >     (here)
> >     m_accounts.insert(path, new TelepathyAccount(path, this));

Agreed - using a QHash is the right way. Remember to clean it up in onAccountRemoved, though.


> On 2010-04-27 12:57:23, Daniele E. Domenichelli wrote:
> > /trunk/playground/network/telepathy-integration-daemon/telepathyaccountmonitor.cpp, line 153
> > <http://reviewboard.kde.org/r/3784/diff/1/?file=24360#file24360line153>
> >
> >     This function should probably just call TelepathyAccount::~TelepathyAccount instead of doing all this stuff. 
> >     All this can be done by the destructor.
> >     Also all contacts in the buddy list for removed account should be unset by TelepathyAccount destructor.

Definitely better. One thing: how does this help when dealing with a removed account? The account would still be available through Nepomuk. See my next review for that.


- Dario


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


On 2010-04-23 11:13:26, Daniele E. Domenichelli wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3784/
> -----------------------------------------------------------
> 
> (Updated 2010-04-23 11:13:26)
> 
> 
> Review request for telepathy and George Goldberg.
> 
> 
> Summary
> -------
> 
> When an account is removed from telepathy-mission-control, Nepomuk data is not updated.
> This means that, for example, if last status for a removed account was online, querying Nepomuk will always return that account among other existing accounts and will always affirm that removed account is online.
> 
> This patch sets statusType property to Tp::ConnectionPresenceTypeUnset in Nepomuk before creating accounts. Later, statusType is updated only for existing accounts, not for removed ones.
> Also adds "onAccountRemoved" slot that resets statusType to Tp::ConnectionPresenceTypeUnset when account is removed.
> 
> 
> Diffs
> -----
> 
>   /trunk/playground/network/telepathy-integration-daemon/telepathyaccountmonitor.h 1116405 
>   /trunk/playground/network/telepathy-integration-daemon/telepathyaccountmonitor.cpp 1116405 
> 
> Diff: http://reviewboard.kde.org/r/3784/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Daniele E.
> 
>