From dd6cf808d69eaa52f7642def5f6f94500ee9bc79 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 4 Aug 2020 17:12:56 +0200 Subject: User: optimise names/avatars storing and updating The current mechanism relied on a complicated and fragile machinery around setNameForRoom() and setAvatarForRoom() that maintained the "most used" entity for a given user along with "other" ones. Given that per-room avatars are pretty rare in Matrix, it's also been inefficient as kitsune-benchmark-set_ForRoom branch shows. The new mechanism stores the "default" (as per user profile) name and avatar and maintains a singleton map of avatar objects across all users. Per-user profile only (normally) exists for the local user so there's yet another inefficiency that will be fixed further down the road by introducing a separate user profile class. --- lib/room.cpp | 140 ++++++++++++++++-------------- lib/room.h | 1 + lib/user.cpp | 277 +++++++++++++---------------------------------------------- lib/user.h | 44 ++++------ 4 files changed, 152 insertions(+), 310 deletions(-) (limited to 'lib') diff --git a/lib/room.cpp b/lib/room.cpp index 970fe56b..ac9db318 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -192,7 +192,6 @@ public: // void inviteUser(User* u); // We might get it at some point in time. void insertMemberIntoMap(User* u); - void renameMember(User* u, const QString& oldName); void removeMemberFromMap(const QString& username, User* u); // This updates the room displayname field (which is the way a room @@ -238,6 +237,15 @@ public: return static_cast(evt); } +// template +// const auto& getCurrentStateContent(const QString& stateKey = {}) const +// { +// if (const auto* evt = +// currentState.value({ EventT::matrixTypeId(), stateKey }, nullptr)) +// return evt->content(); +// return EventT::content_type() +// } + bool isEventNotable(const TimelineItem& ti) const { return !ti->isRedacted() && ti->senderId() != connection->userId() @@ -1344,18 +1352,6 @@ void Room::Private::insertMemberIntoMap(User* u) emit q->memberRenamed(namesakes.front()); } -void Room::Private::renameMember(User* u, const QString& oldName) -{ - if (u->name(q) == oldName) { - qCWarning(MAIN) << "Room::Private::renameMember(): the user " - << u->fullName(q) - << "is already known in the room under a new name."; - } else if (membersMap.contains(oldName, u)) { - removeMemberFromMap(oldName, u); - insertMemberIntoMap(u); - } -} - void Room::Private::removeMemberFromMap(const QString& username, User* u) { User* namesake = nullptr; @@ -2396,14 +2392,66 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) if (!e.isStateEvent()) return Change::NoChange; - const auto* oldStateEvent = - std::exchange(d->currentState[{ e.matrixType(), e.stateKey() }], - static_cast(&e)); + // Find a value (create empty if necessary) and get a reference to it + // getCurrentState<> is not used here because it (creates and) returns + // a stub if a value is not found, and what's needed here is a "real" event + // or nullptr. + const auto*& curStateEvent = + d->currentState[{ e.matrixType(), e.stateKey() }]; + // Prepare for the state change + visit(e, [this, oldRme = static_cast(curStateEvent)]( + const RoomMemberEvent& rme) { + auto* u = user(rme.userId()); + if (!u) { // ??? + qCCritical(MAIN) + << "Could not get a user object for" << rme.userId(); + return; + } + const auto prevMembership = oldRme ? oldRme->membership() + : MembershipType::Leave; + switch (prevMembership) { + case MembershipType::Invite: + if (rme.membership() != prevMembership) { + d->usersInvited.removeOne(u); + Q_ASSERT(!d->usersInvited.contains(u)); + } + break; + case MembershipType::Join: + switch (rme.membership()) { + case MembershipType::Join: // rename/avatar change or no-op + if (rme.displayName() != oldRme->displayName()) { + emit memberAboutToRename(u, rme.displayName()); + d->removeMemberFromMap(u->name(this), u); + } + break; + case MembershipType::Invite: + qCWarning(MAIN) << "Membership change from Join to Invite:" + << rme; + [[fallthrough]]; + default: // whatever the new membership, it's no more Join + d->removeMemberFromMap(u->name(this), u); + emit userRemoved(u); + } + break; + default: + if (rme.membership() == MembershipType::Invite + || rme.membership() == MembershipType::Join) { + d->membersLeft.removeOne(u); + Q_ASSERT(!d->membersLeft.contains(u)); + } + } + }); + + // Change the state + const auto* const oldStateEvent = + std::exchange(curStateEvent, static_cast(&e)); Q_ASSERT(!oldStateEvent || (oldStateEvent->matrixType() == e.matrixType() && oldStateEvent->stateKey() == e.stateKey())); if (!is(e)) // Room member events are too numerous - qCDebug(STATE) << "Room state event:" << e; + qCDebug(STATE) << "Updated room state:" << e; + + // Update internal structures as per the change and work out the return value // clang-format off return visit(e @@ -2411,10 +2459,7 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) return NameChange; } , [] (const RoomAliasesEvent&) { - // clang-format on - // This event has been removed by MSC-2432 - return NoChange; - // clang-format off + return NoChange; // This event has been removed by MSC2432 } , [this, oldStateEvent] (const RoomCanonicalAliasEvent& cae) { // clang-format on @@ -2460,63 +2505,28 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) auto* u = user(evt.userId()); const auto* oldMemberEvent = static_cast(oldStateEvent); - u->processEvent(evt, this, oldMemberEvent == nullptr); const auto prevMembership = oldMemberEvent ? oldMemberEvent->membership() : MembershipType::Leave; - if (u == localUser() && evt.membership() == MembershipType::Invite - && evt.isDirect()) - connection()->addToDirectChats(this, user(evt.senderId())); - - switch (prevMembership) { - case MembershipType::Invite: - if (evt.membership() != prevMembership) { - d->usersInvited.removeOne(u); - Q_ASSERT(!d->usersInvited.contains(u)); - } - break; - case MembershipType::Join: - if (evt.membership() == MembershipType::Invite) - qCWarning(MAIN) << "Invalid membership change from " - "Join to Invite:" - << evt; - if (evt.membership() != prevMembership) { - disconnect(u, &User::nameAboutToChange, this, nullptr); - disconnect(u, &User::nameChanged, this, nullptr); - d->removeMemberFromMap(u->name(this), u); - emit userRemoved(u); - } - break; - default: - if (evt.membership() == MembershipType::Invite - || evt.membership() == MembershipType::Join) { - d->membersLeft.removeOne(u); - Q_ASSERT(!d->membersLeft.contains(u)); - } - } - switch (evt.membership()) { case MembershipType::Join: if (prevMembership != MembershipType::Join) { d->insertMemberIntoMap(u); - connect(u, &User::nameAboutToChange, this, - [=](QString newName, QString, const Room* context) { - if (context == this) - emit memberAboutToRename(u, newName); - }); - connect(u, &User::nameChanged, this, - [=](QString, QString oldName, const Room* context) { - if (context == this) { - d->renameMember(u, oldName); - emit memberRenamed(u); - } - }); emit userAdded(u); + } else { + if (oldMemberEvent->displayName() != evt.displayName()) { + d->insertMemberIntoMap(u); + emit memberRenamed(u); + } + if (oldMemberEvent->avatarUrl() != evt.avatarUrl()) + emit memberAvatarChanged(u); } break; case MembershipType::Invite: if (!d->usersInvited.contains(u)) d->usersInvited.push_back(u); + if (u == localUser() && evt.isDirect()) + connection()->addToDirectChats(this, user(evt.senderId())); break; default: if (!d->membersLeft.contains(u)) diff --git a/lib/room.h b/lib/room.h index 1ddff517..7fc03c19 100644 --- a/lib/room.h +++ b/lib/room.h @@ -652,6 +652,7 @@ signals: void userRemoved(Quotient::User* user); void memberAboutToRename(Quotient::User* user, QString newName); void memberRenamed(Quotient::User* user); + void memberAvatarChanged(Quotient::User* user); /// The list of members has changed /** Emitted no more than once per sync, this is a good signal to * for cases when some action should be done upon any change in diff --git a/lib/user.cpp b/lib/user.cpp index 09cf72a2..ffa4efb9 100644 --- a/lib/user.cpp +++ b/lib/user.cpp @@ -38,150 +38,26 @@ #include using namespace Quotient; -using namespace std::placeholders; using std::move; class User::Private { public: - static Avatar makeAvatar(QUrl url) { return Avatar(move(url)); } - Private(QString userId) : id(move(userId)), hueF(stringToHueF(id)) { } QString id; - - QString mostUsedName; - QMultiHash otherNames; qreal hueF; - Avatar mostUsedAvatar { makeAvatar({}) }; - std::vector otherAvatars; - auto otherAvatar(const QUrl& url) - { - return std::find_if(otherAvatars.begin(), otherAvatars.end(), - [&url](const auto& av) { return av.url() == url; }); - } - QMultiHash avatarsToRooms; - - mutable int totalRooms = 0; - QString nameForRoom(const Room* r, const QString& hint = {}) const; - void setNameForRoom(const Room* r, QString newName, const QString& oldName); - QUrl avatarUrlForRoom(const Room* r, const QUrl& hint = {}) const; - void setAvatarForRoom(const Room* r, const QUrl& newUrl, const QUrl& oldUrl); - - void setAvatarOnServer(QString contentUri, User* q); + QString defaultName; + Avatar defaultAvatar; + // NB: This container is ever-growing. Even if the user no more scrolls + // the timeline that far back, historical avatars are still kept around. + // This is consistent with the rest of Quotient, as room timelines + // are never vacuumed either. This will probably change in the future. + /// Map of mediaId to Avatar objects + static UnorderedMap otherAvatars; }; -QString User::Private::nameForRoom(const Room* r, const QString& hint) const -{ - // If the hint is accurate, this function is O(1) instead of O(n) - if (!hint.isNull() && (hint == mostUsedName || otherNames.contains(hint, r))) - return hint; - return otherNames.key(r, mostUsedName); -} - -static constexpr int MIN_JOINED_ROOMS_TO_LOG = 20; - -void User::Private::setNameForRoom(const Room* r, QString newName, - const QString& oldName) -{ - Q_ASSERT(oldName != newName); - Q_ASSERT(oldName == mostUsedName || otherNames.contains(oldName, r)); - if (totalRooms < 2) { - Q_ASSERT_X(totalRooms > 0 && otherNames.empty(), __FUNCTION__, - "Internal structures inconsistency"); - mostUsedName = move(newName); - return; - } - otherNames.remove(oldName, r); - if (newName != mostUsedName) { - // Check if the newName is about to become most used. - if (otherNames.count(newName) >= totalRooms - otherNames.size()) { - Q_ASSERT(totalRooms > 1); - QElapsedTimer et; - if (totalRooms > MIN_JOINED_ROOMS_TO_LOG) { - qCDebug(MAIN) << "Switching the most used name of user" << id - << "from" << mostUsedName << "to" << newName; - qCDebug(MAIN) << "The user is in" << totalRooms << "rooms"; - et.start(); - } - - for (auto* r1: r->connection()->allRooms()) - if (nameForRoom(r1) == mostUsedName) - otherNames.insert(mostUsedName, r1); - - mostUsedName = newName; - otherNames.remove(newName); - if (totalRooms > MIN_JOINED_ROOMS_TO_LOG) - qCDebug(PROFILER) << et << "to switch the most used name"; - } else - otherNames.insert(newName, r); - } -} - -QUrl User::Private::avatarUrlForRoom(const Room* r, const QUrl& hint) const -{ - // If the hint is accurate, this function is O(1) instead of O(n) - if (hint == mostUsedAvatar.url() || avatarsToRooms.contains(hint, r)) - return hint; - auto it = std::find(avatarsToRooms.begin(), avatarsToRooms.end(), r); - return it == avatarsToRooms.end() ? mostUsedAvatar.url() : it.key(); -} - -void User::Private::setAvatarForRoom(const Room* r, const QUrl& newUrl, - const QUrl& oldUrl) -{ - Q_ASSERT(oldUrl != newUrl); - Q_ASSERT(oldUrl == mostUsedAvatar.url() - || avatarsToRooms.contains(oldUrl, r)); - if (totalRooms < 2) { - Q_ASSERT_X(totalRooms > 0 && otherAvatars.empty(), __FUNCTION__, - "Internal structures inconsistency"); - mostUsedAvatar.updateUrl(newUrl); - return; - } - avatarsToRooms.remove(oldUrl, r); - if (!avatarsToRooms.contains(oldUrl)) { - auto it = otherAvatar(oldUrl); - if (it != otherAvatars.end()) - otherAvatars.erase(it); - } - if (newUrl != mostUsedAvatar.url()) { - // Check if the new avatar is about to become most used. - const auto newUrlUsage = avatarsToRooms.count(newUrl); - if (newUrlUsage >= totalRooms - avatarsToRooms.size()) { - QElapsedTimer et; - if (totalRooms > MIN_JOINED_ROOMS_TO_LOG) { - qCInfo(MAIN) - << "Switching the most used avatar of user" << id - << "from" << mostUsedAvatar.url().toDisplayString() << "to" - << newUrl.toDisplayString(); - et.start(); - } - avatarsToRooms.remove(newUrl); - auto nextMostUsedIt = otherAvatar(newUrl); - if (nextMostUsedIt == otherAvatars.end()) { - qCCritical(MAIN) - << id << "doesn't have" << newUrl.toDisplayString() - << "in otherAvatars though it seems to be used in" - << newUrlUsage << "rooms"; - Q_ASSERT(false); - otherAvatars.emplace_back(makeAvatar(newUrl)); - nextMostUsedIt = otherAvatars.end() - 1; - } - std::swap(mostUsedAvatar, *nextMostUsedIt); - for (const auto* r1: r->connection()->allRooms()) - if (avatarUrlForRoom(r1) == nextMostUsedIt->url()) - avatarsToRooms.insert(nextMostUsedIt->url(), r1); - - if (totalRooms > MIN_JOINED_ROOMS_TO_LOG) - qCDebug(PROFILER) << et << "to switch the most used avatar"; - } else { - if (otherAvatar(newUrl) == otherAvatars.end()) - otherAvatars.emplace_back(makeAvatar(newUrl)); - avatarsToRooms.insert(newUrl, r); - } - } -} +decltype(User::Private::otherAvatars) User::Private::otherAvatars {}; User::User(QString userId, Connection* connection) : QObject(connection), d(new Private(move(userId))) @@ -210,43 +86,31 @@ bool User::isGuest() const int User::hue() const { return int(hueF() * 359); } -QString User::name(const Room* room) const { return d->nameForRoom(room); } - -QString User::rawName(const Room* room) const { return name(room); } - -void User::updateName(const QString& newName, const Room* room) +QString User::name(const Room* room) const { - updateName(newName, d->nameForRoom(room), room); + return room ? room->getCurrentState(id())->displayName() + : d->defaultName; } -void User::updateName(const QString& newName, const QString& oldName, - const Room* room) -{ - Q_ASSERT(oldName == d->mostUsedName - || d->otherNames.contains(oldName, room)); - if (newName != oldName) { - emit nameAboutToChange(newName, oldName, room); - d->setNameForRoom(room, newName, oldName); - emit nameChanged(newName, oldName, room); - } -} - -void User::updateAvatarUrl(const QUrl& newUrl, const QUrl& oldUrl, - const Room* room) -{ - Q_ASSERT(oldUrl == d->mostUsedAvatar.url() - || d->avatarsToRooms.contains(oldUrl, room)); - if (newUrl != oldUrl) { - d->setAvatarForRoom(room, newUrl, oldUrl); - emit avatarChanged(this, room); - } -} +QString User::rawName(const Room* room) const { return name(room); } void User::rename(const QString& newName) { const auto actualNewName = sanitized(newName); + if (actualNewName == d->defaultName) + return; // Nothing to do + connect(connection()->callApi(id(), actualNewName), - &BaseJob::success, this, [=] { updateName(actualNewName); }); + &BaseJob::success, this, [this, actualNewName] { + // Check again, it could have changed meanwhile + if (actualNewName != d->defaultName) { + d->defaultName = actualNewName; + emit defaultNameChanged(); + } else + qCWarning(MAIN) + << "User" << id() << "already has profile name set to" + << actualNewName; + }); } void User::rename(const QString& newName, const Room* r) @@ -262,22 +126,37 @@ void User::rename(const QString& newName, const Room* r) const auto actualNewName = sanitized(newName); MemberEventContent evtC; evtC.displayName = actualNewName; - connect(r->setState(id(), move(evtC)), &BaseJob::success, - this, [=] { updateName(actualNewName, r); }); + r->setState(id(), move(evtC)); + // The state will be updated locally after it arrives with sync +} + +template +inline bool User::doSetAvatar(SourceT&& source) +{ + return d->defaultAvatar.upload( + connection(), source, [this](const QString& contentUri) { + auto* j = connection()->callApi(id(), contentUri); + connect(j, &BaseJob::success, this, + [this, newUrl = QUrl(contentUri)] { + if (newUrl == d->defaultAvatar.url()) { + d->defaultAvatar.updateUrl(move(newUrl)); + emit defaultAvatarChanged(); + } else + qCWarning(MAIN) << "User" << id() + << "already has avatar URL set to" + << newUrl.toDisplayString(); + }); + }); } bool User::setAvatar(const QString& fileName) { - return avatarObject().upload(connection(), fileName, - std::bind(&Private::setAvatarOnServer, - d.data(), _1, this)); + return doSetAvatar(fileName); } bool User::setAvatar(QIODevice* source) { - return avatarObject().upload(connection(), source, - std::bind(&Private::setAvatarOnServer, - d.data(), _1, this)); + return doSetAvatar(source); } void User::requestDirectChat() { connection()->requestDirectChat(this); } @@ -288,34 +167,29 @@ void User::unmarkIgnore() { connection()->removeFromIgnoredUsers(this); } bool User::isIgnored() const { return connection()->isIgnored(this); } -void User::Private::setAvatarOnServer(QString contentUri, User* q) -{ - auto* j = q->connection()->callApi(id, contentUri); - connect(j, &BaseJob::success, q, - [=] { q->updateAvatarUrl(contentUri, avatarUrlForRoom(nullptr)); }); -} - QString User::displayname(const Room* room) const { - if (room) - return room->roomMembername(this); - - const auto name = d->nameForRoom(nullptr); - return name.isEmpty() ? d->id : name; + return room ? room->roomMembername(this) + : d->defaultName.isEmpty() ? d->id + : d->defaultName; } QString User::fullName(const Room* room) const { - const auto name = d->nameForRoom(room); - return name.isEmpty() ? id() : name % " (" % id() % ')'; + const auto displayName = name(room); + return displayName.isEmpty() ? id() : (displayName % " (" % id() % ')'); } QString User::bridged() const { return {}; } const Avatar& User::avatarObject(const Room* room) const { - auto it = d->otherAvatar(d->avatarUrlForRoom(room)); - return it != d->otherAvatars.end() ? *it : d->mostUsedAvatar; + if (!room) + return d->defaultAvatar; + + const auto& url = room->getCurrentState(id())->avatarUrl(); + const auto& mediaId = url.authority() + url.path(); + return d->otherAvatars.try_emplace(mediaId, url).first->second; } QImage User::avatar(int dimension, const Room* room) @@ -331,10 +205,7 @@ QImage User::avatar(int width, int height, const Room* room) QImage User::avatar(int width, int height, const Room* room, const Avatar::get_callback_t& callback) { - return avatarObject(room).get(connection(), width, height, [=] { - emit avatarChanged(this, room); - callback(); - }); + return avatarObject(room).get(connection(), width, height, callback); } QString User::avatarMediaId(const Room* room) const @@ -347,30 +218,4 @@ QUrl User::avatarUrl(const Room* room) const return avatarObject(room).url(); } -void User::processEvent(const RoomMemberEvent& event, const Room* room, - bool firstMention) -{ - Q_ASSERT(room); - - if (firstMention) - ++d->totalRooms; - - if (event.membership() != MembershipType::Invite - && event.membership() != MembershipType::Join) - return; - - if (event.prevContent()) { - // FIXME: the hint doesn't work for bridged users - auto oldNameHint = d->nameForRoom(room, - event.prevContent()->displayName); - updateName(event.displayName(), oldNameHint, room); - updateAvatarUrl(event.avatarUrl(), - d->avatarUrlForRoom(room, event.prevContent()->avatarUrl), - room); - } else { - updateName(event.displayName(), room); - updateAvatarUrl(event.avatarUrl(), d->avatarUrlForRoom(room), room); - } -} - qreal User::hueF() const { return d->hueF; } diff --git a/lib/user.h b/lib/user.h index e4328f1d..a3b22480 100644 --- a/lib/user.h +++ b/lib/user.h @@ -33,13 +33,12 @@ class User : public QObject { Q_PROPERTY(bool isGuest READ isGuest CONSTANT) Q_PROPERTY(int hue READ hue CONSTANT) Q_PROPERTY(qreal hueF READ hueF CONSTANT) - Q_PROPERTY(QString name READ name NOTIFY nameChanged) - Q_PROPERTY(QString displayName READ displayname NOTIFY nameChanged STORED false) - Q_PROPERTY(QString fullName READ fullName NOTIFY nameChanged STORED false) - Q_PROPERTY(QString bridgeName READ bridged NOTIFY nameChanged STORED false) - Q_PROPERTY(QString avatarMediaId READ avatarMediaId NOTIFY avatarChanged - STORED false) - Q_PROPERTY(QUrl avatarUrl READ avatarUrl NOTIFY avatarChanged) + Q_PROPERTY(QString name READ name NOTIFY defaultNameChanged) + Q_PROPERTY(QString displayName READ displayname NOTIFY defaultNameChanged STORED false) + Q_PROPERTY(QString fullName READ fullName NOTIFY defaultNameChanged STORED false) + Q_PROPERTY(QString bridgeName READ bridged NOTIFY defaultNameChanged STORED false) + Q_PROPERTY(QString avatarMediaId READ avatarMediaId NOTIFY defaultAvatarChanged STORED false) + Q_PROPERTY(QUrl avatarUrl READ avatarUrl NOTIFY defaultAvatarChanged) public: User(QString userId, Connection* connection); ~User() override; @@ -123,20 +122,14 @@ public: QString avatarMediaId(const Room* room = nullptr) const; QUrl avatarUrl(const Room* room = nullptr) const; - /// This method is for internal use and should not be called - /// from client code - // FIXME: Move it away to private - void processEvent(const RoomMemberEvent& event, const Room* r, - bool firstMention); - public slots: - /** Set a new name in the global user profile */ + /// Set a new name in the global user profile void rename(const QString& newName); - /** Set a new name for the user in one room */ + /// Set a new name for the user in one room void rename(const QString& newName, const Room* r); - /** Upload the file and use it as an avatar */ + /// Upload the file and use it as an avatar bool setAvatar(const QString& fileName); - /** Upload contents of the QIODevice and set that as an avatar */ + /// Upload contents of the QIODevice and set that as an avatar bool setAvatar(QIODevice* source); /// Create or find a direct chat with this user /*! The resulting chat is returned asynchronously via @@ -151,21 +144,14 @@ public slots: bool isIgnored() const; signals: - void nameAboutToChange(QString newName, QString oldName, - const Quotient::Room* roomContext); - void nameChanged(QString newName, QString oldName, - const Quotient::Room* roomContext); - void avatarChanged(Quotient::User* user, const Quotient::Room* roomContext); - -private slots: - void updateName(const QString& newName, const Room* room = nullptr); - void updateName(const QString& newName, const QString& oldName, - const Room* room = nullptr); - void updateAvatarUrl(const QUrl& newUrl, const QUrl& oldUrl, - const Room* room = nullptr); + void defaultNameChanged(); + void defaultAvatarChanged(); private: class Private; QScopedPointer d; + + template + bool doSetAvatar(SourceT&& source); }; } // namespace Quotient -- cgit v1.2.3