Bug 20034

Summary: Avatar cache reference implementation
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-qtAssignee: Xavier Claessens <xclaesse>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: low    
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/xclaesse/telepathy-qt4.git;a=shortlog;h=refs/heads/avatar-cache
Whiteboard:
i915 platform: i915 features:

Description Simon McVittie 2009-02-10 04:07:41 UTC
telepathy-qt4 and telepathy-glib should have a reference implementation of an avatar cache. For maximum synergy, they should share an on-disk format.
Comment 1 Olivier Le Thanh Duong 2009-03-17 17:35:37 UTC
This should include telepathy-python too
Comment 2 Simon McVittie 2009-03-18 05:09:45 UTC
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 :-)
Comment 3 Xavier Claessens 2010-09-03 04:41:29 UTC
Did the same API as we have in telepathy-glib
Comment 4 Andre Moreira Magalhaes 2010-09-03 05:06:06 UTC
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)
Comment 5 Xavier Claessens 2010-09-03 07:02:32 UTC
(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
Comment 6 Andre Moreira Magalhaes 2010-09-07 06:22:50 UTC
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
Comment 7 Xavier Claessens 2010-09-07 07:15:58 UTC
(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
Comment 8 Andre Moreira Magalhaes 2010-09-07 12:09:35 UTC
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 :/.
Comment 9 Xavier Claessens 2010-09-08 01:51:56 UTC
(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 :)
Comment 10 Andre Moreira Magalhaes 2010-09-08 03:56:13 UTC
+#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.
Comment 11 Xavier Claessens 2010-09-08 04:46:46 UTC
Fixed all QString::fromLatin1() usage.

Branch merged in master!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.