Bug 20034

This should include telepathy-python too

Only if you want to write *clients* in telepathy-python, which is not currently very easy due to dbus-python and telepathy-python deficiencies. Our policy is that clients, not CMs, should provide the avatar cache.

To put it another way, if you're writing a client in telepathy-python, lack of an avatar cache is the least of your worries :-)

Did the same API as we have in telepathy-glib

So here follows the review, already talked to Xavier about them, just in case others want to check also and have anything else to discuss:

- Add a structure Tp::AvatarInfo and use it for avatar getter (AvatrarInfo avatarInfo() const) and signal (void avatarInfoChanged(const Tp::AvatarInfo &info). That is good so once we add Tp::Contact Qt property support we can have property notification for avatar info changes
- Check coding style:
   - Always use {} even for one line statements
   - type& should be type &
   - See HACKING for more info
- Call RequestAvatars in a batch instead of doing it 1 contact by 1 (known issue)
- Tests (known issue)

(In reply to comment #4)
> - Add a structure Tp::AvatarInfo and use it for avatar getter (AvatrarInfo
> avatarInfo() const) and signal (void avatarInfoChanged(const Tp::AvatarInfo
> &info). That is good so once we add Tp::Contact Qt property support we can have
> property notification for avatar info changes

done

> - Check coding style:
>    - Always use {} even for one line statements

done

>    - type& should be type &

done

> - Call RequestAvatars in a batch instead of doing it 1 contact by 1 (known
> issue)

done

> - Tests (known issue)

done

So here it goes another round:


+        realFeatures.insert (Contact::FeatureAvatarToken);
Extra space

+        connection->cmName() +QDir::separator() + connection->protocolName();
Missing space after '+'
Also just use "/" instead of QDir::separator() as we are already using "/" everywhere.

+    if (createDir && !QDir().mkpath(path))
+        return false;
Missing {}

+  mPriv->connection->avatarsInterface()->RequestAvatars(mPriv->requestAvatarsQueue);
This code is sync, change it to use QDBusPendingCallWatcher.

+void ContactManager::requestContactAvatar(Contact *contact)
Make this private to ContactManager and use ContactPtr instead of Contact*

+AvatarData Contact::avatarData() const
+{
+    if (!mPriv->requestedFeatures.contains(FeatureAvatarData)) {
+        warning() << "Contact::avatarFile() used on" << this
The warning should use Contact::avatarData instead

+                if (manager()->supportedFeatures().contains(FeatureAvatarData)) {
+                    mPriv->actualFeatures.insert(FeatureAvatarData);
+                }
+                mPriv->updateAvatarData();
The call to updateAvatarData should be inside the if clause.

I think that buildAvatarFileName should not create the dir itself, as the name suggests it will only build the fileName, move the code to create the dirs outside or rename the method.

I would also sed s/AvatarData/AvatarInfo/g as we don't have the "data/bytearray" there, instead we have the info to where the avatar is located and the mimetype.

Other than that, try using QLatin1String over QString::fromLatin1

(In reply to comment #6)
> So here it goes another round:
> 
> 
> +        realFeatures.insert (Contact::FeatureAvatarToken);
> Extra space

Fixed

> +        connection->cmName() +QDir::separator() + connection->protocolName();
> Missing space after '+'
> Also just use "/" instead of QDir::separator() as we are already using "/"
> everywhere.

Fixed

> +    if (createDir && !QDir().mkpath(path))
> +        return false;
> Missing {}

Fixed

> + 
> mPriv->connection->avatarsInterface()->RequestAvatars(mPriv->requestAvatarsQueue);
> This code is sync, change it to use QDBusPendingCallWatcher.

Fixed

> +void ContactManager::requestContactAvatar(Contact *contact)
> Make this private to ContactManager and use ContactPtr instead of Contact*

Fixed

> +AvatarData Contact::avatarData() const
> +{
> +    if (!mPriv->requestedFeatures.contains(FeatureAvatarData)) {
> +        warning() << "Contact::avatarFile() used on" << this
> The warning should use Contact::avatarData instead

Fixed

> +                if
> (manager()->supportedFeatures().contains(FeatureAvatarData)) {
> +                    mPriv->actualFeatures.insert(FeatureAvatarData);
> +                }
> +                mPriv->updateAvatarData();
> The call to updateAvatarData should be inside the if clause.

Fixed

> I think that buildAvatarFileName should not create the dir itself, as the name
> suggests it will only build the fileName, move the code to create the dirs
> outside or rename the method.

We use the same name in tp-glib with the same behaviour. It is easier to mkdir inside that func because we have the dir path there, otherwise I should return it as well. Do you have another method name to suggest?

> I would also sed s/AvatarData/AvatarInfo/g as we don't have the
> "data/bytearray" there, instead we have the info to where the avatar is located
> and the mimetype.

I prefer AvatarData to not be confusing with ContactInfo, also tp-glib use AvatarData naming. It is about the image itself, AvatarInfo would suggest the token is in it too.

> Other than that, try using QLatin1String over QString::fromLatin1

ok

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.


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

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


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


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


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


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


+ 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().

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.

Sorry being so pedantic about coding style :/.

(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 :)

+#define RAND_STR_LEN 6
+    gchar randStr[RAND_STR_LEN + 1];
+    for (int i = 0; i < RAND_STR_LEN; i++)
+      randStr[i] = letters[qrand() % qstrlen(letters)];
+    QString tmpDir = QDir::tempPath() + QString::fromLatin1("/") +
+        QString::fromLatin1(randStr);
A more Qt/C++ code for this would be:

static const int DirNameLength = 6;
QString dirName;
for (i = 0; i < DirNameLength; ++i) {
    dirName += QLatin1String(letters[qrand() % qstrlen(letters)]);
}
QString dirPath = QString(QLatin1String("%1/%2")).arg(QDir::tempPath()).arg(dirName));

+        cacheDir = QString::fromLatin1(qgetenv("HOME") + "/.cache");
This should be:
cacheDir = QString(QLatin1String("%1/.cache").arg(qgetenv("HOME")));

+    QString path = QString::fromLatin1("%1/telepathy/avatars/%2/%3").
+        arg(cacheDir).arg(connection->cmName()).arg(connection->protocolName());
should be:
QString path = QString(QLatin1String("%1/telepathy/avatars/%2/%3"))
    .arg(cacheDir).arg(connection->cmName()).arg(connection->protocolName());

In other words do not use QString::fromLatin1 anywhere.

Fixed all QString::fromLatin1() usage.

Branch merged in master!