[Bug 20034] Avatar cache reference implementation

[bugzilla-daemon at freedesktop.org]

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

--- Comment #7 from Xavier Claessens <xclaesse at gmail.com> 2010-09-07 07:15:58 PDT ---
(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

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