Fix Eventhandler strings for translation

Change the generic representations of events in event handler to always have a full string to aid translation.

The aggregated list is then converted to be a simple list of single event generic descriptions to avoid string puzzles.

Fixes network/neochat#638

BUG: 466201, BUG: 491024

autotests/eventhandlertest.cpp

@@ -213,11 +213,13 @@ void EventHandlerTest::genericBody_data()
     QTest::addColumn<int>("eventNum");
     QTest::addColumn<QString>("output");
 
-    QTest::newRow("message") << 0 << QStringLiteral("sent a message");
+    QTest::newRow("message") << 0 << QStringLiteral("<a href=\"https://matrix.to/#/@example:example.org\">after</a> sent a message");
-    QTest::newRow("member") << 1 << QStringLiteral("changed their display name and updated their avatar");
+    QTest::newRow("member") << 1
-    QTest::newRow("message 2") << 2 << QStringLiteral("sent a message");
+                            << QStringLiteral(
+                                   "<a href=\"https://matrix.to/#/@example:example.org\">after</a> changed their display name and updated their avatar");
+    QTest::newRow("message 2") << 2 << QStringLiteral("<a href=\"https://matrix.to/#/@example:example.org\">after</a> sent a message");
     QTest::newRow("reaction") << 3 << QStringLiteral("Unknown event");
-    QTest::newRow("video") << 4 << QStringLiteral("sent a message");
+    QTest::newRow("video") << 4 << QStringLiteral("<a href=\"https://matrix.to/#/@example:example.org\">after</a> sent a message");
 }
 
 void EventHandlerTest::genericBody()
@@ -225,13 +227,16 @@ void EventHandlerTest::genericBody()
     QFETCH(int, eventNum);
     QFETCH(QString, output);
 
-    QCOMPARE(EventHandler::genericBody(room->messageEvents().at(eventNum).get()), output);
+    QCOMPARE(EventHandler::genericBody(room, room->messageEvents().at(eventNum).get()), output);
 }
 
 void EventHandlerTest::nullGenericBody()
 {
+    QTest::ignoreMessage(QtWarningMsg, "genericBody called with room set to nullptr.");
+    QCOMPARE(EventHandler::genericBody(nullptr, nullptr), QString());
+
     QTest::ignoreMessage(QtWarningMsg, "genericBody called with event set to nullptr.");
-    QCOMPARE(EventHandler::genericBody(nullptr), QString());
+    QCOMPARE(EventHandler::genericBody(room, nullptr), QString());
 }
 
 void EventHandlerTest::markdownBody()

src/eventhandler.cpp

@@ -33,6 +33,21 @@
 
 using namespace Quotient;
 
+namespace
+{
+enum MemberChange {
+    None = 0,
+    AddName = 1,
+    Rename = 2,
+    RemoveName = 4,
+    AddAvatar = 8,
+    UpdateAvatar = 16,
+    RemoveAvatar = 32,
+};
+Q_DECLARE_FLAGS(MemberChanges, MemberChange)
+Q_DECLARE_OPERATORS_FOR_FLAGS(MemberChanges)
+};
+
 QString EventHandler::id(const Quotient::RoomEvent *event)
 {
     if (event == nullptr) {
@@ -482,8 +497,12 @@ QString EventHandler::getMessageBody(const NeoChatRoom *room, const RoomMessageE
     }
 }
 
-QString EventHandler::genericBody(const Quotient::RoomEvent *event)
+QString EventHandler::genericBody(const NeoChatRoom *room, const Quotient::RoomEvent *event)
 {
+    if (room == nullptr) {
+        qCWarning(EventHandling) << "genericBody called with room set to nullptr.";
+        return {};
+    }
     if (event == nullptr) {
         qCWarning(EventHandling) << "genericBody called with event set to nullptr.";
         return {};
@@ -492,123 +511,149 @@ QString EventHandler::genericBody(const Quotient::RoomEvent *event)
         return i18n("<i>[This message was deleted]</i>");
     }
 
+    const auto sender = room->member(event->senderId());
+    const auto senderString = QStringLiteral("<a href=\"https://matrix.to/#/%1\">%2</a>").arg(sender.id(), sender.htmlSafeDisplayName());
+
     return switchOnType(
         *event,
-        [](const RoomMessageEvent &e) {
+        [senderString](const RoomMessageEvent &) {
-            Q_UNUSED(e)
+            return i18n("%1 sent a message", senderString);
-            return i18n("sent a message");
         },
-        [](const StickerEvent &e) {
+        [senderString](const StickerEvent &) {
-            Q_UNUSED(e)
+            return i18n("%1 sent a sticker", senderString);
-            return i18n("sent a sticker");
         },
-        [](const RoomMemberEvent &e) {
+        [senderString](const RoomMemberEvent &e) {
             switch (e.membership()) {
             case Membership::Invite:
                 if (e.repeatsState()) {
-                    return i18n("reinvited someone to the room");
+                    return i18n("%1 reinvited someone to the room", senderString);
                 }
                 Q_FALLTHROUGH();
             case Membership::Join: {
-                QString text{};
                 // Part 1: invites and joins
                 if (e.repeatsState()) {
-                    text = i18n("joined the room (repeated)");
+                    return i18n("%1 joined the room (repeated)", senderString);
                 } else if (e.changesMembership()) {
-                    text = e.membership() == Membership::Invite ? i18n("invited someone to the room") : i18n("joined the room");
+                    return e.membership() == Membership::Invite ? i18n("%1 invited someone to the room", senderString)
-                }
+                                                                : i18n("%1 joined the room", senderString);
-                if (!text.isEmpty()) {
-                    return text;
                 }
+
                 // Part 2: profile changes of joined members
+                MemberChanges changes = None;
                 if (e.isRename()) {
                     if (!e.newDisplayName()) {
-                        text = i18nc("their refers to a singular user", "cleared their display name");
+                        changes |= RemoveName;
+                    } else if (!e.prevContent()->displayName) {
+                        changes |= AddName;
                     } else {
-                        text = i18nc("their refers to a singular user", "changed their display name");
+                        changes |= Rename;
                     }
                 }
                 if (e.isAvatarUpdate()) {
-                    if (!text.isEmpty()) {
-                        text += i18n(" and ");
-                    }
                     if (!e.newAvatarUrl()) {
-                        text += i18nc("their refers to a singular user", "cleared their avatar");
+                        changes |= RemoveAvatar;
                     } else if (!e.prevContent()->avatarUrl) {
-                        text += i18n("set an avatar");
+                        changes |= AddAvatar;
                     } else {
-                        text += i18nc("their refers to a singular user", "updated their avatar");
+                        changes |= UpdateAvatar;
                     }
                 }
-                if (text.isEmpty()) {
+
-                    text = i18nc("<user> changed nothing", "changed nothing");
+                if (changes.testFlag(AddName)) {
+                    if (changes.testFlag(AddAvatar)) {
+                        return i18n("%1 set a display name and set an avatar", senderString);
+                    } else if (changes.testFlag(UpdateAvatar)) {
+                        return i18n("%1 set a display name and updated their avatar", senderString);
+                    } else if (changes.testFlag(RemoveAvatar)) {
+                        return i18n("%1 set a display name and cleared their avatar", senderString);
+                    }
+                    return i18n("%1 set a display name for this room", senderString);
+                } else if (changes.testFlag(Rename)) {
+                    if (changes.testFlag(AddAvatar)) {
+                        return i18n("%1 changed their display name and set an avatar", senderString);
+                    } else if (changes.testFlag(UpdateAvatar)) {
+                        return i18n("%1 changed their display name and updated their avatar", senderString);
+                    } else if (changes.testFlag(RemoveAvatar)) {
+                        return i18n("%1 changed their display name and cleared their avatar", senderString);
+                    }
+                    return i18n("%1 changed their display name", senderString);
+                } else if (changes.testFlag(RemoveName)) {
+                    if (changes.testFlag(AddAvatar)) {
+                        return i18n("%1 cleared their display name and set an avatar", senderString);
+                    } else if (changes.testFlag(UpdateAvatar)) {
+                        return i18n("%1 cleared their display name and updated their avatar", senderString);
+                    } else if (changes.testFlag(RemoveAvatar)) {
+                        return i18n("%1 cleared their display name and cleared their avatar", senderString);
+                    }
+                    return i18n("%1 cleared their display name", senderString);
                 }
-                return text;
+
+                return i18nc("<user> changed nothing", "%1 changed nothing", senderString);
             }
             case Membership::Leave:
                 if (e.prevContent() && e.prevContent()->membership == Membership::Invite) {
-                    return (e.senderId() != e.userId()) ? i18n("withdrew a user's invitation") : i18n("rejected the invitation");
+                    return (e.senderId() != e.userId()) ? i18n("%1 withdrew a user's invitation", senderString)
+                                                        : i18n("%1 rejected the invitation", senderString);
                 }
 
                 if (e.prevContent() && e.prevContent()->membership == Membership::Ban) {
-                    return (e.senderId() != e.userId()) ? i18n("unbanned a user") : i18n("self-unbanned");
+                    return (e.senderId() != e.userId()) ? i18n("%1 unbanned a user", senderString) : i18n("%1 self-unbanned", senderString);
                 }
-                return (e.senderId() != e.userId()) ? i18n("put a user out of the room") : i18n("left the room");
+                return (e.senderId() != e.userId()) ? i18n("%1 put a user out of the room", senderString) : i18n("%1 left the room", senderString);
             case Membership::Ban:
                 if (e.senderId() != e.userId()) {
-                    return i18n("banned a user from the room");
+                    return i18n("%1 banned a user from the room", senderString);
                 } else {
-                    return i18n("self-banned from the room");
+                    return i18n("%1 self-banned from the room", senderString);
                 }
             case Membership::Knock: {
-                return i18n("requested an invite");
+                return i18n("%1 requested an invite", senderString);
             }
             default:;
             }
-            return i18n("made something unknown");
+            return i18n("%1 made something unknown", senderString);
         },
-        [](const RoomCanonicalAliasEvent &e) {
+        [senderString](const RoomCanonicalAliasEvent &e) {
-            return (e.alias().isEmpty()) ? i18n("cleared the room main alias") : i18n("set the room main alias");
+            return (e.alias().isEmpty()) ? i18n("%1 cleared the room main alias", senderString) : i18n("%1 set the room main alias", senderString);
         },
-        [](const RoomNameEvent &e) {
+        [senderString](const RoomNameEvent &e) {
-            return (e.name().isEmpty()) ? i18n("cleared the room name") : i18n("set the room name");
+            return (e.name().isEmpty()) ? i18n("%1 cleared the room name", senderString) : i18n("%1 set the room name", senderString);
         },
-        [](const RoomTopicEvent &e) {
+        [senderString](const RoomTopicEvent &e) {
-            return (e.topic().isEmpty()) ? i18n("cleared the topic") : i18n("set the topic");
+            return (e.topic().isEmpty()) ? i18n("%1 cleared the topic", senderString) : i18n("%1 set the topic", senderString);
         },
-        [](const RoomAvatarEvent &) {
+        [senderString](const RoomAvatarEvent &) {
-            return i18n("changed the room avatar");
+            return i18n("%1 changed the room avatar", senderString);
         },
-        [](const EncryptionEvent &) {
+        [senderString](const EncryptionEvent &) {
-            return i18n("activated End-to-End Encryption");
+            return i18n("%1 activated End-to-End Encryption", senderString);
         },
-        [](const RoomCreateEvent &e) {
+        [senderString](const RoomCreateEvent &e) {
-            return e.isUpgrade() ? i18n("upgraded the room version") : i18n("created the room");
+            return e.isUpgrade() ? i18n("%1 upgraded the room version", senderString) : i18n("%1 created the room", senderString);
         },
-        [](const RoomPowerLevelsEvent &) {
+        [senderString](const RoomPowerLevelsEvent &) {
-            return i18nc("'power level' means permission level", "changed the power levels for this room");
+            return i18nc("'power level' means permission level", "%1 changed the power levels for this room", senderString);
         },
-        [](const LocationBeaconEvent &) {
+        [senderString](const LocationBeaconEvent &) {
-            return i18n("sent a live location beacon");
+            return i18n("%1 sent a live location beacon", senderString);
         },
-        [](const RoomServerAclEvent &) {
+        [senderString](const RoomServerAclEvent &) {
-            return i18n("changed the server access control lists for this room");
+            return i18n("%1 changed the server access control lists for this room", senderString);
         },
-        [](const WidgetEvent &e) {
+        [senderString](const WidgetEvent &e) {
             if (e.fullJson()["unsigned"_ls]["prev_content"_ls].toObject().isEmpty()) {
-                return i18n("added a widget");
+                return i18n("%1 added a widget", senderString);
             }
             if (e.contentJson().isEmpty()) {
-                return i18n("removed a widget");
+                return i18n("%1 removed a widget", senderString);
             }
-            return i18n("configured a widget");
+            return i18n("%1 configured a widget", senderString);
         },
-        [](const StateEvent &) {
+        [senderString](const StateEvent &) {
-            return i18n("updated the state");
+            return i18n("%1 updated the state", senderString);
         },
-        [](const PollStartEvent &e) {
+        [senderString](const PollStartEvent &) {
-            Q_UNUSED(e);
+            return i18n("%1 started a poll", senderString);
-            return i18n("started a poll");
         },
         i18n("Unknown event"));
 }

src/eventhandler.h

@@ -192,7 +192,7 @@ public:
      *
      * @sa richBody(), plainBody()
      */
-    static QString genericBody(const Quotient::RoomEvent *event);
+    static QString genericBody(const NeoChatRoom *room, const Quotient::RoomEvent *event);
 
     /**
      * @brief Output a string for the event to be used as  a RoomList subtitle.

src/models/messageeventmodel.cpp

@@ -450,7 +450,7 @@ QVariant MessageEventModel::data(const QModelIndex &idx, int role) const
     }
 
     if (role == GenericDisplayRole) {
-        return EventHandler::genericBody(&evt);
+        return EventHandler::genericBody(m_currentRoom, &evt);
     }
 
     if (role == DelegateTypeRole) {

src/models/messagefiltermodel.cpp

@@ -133,14 +133,11 @@ bool MessageFilterModel::showAuthor(QModelIndex index) const
 
 QString MessageFilterModel::aggregateEventToString(int sourceRow) const
 {
-    QStringList parts;
+    QString aggregateString;
-    QVariantList uniqueAuthors;
     for (int i = sourceRow; i >= 0; i--) {
-        parts += sourceModel()->data(sourceModel()->index(i, 0), MessageEventModel::GenericDisplayRole).toString();
+        aggregateString += sourceModel()->data(sourceModel()->index(i, 0), MessageEventModel::GenericDisplayRole).toString();
+        aggregateString += ", "_ls;
         QVariant nextAuthor = sourceModel()->data(sourceModel()->index(i, 0), MessageEventModel::AuthorRole);
-        if (!uniqueAuthors.contains(nextAuthor)) {
-            uniqueAuthors.append(nextAuthor);
-        }
         if (i > 0
             && (sourceModel()->data(sourceModel()->index(i - 1, 0), MessageEventModel::DelegateTypeRole) != DelegateType::State // If it's not a state event
                 || sourceModel()->data(sourceModel()->index(i - 1, 0), MessageEventModel::ShowSectionRole).toBool() // or the section needs to be visible
@@ -148,46 +145,12 @@ QString MessageFilterModel::aggregateEventToString(int sourceRow) const
             break;
         }
     }
-    parts.sort(); // Sort them so that all identical events can be collected.
-    if (!parts.isEmpty()) {
-        QStringList chunks;
-        while (!parts.isEmpty()) {
-            chunks += QString();
-            int count = 1;
-            auto part = parts.takeFirst();
-            while (!parts.isEmpty() && parts.first() == part) {
-                parts.removeFirst();
-                count++;
-            }
-            chunks.last() += i18ncp("%1: What's being done; %2: How often it is done.", " %1", " %1 %2 times", part, count);
-        }
-        chunks.removeDuplicates();
-        // The author text is either "n users" if > 1 user or the matrix.to link to a single user.
-        QString userText = uniqueAuthors.length() > 1 ? i18ncp("n users", " %1 user ", " %1 users ", uniqueAuthors.length())
-                                                      : QStringLiteral("<a href=\"https://matrix.to/#/%1\">%3</a> ")
-                                                            .arg(uniqueAuthors[0].toMap()[QStringLiteral("id")].toString(),
-                                                                 uniqueAuthors[0].toMap()[QStringLiteral("displayName")].toString().toHtmlEscaped());
 
-        QString chunksText;
+    aggregateString = aggregateString.trimmed();
-        chunksText += chunks.takeFirst();
+    if (aggregateString.endsWith(u',')) {
-        if (chunks.size() > 0) {
+        aggregateString.removeLast();
-            while (chunks.size() > 1) {
-                chunksText += i18nc("[action 1], [action 2 and/or action 3]", ", ");
-                chunksText += chunks.takeFirst();
-            }
-            chunksText +=
-                uniqueAuthors.length() > 1 ? i18nc("[action 1, action 2] or [action 3]", " or ") : i18nc("[action 1, action 2] and [action 3]", " and ");
-            chunksText += chunks.takeFirst();
-        }
-        return i18nc(
-            "userText (%1) is either a Matrix username if a single user sent all the states or n users if they were sent by multiple users."
-            "chunksText (%2) is a list of comma separated actions for each of the state events in the group.",
-            "<style>a {text-decoration: none;}</style>%1 %2",
-            userText,
-            chunksText);
-    } else {
-        return {};
     }
+    return aggregateString;
 }
 
 QVariantList MessageFilterModel::stateEventsList(int sourceRow) const

src/timeline/StateDelegate.qml

@@ -147,10 +147,9 @@ TimelineDelegate {
                 Layout.fillWidth: true
                 visible: root.folded
 
-                text: `<style>a {color: ${Kirigami.Theme.textColor}}</style>` + root.aggregateDisplay
+                text: root.aggregateDisplay
-                elide: Qt.ElideRight
+                textFormat: TextEdit.StyledText
-                textFormat: Text.RichText
+                elide: Text.ElideRight
-                wrapMode: Text.WordWrap
                 onLinkActivated: RoomManager.resolveResource(link)
                 HoverHandler {
                     cursorShape: parent.hoveredLink ? Qt.PointingHandCursor : Qt.IBeamCursor