Saturday, 29 April 2017

Simplifying avatars

One of my task for 5.0 was to reimplement avatar support. Version from 4.0 was seemed to complicated for such a simple task as downloading bunch of files and uploading one from time to time.

First simplification was easy. Kadu 4.0 supported two modes of updating avatars. One required server to send a notification about avatar change for each contact. Second one pulled server for new avatars from time to time. It was not used for a long time, as implementation for server notifications for GaduGadu was added several releases ago. Removing that cleaned up a lot of unnecessary code that finished with not storing avatar data in configuration file (its size reduced by 75%!).

The only thing remaining for storage was avatar id attached to each contact (simple string different for each protocol - hash for XMPP, timestamp of last upload for Gadu-Gadu or full URL for Facebook).
Second simplification was making most of requests one way, without callbacks or returning values. Downloading avatars is not a critical job, so knowing when one failed is not important - it will be retried after next login or when server sends another notification.

So API for ContactAvatarService class is extremely simple now:
public:
  void download(const ContactAvatarId &id);
signals:
  void available(const ContactAvatarId &id);
  void downloaded(const ContactAvatarId &id, const QByteArray &content);
  void removed(const ContactId &id);
It is self-explanatory. After receiving available signal handler can ignore it (when given avatar is already locally cached) or call download. Action on downloaded signal is also obvious - store avatar in local cache and update property of Contact. That is it.

Third simplification was change of accessing avatar services for protocol. Most protocol services are (still) accesses by ProtocolHandler class attached to Account instances. The problem is that ProtocolHandler can change for Account if its plugin is reloaded (unlikely event, but has to be taken into account). It makes all code connecting to its signals complicated, as it must also listen to Account::protocolHandlerChanged. Too much indirection. In 5.0 there is one access point for all ContactAvatarService instances named AggregatedContactAvatarService. All ContactAvatarServices are registered there after creation. It has similar API to ContactAvatarService:
public:
  void bool download(const ContactAvatarGlobalId &id)
signals:
  void void available(const ContactAvatarGlobalId &id);
  void downloaded(const ContactAvatarGlobalId &id, const QByteArray &content);
  void removed(const ContactGlobalId &id);
Global part in parameters allows AggregatedContactAvatarService to select proper ContactAvatarService for calling download and forward its arguments there (if there is no service, false is returned). It also passes all signals from these services.

This change will be done for all services currently accesses by ProtocolHandler. Hopefully it will reduce references to ProtocolHandler class to only a few places in code.

In summary, doing these changes reduced avatars size by half, configuration size by three quarters, reduced avatar cache size and allowed for less bugs and easier to understand code with three simple steps:

  • remove what is obviously not needed (polling)
  • remove what turns out to be not needed after some thought (failure tracking)
  • reduce number of access points to one

No comments:

Post a Comment