From 6c6b5b1bc18e16d0b40b674c8a48e0104ec73729 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 27 Feb 2017 16:40:03 +0900 Subject: Renamed logging_util.h to util.h and moved (improved) Owning<> and lookup() there Because these fall outside of SyncJob and Event context, respectively. In addition, Owning<> has gained a move assignment operator (because we have a move constructor) and assign() convenience method to take ownership over an existing container; also, Owning<>::release() is done the right way now (the previous version was copying the return value to a new container instead of releasing the old container). --- jobs/syncjob.cpp | 3 +-- jobs/syncjob.h | 33 +-------------------------------- 2 files changed, 2 insertions(+), 34 deletions(-) (limited to 'jobs') diff --git a/jobs/syncjob.cpp b/jobs/syncjob.cpp index 554ac0f7..cec9595f 100644 --- a/jobs/syncjob.cpp +++ b/jobs/syncjob.cpp @@ -96,8 +96,7 @@ BaseJob::Status SyncJob::parseJson(const QJsonDocument& data) void SyncRoomData::EventList::fromJson(const QJsonObject& roomContents) { - auto l = eventsFromJson(roomContents[jsonKey].toObject()["events"].toArray()); - swap(l); + assign(eventsFromJson(roomContents[jsonKey].toObject()["events"].toArray())); } SyncRoomData::SyncRoomData(QString roomId_, JoinState joinState_, const QJsonObject& room_) diff --git a/jobs/syncjob.h b/jobs/syncjob.h index be1d4776..b41c09d4 100644 --- a/jobs/syncjob.h +++ b/jobs/syncjob.h @@ -22,41 +22,10 @@ #include "../joinstate.h" #include "../events/event.h" +#include "util.h" namespace QMatrixClient { - /** - * @brief A crude wrapper around a container of pointers that owns pointers - * to contained objects - * - * Similar to vector>, upon deletion, EventsHolder - * will delete all events contained in it. - */ - template - class Owning : public ContainerT - { - public: - Owning() = default; -#if defined(_MSC_VER) && _MSC_VER < 1900 - // Workaround: Dangerous (auto_ptr style) copy constructor because - // VS2013 (unnecessarily) instantiates EventList::QVector<>::toList() - // which instantiates QList< Owning<> > which needs the contained - // object to have a non-deleted copy constructor. - Owning(Owning& other) : ContainerT(std::move(other)) { } -#else - Owning(Owning&) = delete; -#endif - Owning(Owning&& other) : ContainerT(std::move(other)) { } - ~Owning() { for (auto e: *this) delete e; } - - /** - * @brief returns the underlying events and releases the ownership - * - * Acts similar to unique_ptr::release. - */ - ContainerT release() { return std::move(*this); } - }; - class SyncRoomData { public: -- cgit v1.2.3 From 81b6d940218a212a37e3c600c66fbe9a7476f433 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 26 Feb 2017 19:35:09 +0900 Subject: Room: Make sure an event with the same id isn't added twice to the timeline; forbid empty event id's Added assertions and enhanced debug messages along the way --- jobs/roommessagesjob.cpp | 8 ++-- jobs/roommessagesjob.h | 2 +- room.cpp | 101 +++++++++++++++++++++++++++++++++++------------ room.h | 9 ++--- 4 files changed, 85 insertions(+), 35 deletions(-) (limited to 'jobs') diff --git a/jobs/roommessagesjob.cpp b/jobs/roommessagesjob.cpp index 1e53f601..7a0bc756 100644 --- a/jobs/roommessagesjob.cpp +++ b/jobs/roommessagesjob.cpp @@ -28,7 +28,7 @@ class RoomMessagesJob::Private public: Private() {} - Events events; + Owning events; QString end; }; @@ -49,9 +49,9 @@ RoomMessagesJob::~RoomMessagesJob() delete d; } -Events RoomMessagesJob::events() +Events RoomMessagesJob::releaseEvents() { - return d->events; + return d->events.release(); } QString RoomMessagesJob::end() @@ -62,7 +62,7 @@ QString RoomMessagesJob::end() BaseJob::Status RoomMessagesJob::parseJson(const QJsonDocument& data) { QJsonObject obj = data.object(); - d->events = eventsFromJson(obj.value("chunk").toArray()); + d->events.assign(eventsFromJson(obj.value("chunk").toArray())); d->end = obj.value("end").toString(); return Success; } diff --git a/jobs/roommessagesjob.h b/jobs/roommessagesjob.h index 348217cc..fd6a131d 100644 --- a/jobs/roommessagesjob.h +++ b/jobs/roommessagesjob.h @@ -35,7 +35,7 @@ namespace QMatrixClient FetchDirectory dir = FetchDirectory::Backwards, int limit=10); virtual ~RoomMessagesJob(); - Events events(); + Events releaseEvents(); QString end(); protected: diff --git a/room.cpp b/room.cpp index 9300056a..dd6e3942 100644 --- a/room.cpp +++ b/room.cpp @@ -95,15 +95,25 @@ class Room::Private void appendEvent(Event* e) { - timeline.push_back(e); - eventsIndex.insert(e->id(), timeline.crbegin()); - Q_ASSERT((*eventsIndex.value(e->id()))->id() == e->id()); + insertEvent(e, timeline.end()); } void prependEvent(Event* e) { - timeline.push_front(e); - eventsIndex.insert(e->id(), prev(timeline.crend())); - Q_ASSERT((*eventsIndex.value(e->id()))->id() == e->id()); + insertEvent(e, timeline.begin()); + } + + /** + * Removes events from the passed container that are already in the timeline + */ + void dropDuplicateEvents(Events& events) const + { + // Collect all duplicate events at the end of the container + auto dupsBegin = + std::stable_partition(events.begin(), events.end(), + [&] (Event* e) { return !eventsIndex.contains(e->id()); }); + // Dispose of those dups + std::for_each(dupsBegin, events.end(), [] (Event* e) { delete e; }); + events.erase(dupsBegin, events.end()); } rev_iter_t readMarker(const User* user) const @@ -120,6 +130,8 @@ class Room::Private void insertMemberIntoMap(User* u); void removeMemberFromMap(QString username, User* u); + + void insertEvent(Event* e, Timeline::iterator where); }; Room::Room(Connection* connection, QString id) @@ -198,23 +210,29 @@ void Room::setLastReadEvent(User* user, Event* event) std::pair Room::Private::promoteReadMarker(User* u, QString eventId) { - // If the eventId is empty, promote to the latest event; otherwise find the - // event by its id and position the marker at it. If the event is not found - // (most likely, because it's not fetched from the server), or it's older - // than the current marker position, keep the marker intact. - auto newMarker = eventId.isEmpty() ? timeline.crbegin() : - eventsIndex.value(eventId, timeline.crend()); - auto prevMarker = readMarker(u); - if (prevMarker <= newMarker) + Q_ASSERT_X (!eventId.isEmpty(), __FUNCTION__, + "Attempt to promote a read marker to an event with no id"); + + // Find the event by its id and position the marker at it. If the event + // is not found (most likely, because it's not fetched from the server), + // or if it's older than the current marker position, keep the marker intact. + const auto newMarker = eventsIndex.value(eventId, timeline.crend()); + const auto prevMarker = readMarker(u); + if (prevMarker <= newMarker) // Remember, we deal with reverse iterators return { prevMarker, prevMarker }; using namespace std; - // Try to auto-promote the read marker over the user's own messages. + // Try to auto-promote the read marker over the user's own messages + // (switch to direct iterators for that). auto eagerMarker = find_if(newMarker.base(), timeline.cend(), [=](Event* e) { return e->senderId() != u->id(); }); if (eagerMarker > timeline.begin()) + { + qDebug() << "Promoting the read marker in room" << displayname + << "for" << u->id() << "to" << (*(eagerMarker - 1))->id(); q->setLastReadEvent(u, *(eagerMarker - 1)); + } if (u == connection->user() && unreadMessages) { @@ -225,12 +243,13 @@ Room::Private::promoteReadMarker(User* u, QString eventId) if (stillUnreadMessagesCount == 0) { unreadMessages = false; - qDebug() << "Room" << q->displayName() << ": no more unread messages"; + qDebug() << "Room" << q->displayName() + << "has no more unread messages"; emit q->unreadMessagesChanged(q); } else - qDebug() << "Room" << q->displayName() - << ": still" << stillUnreadMessagesCount << "unread message(s)"; + qDebug() << "Room" << q->displayName() << "still has" + << stillUnreadMessagesCount << "unread message(s)"; } // Return newMarker, rather than eagerMarker, to save markMessagesAsRead() @@ -331,6 +350,26 @@ void Room::Private::removeMemberFromMap(QString username, User* u) updateDisplayname(); } +inline QByteArray makeErrorStr(Event* e, const char* msg) +{ + return QString("%1; event dump follows:\n%2") + .arg(msg, e->originalJson()).toUtf8(); +} + +void Room::Private::insertEvent(Event* e, Room::Timeline::iterator where) +{ + Q_ASSERT_X(e, __FUNCTION__, "Attempt to add nullptr to timeline"); + Q_ASSERT_X(!e->id().isEmpty(), __FUNCTION__, + makeErrorStr(e, + "Event with empty id cannot be in the timeline")); + Q_ASSERT_X(!eventsIndex.contains(e->id()), __FUNCTION__, + makeErrorStr(e, + "Event is already in the timeline (use dropDuplicateEvents())")); + Q_ASSERT(where >= timeline.begin() && where <= timeline.end()); + eventsIndex.insert(e->id(), rev_iter_t(timeline.insert(where, e) + 1)); + Q_ASSERT((*eventsIndex.value(e->id()))->id() == e->id()); +} + void Room::Private::addMember(User *u) { if (!hasMember(u)) @@ -463,7 +502,7 @@ void Room::Private::getPreviousContent() connect( roomMessagesJob, &RoomMessagesJob::result, [=]() { if( !roomMessagesJob->error() ) { - q->addHistoricalMessageEvents(roomMessagesJob->events()); + q->addHistoricalMessageEvents(roomMessagesJob->releaseEvents()); prevBatch = roomMessagesJob->end(); } roomMessagesJob = nullptr; @@ -476,8 +515,9 @@ Connection* Room::connection() const return d->connection; } -void Room::addNewMessageEvents(const Events& events) +void Room::addNewMessageEvents(Events events) { + d->dropDuplicateEvents(events); if (events.empty()) return; emit aboutToAddNewMessages(events); @@ -493,11 +533,12 @@ bool Room::Private::isEventNotable(const Event* e) const void Room::doAddNewMessageEvents(const Events& events) { - + Q_ASSERT(!events.isEmpty()); Timeline::size_type newUnreadMessages = 0; - // The first message in the batch defines whose read marker we can - // automatically promote any further. Others will need explicit read receipts + // The first event in the batch defines whose read marker we can + // automatically promote any further (if this author has read the whole + // timeline by then). Others will need explicit read receipts // from the server (or, for the local user, markMessagesAsRead() invocation) // to promote their read markers over the new message events. User* firstWriter = connection()->user(events.front()->senderId()); @@ -508,19 +549,26 @@ void Room::doAddNewMessageEvents(const Events& events) d->appendEvent(e); newUnreadMessages += d->isEventNotable(e); } + qDebug() << "Added" << events.size() + << "(with" << newUnreadMessages << "notable)" + << "new events to room" << displayName(); if (canAutoPromote) + { + qDebug() << "Auto-promoting" << firstWriter->id() << "over own events"; d->promoteReadMarker(firstWriter, events.front()->id()); + } if( !d->unreadMessages && newUnreadMessages > 0) { d->unreadMessages = true; emit unreadMessagesChanged(this); - qDebug() << "Room" << displayName() << ": unread messages"; + qDebug() << "Room" << displayName() << "has unread messages"; } } -void Room::addHistoricalMessageEvents(const Events& events) +void Room::addHistoricalMessageEvents(Events events) { + d->dropDuplicateEvents(events); if (events.empty()) return; emit aboutToAddHistoricalMessages(events); @@ -530,9 +578,12 @@ void Room::addHistoricalMessageEvents(const Events& events) void Room::doAddHistoricalMessageEvents(const Events& events) { + Q_ASSERT(!events.isEmpty()); // Historical messages arrive in newest-to-oldest order for (auto e: events) d->prependEvent(e); + qDebug() << "Added" << events.size() << "historical events to room" + << displayName(); } void Room::processStateEvents(const Events& events) diff --git a/room.h b/room.h index ff958680..60b59bb3 100644 --- a/room.h +++ b/room.h @@ -79,10 +79,9 @@ namespace QMatrixClient * Finds in the timeline and marks as read the event with * the specified id; also posts a read receipt to the server either * for this message or, if it's from the local user, for - * the nearest non-local message before. If the event id is empty, - * marks the whole timeline as read. + * the nearest non-local message before. uptoEventId must be non-empty. */ - Q_INVOKABLE void markMessagesAsRead(QString uptoEventId = {}); + Q_INVOKABLE void markMessagesAsRead(QString uptoEventId); Q_INVOKABLE bool hasUnreadMessages(); @@ -133,8 +132,8 @@ namespace QMatrixClient class Private; Private* d; - void addNewMessageEvents(const Events& events); - void addHistoricalMessageEvents(const Events& events); + void addNewMessageEvents(Events events); + void addHistoricalMessageEvents(Events events); void setLastReadEvent(User* user, Event* event); }; -- cgit v1.2.3