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

[Alexandr Akulich]

> On Dec. 20, 2013, 4:49 p.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?

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.


> On Dec. 20, 2013, 4:49 p.m., Alexandr Akulich wrote:
> > src/personpluginmanager.h, line 38
> > <http://git.reviewboard.kde.org/r/114559/diff/1/?file=226655#file226655line38>
> >
> >     The pointer sign should be moved to the variable
> 
> David Edmundson wrote:
>     I think this is right isn't it? There is no variable name on a return type.

I reads http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Whitespace as generic rule, that order us to write it as
static BasePersonsDataSource *dataSource(const QString &sourceId);
IMO method signatures shouldn't go against this rule.
I'll be glad if someone clarify it for me.


- Alexandr


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


On Dec. 20, 2013, 6: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, 6: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/20131220/925f27c0/attachment.html>