API review: FavIconManager vs FavIconHostRequestJob

[laurent Montel]

Le dimanche 31 janvier 2016, 10:40:12 CET David Faure a écrit :
> On Thursday 28 January 2016 09:35:53 laurent Montel wrote:
> > +ecm_qt_declare_logging_category(kiogui_SRCS HEADER favicons_debug.h
> > IDENTIFIER FAVICONS_LOG CATEGORY_NAME kde.kio.favicons)
> > In this directory we will use just favicon ?
> > Perhaps a more generic log name no ?
> 
> Well, one debug area per lib is the lazy solution. More precise debug areas
> allow to turn on favicon debugging without turning on a whole lot of other
> stuff.

For me too, but as you create an unique ecm_qt_declare_logging_category for 
this lib I thought that you wanted to create just one debug area.
=> you will add other ecm_qt_declare_logging_category for each new class in 
kiogui lib ?

> > For me api seems good.
> 
> Which one? The Manager API or the Job API? :-)

I spoke about Manager I prefere this one.

> 
> > > // explicit start() like KJob says, or automatic start() like KIO does?
> > > // I'm always afraid people will forget to call start(), given the
> > > current
> > > inconsistency...
> > 
> > Yep some time I forgot it in kmail when I used manual call to start.
> > So if you want to use it make it automatic it's better.
> 
> I agree, but then I wonder why KJob doesn't autostart (with an early
> exit in start if it was already called). Kévin?
> 
> > For the moment this favicon module is not released.
> 
> Ah, right, I keep forgetting that.
> 
> > So all existing application will failed to register to it until it's
> > release So it's not a real problem if we migrate now to your class.
> > Now how many application use it ?
> 
> https://lxr.kde.org/search?_filestring=&_string=%22%2Fmodules%2Ffavicons%22
> says:
> KIO, keditbookmarks, konqueror, akregator, the rss dataengine in plasma.

Keditbookmarks is not released, konqueror is not.
Rss dataengine I don't know

Akregator I can port it :)

> 
> Of course many other apps use it indirectly, via the KIO::iconNameForUrl()
> API, so they will benefit from this, but they don't need any porting.

Indeed.
So really just rss dataengine + akregator needs to be ported.

Regards


-- 
Laurent Montel | laurent.montel at kdab.com | KDE/Qt Senior Software Engineer
KDAB (France) S.A.S., a KDAB Group company
Tel. France +33 (0)4 90 84 08 53,  http://www.kdab.fr