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

Friday, 21 April 2017

Injeqt 1.2 and plans for 2.0

Injeqt 1.2 will be the last release of 1.x series. It includes one bugfix (for invalid test results in release mode) and one more feature - get_all_with_type_role method.

I do not have any ideas for new features for Injeqt 2.0. It may mean that it is feature-complete or that my use cases are very simple and not representative for wider audiences. However, for each thing I wanted to add to the library, I have come up with better (and simpler) solution that works best outside of library.

For example, I wanted a Repository feature to automatically add object that are subclasses of given class to another object. In Kadu that would be sublasses of Action added to repository named Actions. Core has a lot of them and most of plugins provides additional ones. After lots of thinking a InjectorRegisteredActions RAII class was created inside of Kadu that takes care of this without any kind of new class annotations.

Here it is:

InjectorRegisteredActions::InjectorRegisteredActions(
    Actions &actions,
    injeqt::injector &injector) : m_actions{actions}
{
    for (auto const &o : injector.get_all_with_type_role(ACTION))
    {
        auto action = qobject_cast<ActionDescription *>(o);
        if (action && m_actions.insert(action))
            m_registeredActions.push_back(action);
    }
}

InjectorRegisteredActions::~InjectorRegisteredActions()
{
    for (auto const &a : m_registeredActions)
        m_actions.remove(a);
}

It can be easily turned to template, and it will be as soon as need arise.

As I can think of any new features, Injeqt 2.0 will be more of API improvement release. There are things I need to do:

  • get rid of instantiate_all_with_type_role()
  • get rid of just-introduced get_all_with_type_role()
  • rename type role to tag
  • replace it with a types() method that will return all types with their tag information
  • then use all the data to implement instantiate_all_with_type_role() and get_all_with_type_role() outside of library or as free helper functions (with better names, of course)
  • get rid of version namespace, it is not needed as nobody will ever use it
  • check if ranges library will be useful (injeqt uses lots of containers and algorithms)