API review: FavIconManager vs FavIconHostRequestJob

Thu Jan 28 08:35:53 UTC 2016

Hi 
I like the idea to move away from dbus.
It's better for sure.

Le jeudi 28 janvier 2016, 01:33:32 CET David Faure a écrit :
> I'm working on a replacement for the favicons kded module (which lives in
> libkonq), which will be a class in kio instead (proper C++ API certainly
> beats just a DBus interface as public API).
> 
> I started from the DBus API of the kded module, cleaned up a number
> of things (in particular the awful "(bool isHost, QString hostOrUrl)").
> 
> And this led me to the API I'm pasting below.
> 
> I think it kind of works and would be easy enough to port the apps from the
> current code to that, but the more I think about it, the less I like it
> .... I'm not 100% sure though so I'd like to discuss it with you guys
> before I rewrite it all and then think some more and have regrets :-)
> 
> So, first, to give some context, here's what I currently have:
> 
> namespace KIO {
> 
> /**
>  * FavIconsManager is the class for handling favicons.
>  *
>  * For instance, the icon for http://www.google.com exists at
> http://www.google.com/favicon.ico * This class will download the favicon,
> and make it available as a local PNG for fast lookups afterwards. *
>  * Usage:
>  * For a given HTTP URL, you can find out if a favicon is available by
> calling KIO::favIconForUrl() in KIOCore. * If it returns empty, you can
> then trigger a download of the favicon by calling downloadHostIcon(), * and
> wait for the signal hostIconChanged() to be emitted.
>  *
>  * @code
>  * // Assuming a member variable: FavIconsManager m_faviconsManager;
>  * connect(&m_faviconsManager, &KIO::FavIconsManager::hostIconChanged, this,
> &MyClass::slotHostIconChanged); *
>  * // Now let's say we want to show the icon for QUrl m_url
>  * QString icon = KIO::favIconForUrl(m_url);
>  * if (!icon.isEmpty()) {
>  *     // show icon right away
>  * } else {
>  *     m_favIconsManager.downloadHostIcon(m_url); // asynchronous
>  * }
>  *
>  * void MyClass::slotHostIconChanged(const QString &host, const QString
> &iconName) * {
>  *     if (m_url.host() == host) {
>  *         // show iconName
>  *     }
>  * }
>  * @endcode
>  *
>  * In Web Browsers, additional information exists: the HTML for a given page
> can * specify something like
>  *  <link rel="shortcut icon" href="another_favicon.ico" />
>  * To handle this, call setIconForUrl(pageUrl, iconUrl).
>  * (KParts-based web engines use the signal BrowserExtension::setIconUrl to
> call back * into the web browser application, which should then call this).
> * The signal urlIconChanged will be emitted once the icon has been
> downloaded. *
>  * The on-disk cache is shared between processes.
>  *
>  * @since 5.19
>  */
> class KIOGUI_EXPORT FavIconsManager : public QObject
> {
>     Q_OBJECT
> 
> public:
>     /**
>      * Creates a FavIconsManager.
>      * You can create multiple instance, they will share the same cache.
>      */
>     explicit FavIconsManager(QObject *parent = 0);
> 
>     /**
>      * Deletes the FavIconsManager.
>      */
>     ~FavIconsManager();
> 
>     /**
>      * Downloads the icon for a given host if it was not downloaded before
>      * or the download was too long ago. If the download finishes
>      * successfully, the hostIconChanged() signal is emitted, otherwise
>      * hostIconError() is emitted.
>      *
>      * @param url any URL on the host for which the icon is to be downloaded
> */
>     void downloadHostIcon(const QUrl &url);
> 
>     /**
>      * Associates an icon with the given URL. If the icon is already
>      * available (in cache) the signal urlIconChanged() is emitted
> immediately. * If the icon was not downloaded before or the downloaded was
> too long ago, * a download attempt will be started and the urlIconChanged()
> * signal is emitted after the download finished successfully. * In case of
> an error during the download, urlIconError() is emitted. *
>      * @param url the URL which will be associated with the icon
>      * @param iconURL the URL of the icon to be downloaded
>      */
>     void setIconForUrl(const QUrl &url, const QUrl &iconURL);
> 
> Q_SIGNALS:
>     /**
>      * Emitted once a new icon is available, for a host
>      */
>     void hostIconChanged(const QString &host, const QString &iconName);
> 
>     /**
>      * Emitted if an error occurred while downloading the icon for the given
> host. * You can usually ignore this (e.g. web browsers don't need to do
> anything if * no favicon was found), but this signal can be useful in some
> cases, e.g. * to let keditbookmarks know that it should move on to the next
> bookmark. */
>     void hostIconError(const QString &host, const QString &errorString);
> 
>     /**
>      * Emitted once a new icon is available, for a url
>      */
>     void urlIconChanged(const QUrl &url, const QString &iconName);
> 
>     /**
>      * Emitted if an error occurred while downloading the icon for the given
> host. * You can usually ignore this (e.g. web browsers don't need to do
> anything if * no favicon was found), but this signal can be useful in some
> cases, e.g. * to let keditbookmarks know that it should move on to the next
> bookmark. */
>     void urlIconError(const QUrl &url, const QString &errorString);
> 
> private:
>     FavIconsManagerPrivate *const d;
> 
>     Q_PRIVATE_SLOT(d, void slotData(KIO::Job *, QByteArray))
>     Q_PRIVATE_SLOT(d, void slotResult(KJob *))
>     Q_PRIVATE_SLOT(d, void slotKillJobs())
> };
> }
> 
> (full code at http://www.davidfaure.fr/2016/faviconsmanager.diff for the
> curious)

+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 ?

> 
> Notice how the sample code shows
> 1) that you need a manager object (could also be a singleton, but that's not
> very Qt like) 2) that you need to store m_url to be able to find out if the
> slot is being called for that url or for something unrelated.
> 
> The reason for that API was that it was all centralized between processes,
> so if process1 would trigger the download of the favicon for host1, all
> processes would then be informed that this favicon is now available.
> 
> My idea with FavIconManager was: shared on disk cache, but no DBus needed.
> So if process2 starts caring about host1 later, it can find it in the cache,
> but if for some reason it starts caring at the exact same time as process1
> then they will both download it, and both save it (I'll use a lock file);

A lock file will be usefull.
And as you write "Minor downside, but not expected to happen often."

For me api seems good.

> the last one wins. Minor downside, but not expected to happen often.
> 
> What this means though, is that it's always one signal for one request.
> Which leads to a Job API rather than this global-manager-with-signals.
> 
> Let's write an alternative API...
> 
> class FavIconHostRequestJob : public KJob
> {
> public:
>     explicit FavIconsHostRequestJob(const QString &host, QObject *parent =
> 0);
> 
>     QString host(); // as passed to the ctor
>     QString iconName(); // available once result() was emitted (and error()
> is 0) };
> 
> // 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.

> 
> and another job with a ctor
>   FavIconsIconUrlRequestJob(const QUrl &url, const QUrl &favIconUrl, QObject
> *parent = 0); These classnames are a bit long though ;)
> 
> Typical slot:
> void slotFavIconResult(KJob *job) {
>     FavIconHostRequestJob *iconJob = static_cast<FavIconHostRequestJob
> *>(job); if (iconJob->error()) {
>        // do something, or not
>     } else {
>         // show iconJob->iconName() as the icon for iconJob->host()
>     }
> }
> 
> One downside of these nice jobs: if two classes in one process need a
> favicon, they will both trigger a download. Unless..... a hidden singleton
> behind the scenes takes care of the downloads and associates jobs with the
> downloads... smells a bit like the KDirListerCache hell, but simpler so I
> hope it is doable somewhat cleanly.
> 
> That is, I think, a much more common case than two separate processes.
> Open one URL in a webbrowser -> favicon for the tab, favicon for the
> mainwindow, favicon for the combobox icon, for the history tree (and later
> on for adding as bookmark, but by then it's cached, hopefully).
> In Akregator I see both BrowserFrame and Feed registering for the favicon.
> 
> Migration strategy: the kded module will stay around until KF6, so we can
> take our time porting the apps to the new favicon class.

For the moment this favicon module is not released.
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 ?

For akregator if you release it in time I will port it directly.

> 
> Well, that was much thinking aloud. Any comments?
> 
> Anyone thinks the backend should be a kiod module instead, to avoid
> concurrent downloads of the same icon and notify all processes when the
> favicon is available?

-- 
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