Review Request 114559: Only try loading data from the relevant data source

[Martin Klapetek]

> On Dec. 20, 2013, 11:49 a.m., Alexandr Akulich wrote:
> > src/basepersonsdatasource.h, line 49
> > <http://git.reviewboard.kde.org/r/114559/diff/1/?file=226653#file226653line49>
> >
> >     So, one plugin can provide only one source? I'm ok for that, but as it ABI changes, we should think again - what's if plugin support several simular sources via one codebase? Though, such code can be extracted to library.
> 
> David Edmundson wrote:
>     I can't see it being a problem - it's a name to match a hidden identifier to a plugin so we know which plugin to query to get data.
>     Inside a plugin we can do multiple things, like Akonadi abstracts all sorts of crazy real sources - but to kpeople it's all just "akonadi"
>     
>     Maybe source is a bad name?
> 
> Alexandr Akulich wrote:
>     May be my understanding is not correct. I thinking that it is like "protocol" or "schema", so it is possible that several different plugins can provide same "sourceId" (sourceId is string like "skype" or "akonadi" or may be "jabber" and so on), so user can select prefered plugin/backend. At same time, one plugin (somewhat like gabble) may provide several sources.
> 
> David Edmundson wrote:
>     It's so that the PersonData knows which plugin to query to get the data. So jabber + msn + icq etc, will all be "ktp". If Kopete gets kpeople support the source would be "kopete" regardless of the underlying real source. 
>     
>     
>     Each plugin then only has to keep the local ID  unique within it's own protocol. I.e ktp can use a mesh the accounts+protocols,etc, akonadi can just keep their silly integer.
>     
>     I really don't want to allow two plugins to provide the same source (i.e two people having "jabber" or "facebook") as it would end up in a horrible conflicting mess.

I think better naming that method might help - sourcePluginId()? 

As it is pure virtual method and every extending class will have to implement, add comment what it needs to do.


- Martin


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


On Dec. 20, 2013, 1:27 a.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114559/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2013, 1:27 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: libkpeople
> 
> 
> Description
> -------
> 
> Only try loading data from the relevant data source
> 
> i.e only query akonadi for contacts starting with akonadi://
> 
> 
> Thoughts: 
>  - should this be a pure virtual method in the data source or a property in the .desktop file?
> 
>  - the parsing of the IDs to get the sourceId from the url-looking string is a bit rubbish, in a future patch I'll tidy this up with an ID class that doesn't have this horrible string parsing.
> 
> 
> Note this is an ABI break. 
> 
> 
> Diffs
> -----
> 
>   src/basepersonsdatasource.h 12f698e 
>   src/persondata.cpp 277fa6b 
>   src/personpluginmanager.h adc7b9d 
>   src/personpluginmanager.cpp e433bb5 
>   src/plugins/akonadi/akonadidatasource.h ef5c602 
>   src/plugins/akonadi/akonadidatasource.cpp b640d00 
> 
> Diff: http://git.reviewboard.kde.org/r/114559/diff/
> 
> 
> Testing
> -------
> 
> Opened PersonViewer
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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