Merge pull request #526 from quotient-im/kitsune/simpler-event-factories

Simplify event factories

7 files changed, 133 insertions, 130 deletions

diff --git a/lib/events/event.h b/lib/events/event.h
index 4d4bb16b..e786fb30 100644
--- a/lib/events/event.h
+++ b/lib/events/event.h
@@ -110,94 +110,90 @@ inline event_type_t typeId()
 
 inline event_type_t unknownEventTypeId() { return typeId<void>(); }
 
-// === EventFactory ===
+// === Event creation facilities ===
 
-/** Create an event of arbitrary type from its arguments */
+//! Create an event of arbitrary type from its arguments
 template <typename EventT, typename... ArgTs>
 inline event_ptr_tt<EventT> makeEvent(ArgTs&&... args)
 {
     return std::make_unique<EventT>(std::forward<ArgTs>(args)...);
 }
 
-template <typename BaseEventT>
-class EventFactory {
-public:
-    template <typename FnT>
-    static auto addMethod(FnT&& method)
-    {
-        factories().emplace_back(std::forward<FnT>(method));
-        return 0;
-    }
-
-    /** Chain two type factories
-     * Adds the factory class of EventT2 (EventT2::factory_t) to
-     * the list in factory class of EventT1 (EventT1::factory_t) so
-     * that when EventT1::factory_t::make() is invoked, types of
-     * EventT2 factory are looked through as well. This is used
-     * to include RoomEvent types into the more general Event factory,
-     * and state event types into the RoomEvent factory.
-     */
-    template <typename EventT>
-    static auto chainFactory()
-    {
-        return addMethod(&EventT::factory_t::make);
-    }
-
-    static event_ptr_tt<BaseEventT> make(const QJsonObject& json,
-                                         const QString& matrixType)
-    {
-        for (const auto& f : factories())
-            if (auto e = f(json, matrixType))
-                return e;
-        return nullptr;
-    }
-
-private:
-    static auto& factories()
+namespace _impl {
+    template <class EventT, class BaseEventT>
+    event_ptr_tt<BaseEventT> makeIfMatches(const QJsonObject& json,
+                                           const QString& matrixType)
     {
-        using inner_factory_tt = std::function<event_ptr_tt<BaseEventT>(
-            const QJsonObject&, const QString&)>;
-        static std::vector<inner_factory_tt> _factories {};
-        return _factories;
+        return QLatin1String(EventT::matrixTypeId()) == matrixType
+                   ? makeEvent<EventT>(json)
+                   : nullptr;
     }
-};
-
-/** Add a type to its default factory
- * Adds a standard factory method (via makeEvent<>) for a given
- * type to EventT::factory_t factory class so that it can be
- * created dynamically from loadEvent<>().
- *
- * \tparam EventT the type to enable dynamic creation of
- * \return the registered type id
- * \sa loadEvent, Event::type
- */
-template <typename EventT>
-inline auto setupFactory()
-{
-    qDebug(EVENTS) << "Adding factory method for" << EventT::matrixTypeId();
-    return EventT::factory_t::addMethod([](const QJsonObject& json,
-                                           const QString& jsonMatrixType) {
-        return EventT::matrixTypeId() == jsonMatrixType ? makeEvent<EventT>(json)
-                                                        : nullptr;
-    });
-}
 
-template <typename EventT>
-inline auto registerEventType()
-{
-    // Initialise exactly once, even if this function is called twice for
-    // the same type (for whatever reason - you never know the ways of
-    // static initialisation is done).
-    static const auto _ = setupFactory<EventT>();
-    return _; // Only to facilitate usage in static initialisation
-}
+    //! \brief A family of event factories to create events from CS API responses
+    //!
+    //! Each of these factories, as instantiated by event base types (Event,
+    //! RoomEvent etc.) is capable of producing an event object derived from
+    //! \p BaseEventT, using the JSON payload and the event type passed to its
+    //! make() method. Don't use these directly to make events; use loadEvent()
+    //! overloads as the frontend for these. Never instantiate new factories
+    //! outside of base event classes.
+    //! \sa loadEvent, setupFactory, Event::factory, RoomEvent::factory,
+    //!     StateEventBase::factory
+    template <typename BaseEventT>
+    class EventFactory
+        : private std::vector<event_ptr_tt<BaseEventT> (*)(const QJsonObject&,
+                                                           const QString&)> {
+        // Actual makeIfMatches specialisations will differ in the first
+        // template parameter but that doesn't affect the function type
+    public:
+        explicit EventFactory(const char* name = "")
+            : name(name)
+        {
+            static auto yetToBeConstructed = true;
+            Q_ASSERT(yetToBeConstructed);
+            if (!yetToBeConstructed) // For Release builds that pass Q_ASSERT
+                qCritical(EVENTS)
+                    << "Another EventFactory for the same base type is being "
+                       "created - event creation logic will be splintered";
+            yetToBeConstructed = false;
+        }
+        EventFactory(const EventFactory&) = delete;
+
+        //! \brief Add a method to create events of a given type
+        //!
+        //! Adds a standard factory method (makeIfMatches) for \p EventT so that
+        //! event objects of this type can be created dynamically by loadEvent.
+        //! The caller is responsible for ensuring this method is called only
+        //! once per type.
+        //! \sa makeIfMatches, loadEvent, Quotient::loadEvent
+        template <class EventT>
+        bool addMethod()
+        {
+            this->emplace_back(&makeIfMatches<EventT, BaseEventT>);
+            qDebug(EVENTS) << "Added factory method for"
+                           << EventT::matrixTypeId() << "events;" << this->size()
+                           << "methods in the" << name << "chain by now";
+            return true;
+        }
+
+        auto loadEvent(const QJsonObject& json, const QString& matrixType)
+        {
+            for (const auto& f : *this)
+                if (auto e = f(json, matrixType))
+                    return e;
+            return makeEvent<BaseEventT>(unknownEventTypeId(), json);
+        }
+
+        const char* const name;
+    };
+} // namespace _impl
 
 // === Event ===
 
 class Event {
 public:
     using Type = event_type_t;
-    using factory_t = EventFactory<Event>;
+    static inline _impl::EventFactory<Event> factory { "Event" };
 
     explicit Event(Type type, const QJsonObject& json);
     explicit Event(Type type, event_mtype_t matrixType,
@@ -272,7 +268,7 @@ template <typename EventT>
 using EventsArray = std::vector<event_ptr_tt<EventT>>;
 using Events = EventsArray<Event>;
 
-// === Macros used with event class definitions ===
+// === Facilities for event class definitions ===
 
 // This macro should be used in a public section of an event class to
 // provide matrixTypeId() and typeId().
@@ -284,13 +280,27 @@ using Events = EventsArray<Event>;
 // This macro should be put after an event class definition (in .h or .cpp)
 // to enable its deserialisation from a /sync and other
 // polymorphic event arrays
-#define REGISTER_EVENT_TYPE(_Type)                                \
-    namespace {                                                   \
-        [[maybe_unused]] static const auto _factoryAdded##_Type = \
-            registerEventType<_Type>();                           \
-    }                                                             \
+#define REGISTER_EVENT_TYPE(_Type)                            \
+    [[maybe_unused]] inline const auto _factoryAdded##_Type = \
+        _Type::factory.addMethod<_Type>();                    \
     // End of macro
 
+// === Event loading ===
+// (see also event_loader.h)
+
+//! \brief Point of customisation to dynamically load events
+//!
+//! The default specialisation of this calls BaseEventT::factory and if that
+//! fails (i.e. returns nullptr) creates an unknown event of BaseEventT.
+//! Other specialisations may reuse other factories, add validations common to
+//! BaseEventT, and so on
+template <class BaseEventT>
+event_ptr_tt<BaseEventT> doLoadEvent(const QJsonObject& json,
+                                     const QString& matrixType)
+{
+    return BaseEventT::factory.loadEvent(json, matrixType);
+}
+
 // === is<>(), eventCast<>() and switchOnType<>() ===
 
 template <class EventT>
diff --git a/lib/events/eventloader.h b/lib/events/eventloader.h
index 978668f2..fe624d70 100644
--- a/lib/events/eventloader.h
+++ b/lib/events/eventloader.h
@@ -6,16 +6,6 @@
 #include "stateevent.h"
 
 namespace Quotient {
-namespace _impl {
-    template <typename BaseEventT>
-    static inline auto loadEvent(const QJsonObject& json,
-                                 const QString& matrixType)
-    {
-        if (auto e = EventFactory<BaseEventT>::make(json, matrixType))
-            return e;
-        return makeEvent<BaseEventT>(unknownEventTypeId(), json);
-    }
-} // namespace _impl
 
 /*! Create an event with proper type from a JSON object
  *
@@ -26,7 +16,7 @@ namespace _impl {
 template <typename BaseEventT>
 inline event_ptr_tt<BaseEventT> loadEvent(const QJsonObject& fullJson)
 {
-    return _impl::loadEvent<BaseEventT>(fullJson, fullJson[TypeKeyL].toString());
+    return doLoadEvent<BaseEventT>(fullJson, fullJson[TypeKeyL].toString());
 }
 
 /*! Create an event from a type string and content JSON
@@ -39,8 +29,8 @@ template <typename BaseEventT>
 inline event_ptr_tt<BaseEventT> loadEvent(const QString& matrixType,
                                           const QJsonObject& content)
 {
-    return _impl::loadEvent<BaseEventT>(basicEventJson(matrixType, content),
-                                        matrixType);
+    return doLoadEvent<BaseEventT>(basicEventJson(matrixType, content),
+                                   matrixType);
 }
 
 /*! Create a state event from a type string, content JSON and state key
@@ -53,7 +43,7 @@ inline StateEventPtr loadStateEvent(const QString& matrixType,
                                     const QJsonObject& content,
                                     const QString& stateKey = {})
 {
-    return _impl::loadEvent<StateEventBase>(
+    return doLoadEvent<StateEventBase>(
         basicStateEventJson(matrixType, content, stateKey), matrixType);
 }
 
diff --git a/lib/events/roomevent.cpp b/lib/events/roomevent.cpp
index fb921af6..b728e0bf 100644
--- a/lib/events/roomevent.cpp
+++ b/lib/events/roomevent.cpp
@@ -9,9 +9,6 @@
 
 using namespace Quotient;
 
-[[maybe_unused]] static auto roomEventTypeInitialised =
-    Event::factory_t::chainFactory<RoomEvent>();
-
 RoomEvent::RoomEvent(Type type, event_mtype_t matrixType,
                      const QJsonObject& contentJson)
     : Event(type, matrixType, contentJson)
diff --git a/lib/events/roomevent.h b/lib/events/roomevent.h
index 7f13f6f2..8be58481 100644
--- a/lib/events/roomevent.h
+++ b/lib/events/roomevent.h
@@ -13,7 +13,7 @@ class RedactionEvent;
 /** This class corresponds to m.room.* events */
 class RoomEvent : public Event {
 public:
-    using factory_t = EventFactory<RoomEvent>;
+    static inline _impl::EventFactory<RoomEvent> factory { "RoomEvent" };
 
     // RedactionEvent is an incomplete type here so we cannot inline
     // constructors and destructors and we cannot use 'using'.
diff --git a/lib/events/roommemberevent.h b/lib/events/roommemberevent.h
index f3047159..0fb464d4 100644
--- a/lib/events/roommemberevent.h
+++ b/lib/events/roommemberevent.h
@@ -49,16 +49,15 @@ public:
                      std::forward<ArgTs>(contentArgs)...)
     {}
 
-    /// A special constructor to create unknown RoomMemberEvents
-    /**
-     * This is needed in order to use RoomMemberEvent as a "base event
-     * class" in cases like GetMembersByRoomJob when RoomMemberEvents
-     * (rather than RoomEvents or StateEvents) are resolved from JSON.
-     * For such cases loadEvent<> requires an underlying class to be
-     * constructible with unknownTypeId() instead of its genuine id.
-     * Don't use it directly.
-     * \sa GetMembersByRoomJob, loadEvent, unknownTypeId
-     */
+    //! \brief A special constructor to create unknown RoomMemberEvents
+    //!
+    //! This is needed in order to use RoomMemberEvent as a "base event class"
+    //! in cases like GetMembersByRoomJob when RoomMemberEvents (rather than
+    //! RoomEvents or StateEvents) are resolved from JSON. For such cases
+    //! loadEvent\<> requires an underlying class to have a specialisation of
+    //! EventFactory\<> and be constructible with unknownTypeId() instead of
+    //! its genuine id. Don't use directly.
+    //! \sa EventFactory, loadEvent, GetMembersByRoomJob
     RoomMemberEvent(Type type, const QJsonObject& fullJson)
         : StateEvent(type, fullJson)
     {}
@@ -89,14 +88,13 @@ public:
 };
 
 template <>
-class EventFactory<RoomMemberEvent> {
-public:
-    static event_ptr_tt<RoomMemberEvent> make(const QJsonObject& json,
-                                              const QString&)
-    {
+inline event_ptr_tt<RoomMemberEvent>
+doLoadEvent<RoomMemberEvent>(const QJsonObject& json, const QString& matrixType)
+{
+    if (matrixType == QLatin1String(RoomMemberEvent::matrixTypeId()))
         return makeEvent<RoomMemberEvent>(json);
-    }
-};
+    return makeEvent<RoomMemberEvent>(unknownEventTypeId(), json);
+}
 
 REGISTER_EVENT_TYPE(RoomMemberEvent)
 } // namespace Quotient
diff --git a/lib/events/stateevent.cpp b/lib/events/stateevent.cpp
index efe011a0..e53d47d4 100644
--- a/lib/events/stateevent.cpp
+++ b/lib/events/stateevent.cpp
@@ -5,20 +5,13 @@
 
 using namespace Quotient;
 
-// Aside from the normal factory to instantiate StateEventBase inheritors
-// StateEventBase itself can be instantiated if there's a state_key JSON key
-// but the event type is unknown.
-[[maybe_unused]] static auto stateEventTypeInitialised =
-    RoomEvent::factory_t::addMethod(
-        [](const QJsonObject& json, const QString& matrixType) -> StateEventPtr {
-            if (!json.contains(StateKeyKeyL))
-                return nullptr;
-
-            if (auto e = StateEventBase::factory_t::make(json, matrixType))
-                return e;
-
-            return makeEvent<StateEventBase>(unknownEventTypeId(), json);
-        });
+StateEventBase::StateEventBase(Type type, const QJsonObject& json)
+    : RoomEvent(json.contains(StateKeyKeyL) ? type : unknownEventTypeId(), json)
+{
+    if (Event::type() == unknownEventTypeId() && !json.contains(StateKeyKeyL))
+        qWarning(EVENTS) << "Attempt to create a state event with no stateKey -"
+                            "forcing the event type to unknown to avoid damage";
+}
 
 StateEventBase::StateEventBase(Event::Type type, event_mtype_t matrixType,
                                const QString& stateKey,
diff --git a/lib/events/stateevent.h b/lib/events/stateevent.h
index b0aa9907..c37965aa 100644
--- a/lib/events/stateevent.h
+++ b/lib/events/stateevent.h
@@ -19,10 +19,9 @@ inline QJsonObject basicStateEventJson(const QString& matrixTypeId,
 
 class StateEventBase : public RoomEvent {
 public:
-    using factory_t = EventFactory<StateEventBase>;
+    static inline _impl::EventFactory<StateEventBase> factory { "StateEvent" };
 
-    StateEventBase(Type type, const QJsonObject& json) : RoomEvent(type, json)
-    {}
+    StateEventBase(Type type, const QJsonObject& json);
     StateEventBase(Type type, event_mtype_t matrixType,
                    const QString& stateKey = {},
                    const QJsonObject& contentJson = {});
@@ -37,6 +36,22 @@ public:
 using StateEventPtr = event_ptr_tt<StateEventBase>;
 using StateEvents = EventsArray<StateEventBase>;
 
+//! \brief Override RoomEvent factory with that from StateEventBase if JSON has
+//! stateKey
+//!
+//! This means in particular that an event with a type known to RoomEvent but
+//! having stateKey set (even to an empty value) will be treated as a state
+//! event and most likely end up as unknown (consider, e.g., m.room.message
+//! that has stateKey set).
+template <>
+inline RoomEventPtr doLoadEvent(const QJsonObject& json,
+                                const QString& matrixType)
+{
+    if (json.contains(StateKeyKeyL))
+        return StateEventBase::factory.loadEvent(json, matrixType);
+    return RoomEvent::factory.loadEvent(json, matrixType);
+}
+
 template <>
 inline bool is<StateEventBase>(const Event& e)
 {