[Bug 20034] Avatar cache reference implementation

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=20034

--- Comment #9 from Xavier Claessens <xclaesse at gmail.com> 2010-09-08 01:51:56 PDT ---
(In reply to comment #8)
> Coding style issues:
> +    Q_FOREVER {
> +        tmpDir = QDir::tempPath() +
> QString::fromLatin1("/avatars-%1").arg(i++);
> +        if (QDir().mkdir(tmpDir))
> +            break;
> +    }
> I am afraid that this code will run forever if we don't have write access to
> QDir::tempPath(). Not that this will ever going to happen. Not a blocker
> anyway.

Fixed that by using a random directory, like tp-glib test does.

> +    setenv ("XDG_CACHE_HOME", a.constData(), true);
> +    QVERIFY (SmartDir(tmpDir).removeDirectory());
> Extra spaces here. Also there are many other places where there are extra
> spaces in TestContactsAvatar::createContactWithFakeAvatar.

Fixed

> All coding style in HACKING should be applied to glib/C code inside written a
> C++ source. Especially lowerCamelCase for variable names and no extra space
> before (. For example:
> +    TpHandleRepoIface *service_repo = tp_base_connection_get_handles (
> +        (TpBaseConnection *) mConnService, TP_HANDLE_TYPE_CONTACT);
> +    handle = tp_handle_ensure (service_repo, id, NULL, NULL);
> Should be:
> +    TpHandleRepoIface *serviceRepo = tp_base_connection_get_handles(
> +        (TpBaseConnection *) mConnService, TP_HANDLE_TYPE_CONTACT);
> +    handle = tp_handle_ensure(serviceRepo, id, NULL, NULL);

Fixed

> +     * AvatarRetrived should NOT be called */
> typo in AvatarRetrieved

Fixed

> +    SmartDir(const QString &path):QDir(path) { }
> Should be:
> +    SmartDir(const QString &path) : QDir(path) { }

Fixed

> +    Q_FOREACH(QFileInfo info, list) {
> should be:
> +    Q_FOREACH (QFileInfo info, list) {

That's not consistent with the rule of not having space before '(', but Fixed.

> +        }
> +        else {
> should be:
> +        } else {

Hm, I've seen that coding style in various qt code... but Fixed.

> + QString path = cacheDir + QString::fromLatin1("/telepathy/avatars/") +
> connection->cmName() + QString::fromLatin1("/") + connection->protocolName();
> Avoid using QString + operator. This code should be changed to something like:
>     QString path = QString(QLatin1String("%1/telepathy/avatars/%2/%3")).
>        
> arg(cacheDir).arg(connection->cmName()).arg(connection->protocolName());
> 
> Do this for all places where you are using the + operator and
> QString::fromLatin1().

Fixed

> Also try to declare variables right before the usage if possible. utils.cpp
> could have some c++ love there. But don't bother changing this, not a blocker
> at all.

Old C reflex... :P

> Sorry being so pedantic about coding style :/.

Don't be sorry, pedantic review is what I need to learn :)

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.