From 67dfc7b32e4d8747b1e5e419cbdf0b8f7951f34d Mon Sep 17 00:00:00 2001 From: James Graham Date: Fri, 13 Sep 2024 17:11:50 +0000 Subject: [PATCH] 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 | 17 +-- src/eventhandler.cpp | 165 +++++++++++++++++++----------- src/eventhandler.h | 2 +- src/models/messageeventmodel.cpp | 2 +- src/models/messagefiltermodel.cpp | 51 ++------- src/timeline/StateDelegate.qml | 7 +- 6 files changed, 128 insertions(+), 116 deletions(-) diff --git a/autotests/eventhandlertest.cpp b/autotests/eventhandlertest.cpp index d7d8c2990..225a89934 100644 --- a/autotests/eventhandlertest.cpp +++ b/autotests/eventhandlertest.cpp @@ -213,11 +213,13 @@ void EventHandlerTest::genericBody_data() QTest::addColumn("eventNum"); QTest::addColumn("output"); - QTest::newRow("message") << 0 << QStringLiteral("sent a message"); - QTest::newRow("member") << 1 << QStringLiteral("changed their display name and updated their avatar"); - QTest::newRow("message 2") << 2 << QStringLiteral("sent a message"); + QTest::newRow("message") << 0 << QStringLiteral("after sent a message"); + QTest::newRow("member") << 1 + << QStringLiteral( + "after changed their display name and updated their avatar"); + QTest::newRow("message 2") << 2 << QStringLiteral("after sent a message"); QTest::newRow("reaction") << 3 << QStringLiteral("Unknown event"); - QTest::newRow("video") << 4 << QStringLiteral("sent a message"); + QTest::newRow("video") << 4 << QStringLiteral("after 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() diff --git a/src/eventhandler.cpp b/src/eventhandler.cpp index fd912c7c4..7d7217634 100644 --- a/src/eventhandler.cpp +++ b/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("[This message was deleted]"); } + const auto sender = room->member(event->senderId()); + const auto senderString = QStringLiteral("%2").arg(sender.id(), sender.htmlSafeDisplayName()); + return switchOnType( *event, - [](const RoomMessageEvent &e) { - Q_UNUSED(e) - return i18n("sent a message"); + [senderString](const RoomMessageEvent &) { + return i18n("%1 sent a message", senderString); }, - [](const StickerEvent &e) { - Q_UNUSED(e) - return i18n("sent a sticker"); + [senderString](const StickerEvent &) { + return i18n("%1 sent a sticker", senderString); }, - [](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"); - } - if (!text.isEmpty()) { - return text; + return e.membership() == Membership::Invite ? i18n("%1 invited someone to the room", senderString) + : i18n("%1 joined the room", senderString); } + // 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(" 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(" 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) { - return (e.alias().isEmpty()) ? i18n("cleared the room main alias") : i18n("set the room main alias"); + [senderString](const RoomCanonicalAliasEvent &e) { + return (e.alias().isEmpty()) ? i18n("%1 cleared the room main alias", senderString) : i18n("%1 set the room main alias", senderString); }, - [](const RoomNameEvent &e) { - return (e.name().isEmpty()) ? i18n("cleared the room name") : i18n("set the room name"); + [senderString](const RoomNameEvent &e) { + return (e.name().isEmpty()) ? i18n("%1 cleared the room name", senderString) : i18n("%1 set the room name", senderString); }, - [](const RoomTopicEvent &e) { - return (e.topic().isEmpty()) ? i18n("cleared the topic") : i18n("set the topic"); + [senderString](const RoomTopicEvent &e) { + return (e.topic().isEmpty()) ? i18n("%1 cleared the topic", senderString) : i18n("%1 set the topic", senderString); }, - [](const RoomAvatarEvent &) { - return i18n("changed the room avatar"); + [senderString](const RoomAvatarEvent &) { + return i18n("%1 changed the room avatar", senderString); }, - [](const EncryptionEvent &) { - return i18n("activated End-to-End Encryption"); + [senderString](const EncryptionEvent &) { + return i18n("%1 activated End-to-End Encryption", senderString); }, - [](const RoomCreateEvent &e) { - return e.isUpgrade() ? i18n("upgraded the room version") : i18n("created the room"); + [senderString](const RoomCreateEvent &e) { + return e.isUpgrade() ? i18n("%1 upgraded the room version", senderString) : i18n("%1 created the room", senderString); }, - [](const RoomPowerLevelsEvent &) { - return i18nc("'power level' means permission level", "changed the power levels for this room"); + [senderString](const RoomPowerLevelsEvent &) { + return i18nc("'power level' means permission level", "%1 changed the power levels for this room", senderString); }, - [](const LocationBeaconEvent &) { - return i18n("sent a live location beacon"); + [senderString](const LocationBeaconEvent &) { + return i18n("%1 sent a live location beacon", senderString); }, - [](const RoomServerAclEvent &) { - return i18n("changed the server access control lists for this room"); + [senderString](const RoomServerAclEvent &) { + 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 &) { - return i18n("updated the state"); + [senderString](const StateEvent &) { + return i18n("%1 updated the state", senderString); }, - [](const PollStartEvent &e) { - Q_UNUSED(e); - return i18n("started a poll"); + [senderString](const PollStartEvent &) { + return i18n("%1 started a poll", senderString); }, i18n("Unknown event")); } diff --git a/src/eventhandler.h b/src/eventhandler.h index 0184f7670..014947c58 100644 --- a/src/eventhandler.h +++ b/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. diff --git a/src/models/messageeventmodel.cpp b/src/models/messageeventmodel.cpp index f90170c9e..2d568e3ca 100644 --- a/src/models/messageeventmodel.cpp +++ b/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) { diff --git a/src/models/messagefiltermodel.cpp b/src/models/messagefiltermodel.cpp index babd35b21..ce07f041c 100644 --- a/src/models/messagefiltermodel.cpp +++ b/src/models/messagefiltermodel.cpp @@ -133,14 +133,11 @@ bool MessageFilterModel::showAuthor(QModelIndex index) const QString MessageFilterModel::aggregateEventToString(int sourceRow) const { - QStringList parts; - QVariantList uniqueAuthors; + QString aggregateString; 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("%3 ") - .arg(uniqueAuthors[0].toMap()[QStringLiteral("id")].toString(), - uniqueAuthors[0].toMap()[QStringLiteral("displayName")].toString().toHtmlEscaped()); - QString chunksText; - chunksText += chunks.takeFirst(); - if (chunks.size() > 0) { - 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.", - "%1 %2", - userText, - chunksText); - } else { - return {}; + aggregateString = aggregateString.trimmed(); + if (aggregateString.endsWith(u',')) { + aggregateString.removeLast(); } + return aggregateString; } QVariantList MessageFilterModel::stateEventsList(int sourceRow) const diff --git a/src/timeline/StateDelegate.qml b/src/timeline/StateDelegate.qml index b27090f67..2db968d71 100644 --- a/src/timeline/StateDelegate.qml +++ b/src/timeline/StateDelegate.qml @@ -147,10 +147,9 @@ TimelineDelegate { Layout.fillWidth: true visible: root.folded - text: `` + root.aggregateDisplay - elide: Qt.ElideRight - textFormat: Text.RichText - wrapMode: Text.WordWrap + text: root.aggregateDisplay + textFormat: TextEdit.StyledText + elide: Text.ElideRight onLinkActivated: RoomManager.resolveResource(link) HoverHandler { cursorShape: parent.hoveredLink ? Qt.PointingHandCursor : Qt.IBeamCursor