From c254572e808f5843e6c8b7219045ab9e293d4980 Mon Sep 17 00:00:00 2001 From: Camila Date: Tue, 29 Mar 2022 13:33:06 +0200 Subject: [PATCH 1/6] Add 'Reply' as activity link instead of changing the 'View chat' link. - Align 'Reply' button. - Do not display 'View chat' as a link: the 'View chat' action is active when the user clickcs on the notification item in the tray window. - Convert the QByteArray verb value to String. - Handle logic to toggle the reply textfield in the QML code. Signed-off-by: Camila --- src/gui/tray/ActivityItem.qml | 13 +++++++--- src/gui/tray/ActivityItemActions.qml | 9 +++++-- src/gui/tray/ActivityItemContent.qml | 16 ++++++++++-- src/gui/tray/TalkReplyTextField.qml | 11 ++------ src/gui/tray/activitydata.h | 1 - src/gui/tray/activitylistmodel.cpp | 38 ++++------------------------ src/gui/tray/activitylistmodel.h | 8 ++---- src/gui/tray/notificationhandler.cpp | 14 +++++++--- 8 files changed, 50 insertions(+), 60 deletions(-) diff --git a/src/gui/tray/ActivityItem.qml b/src/gui/tray/ActivityItem.qml index 46c353f945..3003ae667d 100644 --- a/src/gui/tray/ActivityItem.qml +++ b/src/gui/tray/ActivityItem.qml @@ -12,8 +12,9 @@ MouseArea { property bool isFileActivityList: false - property bool isChatActivity: model.objectType === "chat" || model.objectType === "room" || model.objectType === "call" - property bool isTalkReplyPossible: model.conversationToken !== "" + readonly property bool isChatActivity: model.objectType === "chat" || model.objectType === "room" || model.objectType === "call" + readonly property bool isTalkReplyPossible: model.conversationToken !== "" + property bool isTalkReplyOptionVisible: model.messageSent !== "" signal fileActivityButtonClicked(string absolutePath) @@ -26,6 +27,10 @@ MouseArea { Accessible.name: (model.path !== "" && model.displayPath !== "") ? qsTr("Open %1 locally").arg(model.displayPath) : model.message Accessible.onPressAction: root.clicked() + function showReplyOptions(activityIndex) { + isTalkReplyOptionVisible = !isTalkReplyOptionVisible + } + Rectangle { id: activityHover anchors.fill: parent @@ -73,11 +78,11 @@ MouseArea { ActivityItemActions { id: activityActions - visible: !root.isFileActivityList && model.linksForActionButtons.length > 0 && !model.displayReplyOption + visible: !root.isFileActivityList && model.linksForActionButtons.length > 0 && !isTalkReplyOptionVisible Layout.preferredHeight: Style.trayWindowHeaderHeight * 0.85 Layout.fillWidth: true - Layout.leftMargin: 40 + Layout.leftMargin: 60 Layout.bottomMargin: model.links.length > 1 ? 5 : 0 displayActions: model.displayActions diff --git a/src/gui/tray/ActivityItemActions.qml b/src/gui/tray/ActivityItemActions.qml index 3f14fbd4d4..cdf7ff70f9 100644 --- a/src/gui/tray/ActivityItemActions.qml +++ b/src/gui/tray/ActivityItemActions.qml @@ -31,7 +31,12 @@ RowLayout { ActivityActionButton { id: activityActionButton - readonly property bool primary: model.index === 0 && model.modelData.verb !== "DELETE" + readonly property string verb: model.modelData.verb + readonly property bool primary: model.index === 0 && verb !== "DELETE" + readonly property bool isViewChatlink: verb === "WEB" + readonly property bool isTalkReplyButton: verb === "REPLY" + + visible: !isViewChatlink Layout.minimumWidth: primary ? Style.activityItemActionPrimaryButtonMinWidth : Style.activityItemActionSecondaryButtonMinWidth Layout.preferredHeight: primary ? parent.height : parent.height * 0.3 @@ -48,7 +53,7 @@ RowLayout { bold: primary - onClicked: root.triggerAction(model.index) + onClicked: !isTalkReplyButton? root.triggerAction(model.index) : showReplyOptions(model.index) } } diff --git a/src/gui/tray/ActivityItemContent.qml b/src/gui/tray/ActivityItemContent.qml index f18244e561..7b49e07bdc 100644 --- a/src/gui/tray/ActivityItemContent.qml +++ b/src/gui/tray/ActivityItemContent.qml @@ -131,10 +131,22 @@ RowLayout { color: Style.ncSecondaryTextColor } + Label { + id: talkReplyMessageSent + text: root.activityData.messageSent + height: (text === "") ? 0 : implicitHeight + width: parent.width + elide: Text.ElideRight + wrapMode: Text.Wrap + maximumLineCount: 2 + font.pixelSize: Style.topLinePixelSize + color: Style.ncSecondaryTextColor + } + Loader { id: talkReplyTextFieldLoader - active: isChatActivity && isTalkReplyPossible && model.displayReplyOption - visible: model.displayReplyOption + active: isChatActivity && isTalkReplyPossible + visible: isTalkReplyOptionVisible anchors.top: activityTextDateTime.bottom anchors.topMargin: 10 diff --git a/src/gui/tray/TalkReplyTextField.qml b/src/gui/tray/TalkReplyTextField.qml index 610e28af1e..a53c88099a 100644 --- a/src/gui/tray/TalkReplyTextField.qml +++ b/src/gui/tray/TalkReplyTextField.qml @@ -13,25 +13,18 @@ Item { } UserModel.currentUser.sendReplyMessage(model.index, model.conversationToken, replyMessageTextField.text, model.messageId); - } - - Text { - id: replyMessageSent - text: model.messageSent - font.pixelSize: Style.topLinePixelSize - color: Style.menuBorder - visible: model.messageSent !== "" + replyMessageTextField.visible = false } TextField { id: replyMessageTextField + visible: model.messageSent === "" // TODO use Layout to manage width/height. The Layout.minimunWidth does not apply to the width set. height: 38 width: 250 onAccepted: root.sendReplyMessage() - visible: replyMessageSent.text === "" topPadding: 4 diff --git a/src/gui/tray/activitydata.h b/src/gui/tray/activitydata.h index 166832f4f3..9c035e2a32 100644 --- a/src/gui/tray/activitydata.h +++ b/src/gui/tray/activitydata.h @@ -113,7 +113,6 @@ public: QString conversationToken; QString messageId; QString messageSent; - bool displayReplyOption = false; }; Type _type; diff --git a/src/gui/tray/activitylistmodel.cpp b/src/gui/tray/activitylistmodel.cpp index a34c2464b9..20badd5dca 100644 --- a/src/gui/tray/activitylistmodel.cpp +++ b/src/gui/tray/activitylistmodel.cpp @@ -1,4 +1,4 @@ -/* +/* * Copyright (C) by Klaas Freitag * * This program is free software; you can redistribute it and/or modify @@ -80,7 +80,6 @@ QHash ActivityListModel::roleNames() const roles[TalkNotificationConversationTokenRole] = "conversationToken"; roles[TalkNotificationMessageIdRole] = "messageId"; roles[TalkNotificationMessageSentRole] = "messageSent"; - roles[TalkNotificationDisplayReplyOptionRole] = "displayReplyOption"; return roles; } @@ -333,8 +332,6 @@ QVariant ActivityListModel::data(const QModelIndex &index, int role) const return a._talkNotificationData.messageId; case TalkNotificationMessageSentRole: return replyMessageSent(a); - case TalkNotificationDisplayReplyOptionRole: - return displayReplyOption(a); default: return QVariant(); } @@ -614,12 +611,6 @@ void ActivityListModel::slotTriggerAction(const int activityIndex, const int act const auto action = activity._links[actionIndex]; - // TODO this will change with https://github.com/nextcloud/desktop/issues/4159 - if (action._verb == "WEB" && action._label == tr("View chat")) { - setDisplayReplyOption(activityIndex); - return; - } - if (action._verb == "WEB") { Utility::openBrowser(QUrl(action._link)); return; @@ -672,7 +663,7 @@ QVariantList ActivityListModel::convertLinksToActionButtons(const Activity &acti } if (static_cast(activity._links.size()) > maxActionButtons()) { - customList << ActivityListModel::convertLinkToActionButton(activity, activity._links.first()); + customList << ActivityListModel::convertLinkToActionButton(activity._links.first()); return customList; } @@ -680,20 +671,18 @@ QVariantList ActivityListModel::convertLinksToActionButtons(const Activity &acti if (activityLink._verb == QStringLiteral("DELETE") || (activity._objectType == QStringLiteral("chat") || activity._objectType == QStringLiteral("call") || activity._objectType == QStringLiteral("room"))) { - customList << ActivityListModel::convertLinkToActionButton(activity, activityLink); + customList << ActivityListModel::convertLinkToActionButton(activityLink); } } return customList; } -QVariant ActivityListModel::convertLinkToActionButton(const OCC::Activity &activity, const OCC::ActivityLink &activityLink) +QVariant ActivityListModel::convertLinkToActionButton(const OCC::ActivityLink &activityLink) { auto activityLinkCopy = activityLink; - const auto isReplyIconApplicable = activityLink._verb == QStringLiteral("WEB") - && (activity._objectType == QStringLiteral("chat") || activity._objectType == QStringLiteral("call") - || activity._objectType == QStringLiteral("room")); + const auto isReplyIconApplicable = activityLink._verb == QStringLiteral("REPLY"); const QString replyButtonPath = QStringLiteral("image://svgimage-custom-color/reply.svg"); @@ -704,14 +693,8 @@ QVariant ActivityListModel::convertLinkToActionButton(const OCC::Activity &activ QString(replyButtonPath + "/" + OCC::Theme::instance()->wizardHeaderTitleColor().name()); } - const auto isReplyLabelApplicable = activityLink._verb == QStringLiteral("WEB") - && (activity._objectType == QStringLiteral("chat") - || (activity._objectType != QStringLiteral("room") && activity._objectType != QStringLiteral("call"))); - if (activityLink._verb == QStringLiteral("DELETE")) { activityLinkCopy._label = QObject::tr("Mark as read"); - } else if (isReplyLabelApplicable) { - activityLinkCopy._label = QObject::tr("Reply"); } return QVariant::fromValue(activityLinkCopy); @@ -836,15 +819,4 @@ QString ActivityListModel::replyMessageSent(const Activity &activity) const { return activity._talkNotificationData.messageSent; } - -void ActivityListModel::setDisplayReplyOption(const int activityIndex) -{ - _finalList[activityIndex]._talkNotificationData.displayReplyOption = true; - emit dataChanged(index(activityIndex, 0), index(activityIndex, 0), {ActivityListModel::TalkNotificationDisplayReplyOptionRole}); -} - -bool ActivityListModel::displayReplyOption(const Activity &activity) const -{ - return activity._talkNotificationData.displayReplyOption; -} } diff --git a/src/gui/tray/activitylistmodel.h b/src/gui/tray/activitylistmodel.h index c0698a9e6b..1a421861e3 100644 --- a/src/gui/tray/activitylistmodel.h +++ b/src/gui/tray/activitylistmodel.h @@ -71,7 +71,6 @@ public: TalkNotificationConversationTokenRole, TalkNotificationMessageIdRole, TalkNotificationMessageSentRole, - TalkNotificationDisplayReplyOptionRole, }; Q_ENUM(DataRole) @@ -108,7 +107,6 @@ public: void setReplyMessageSent(const int activityIndex, const QString &message); QString replyMessageSent(const Activity &activity) const; - bool displayReplyOption(const Activity &activity) const; public slots: void slotRefreshActivity(); @@ -143,14 +141,12 @@ protected: private: static QVariantList convertLinksToMenuEntries(const Activity &activity); static QVariantList convertLinksToActionButtons(const Activity &activity); - static QVariant convertLinkToActionButton(const Activity &activity, const ActivityLink &activityLink); + static QVariant convertLinkToActionButton(const ActivityLink &activityLink); void combineActivityLists(); bool canFetchActivities() const; void ingestActivities(const QJsonArray &activities); - void setDisplayReplyOption(const int activityIndex); - ActivityList _activityLists; ActivityList _syncFileItemLists; ActivityList _notificationLists; @@ -175,7 +171,7 @@ private: bool _doneFetching = false; bool _hideOldActivities = true; - static constexpr quint32 MaxActionButtons = 2; + static constexpr quint32 MaxActionButtons = 3; }; } diff --git a/src/gui/tray/notificationhandler.cpp b/src/gui/tray/notificationhandler.cpp index 0e79d13ad9..9246486e84 100644 --- a/src/gui/tray/notificationhandler.cpp +++ b/src/gui/tray/notificationhandler.cpp @@ -98,9 +98,9 @@ void ServerNotificationHandler::slotNotificationsReceived(const QJsonDocument &j a._id = json.value("notification_id").toInt(); // 2 cases to consider: - // - server == 24 & has Talk: notification type chat/call contains conversationToken/messageId in object_type - // - server < 24 & has Talk: notification type chat/call contains _only_ the conversationToken in object_type - if (a._objectType == "chat" || a._objectType == "call") { + // 1. server == 24 & has Talk: object_type is chat/call/room & object_id contains conversationToken/messageId + // 2. server < 24 & has Talk: object_type is chat/call/room & object_id contains _only_ conversationToken + if (a._objectType == "chat" || a._objectType == "call" || a._objectType == "room") { const auto objectId = json.value("object_id").toString(); const auto objectIdData = objectId.split("/"); a._talkNotificationData.conversationToken = objectIdData.first(); @@ -109,6 +109,14 @@ void ServerNotificationHandler::slotNotificationsReceived(const QJsonDocument &j } else { qCInfo(lcServerNotification) << "Replying directly to Talk conversation" << a._talkNotificationData.conversationToken << "will not be possible because the notification doesn't contain the message ID."; } + + ActivityLink al; + al._label = tr("Reply"); + al._verb = "REPLY"; + al._primary = true; + a._links.insert(0, al); + + list.append(a); } a._status = 0; From ead4eeda1e9667ccc6841392af5000646a3fd141 Mon Sep 17 00:00:00 2001 From: Camila Date: Tue, 5 Apr 2022 14:26:59 +0200 Subject: [PATCH 2/6] Windows fix: activityIndex was not being passed to the lambda slot function. Signed-off-by: Camila --- src/gui/tray/usermodel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gui/tray/usermodel.cpp b/src/gui/tray/usermodel.cpp index 16d3c8263c..21247c0e85 100644 --- a/src/gui/tray/usermodel.cpp +++ b/src/gui/tray/usermodel.cpp @@ -797,7 +797,7 @@ void User::slotSendReplyMessage(const int activityIndex, const QString &token, c { QPointer talkReply = new TalkReply(_account.data(), this); talkReply->sendReplyMessage(token, message, replyTo); - connect(talkReply, &TalkReply::replyMessageSent, this, [&](const QString &message) { + connect(talkReply, &TalkReply::replyMessageSent, this, [&, activityIndex](const QString &message) { _activityModel->setReplyMessageSent(activityIndex, message); }); } From f524635cf400aac4cb2799bcab9353d0f777b198 Mon Sep 17 00:00:00 2001 From: Camila Date: Tue, 5 Apr 2022 15:07:00 +0200 Subject: [PATCH 3/6] Do not hide'View chat' link. This logic was also hiding 'Answer call'. Signed-off-by: Camila --- src/gui/tray/ActivityItemActions.qml | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/gui/tray/ActivityItemActions.qml b/src/gui/tray/ActivityItemActions.qml index cdf7ff70f9..faaedb8c0a 100644 --- a/src/gui/tray/ActivityItemActions.qml +++ b/src/gui/tray/ActivityItemActions.qml @@ -33,11 +33,8 @@ RowLayout { readonly property string verb: model.modelData.verb readonly property bool primary: model.index === 0 && verb !== "DELETE" - readonly property bool isViewChatlink: verb === "WEB" readonly property bool isTalkReplyButton: verb === "REPLY" - visible: !isViewChatlink - Layout.minimumWidth: primary ? Style.activityItemActionPrimaryButtonMinWidth : Style.activityItemActionSecondaryButtonMinWidth Layout.preferredHeight: primary ? parent.height : parent.height * 0.3 Layout.preferredWidth: primary ? -1 : parent.height From 6189b2115ed43b9b0e4cdaaa9cc30ab95dfe2a49 Mon Sep 17 00:00:00 2001 From: Camila Date: Tue, 5 Apr 2022 16:14:58 +0200 Subject: [PATCH 4/6] Adress design issues. - The input field was not using the theming. - After submitting the reply, the text and icon on the left were moving. Signed-off-by: Camila --- src/gui/tray/ActivityItemContent.qml | 2 +- src/gui/tray/TalkReplyTextField.qml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/gui/tray/ActivityItemContent.qml b/src/gui/tray/ActivityItemContent.qml index 7b49e07bdc..4b4f1e3ffd 100644 --- a/src/gui/tray/ActivityItemContent.qml +++ b/src/gui/tray/ActivityItemContent.qml @@ -134,7 +134,7 @@ RowLayout { Label { id: talkReplyMessageSent text: root.activityData.messageSent - height: (text === "") ? 0 : implicitHeight + height: implicitHeight width: parent.width elide: Text.ElideRight wrapMode: Text.Wrap diff --git a/src/gui/tray/TalkReplyTextField.qml b/src/gui/tray/TalkReplyTextField.qml index a53c88099a..9c9d501c85 100644 --- a/src/gui/tray/TalkReplyTextField.qml +++ b/src/gui/tray/TalkReplyTextField.qml @@ -34,7 +34,7 @@ Item { id: replyMessageTextFieldBorder radius: 24 border.width: 1 - border.color: Style.ncBlue + border.color: parent.activeFocus ? UserModel.currentUser.accentColor : Style.menuBorder color: Style.backgroundColor } @@ -48,10 +48,10 @@ Item { onClicked: root.sendReplyMessage() icon { - source: "image://svgimage-custom-color/send.svg" + "/" + Style.ncBlue + source: "image://svgimage-custom-color/send.svg" + "/" + Style.menuBorder width: 38 height: 38 - color: hovered || !sendReplyMessageButton.enabled? Style.menuBorder : Style.ncBlue + color: hovered || !sendReplyMessageButton.enabled? Style.menuBorder : UserModel.currentUser.accentColor } anchors { From 56825825b5b8e86e5d111d3dff706bed58adecc4 Mon Sep 17 00:00:00 2001 From: Camila Date: Tue, 5 Apr 2022 18:44:35 +0200 Subject: [PATCH 5/6] Update ActivityListModel tests. Signed-off-by: Camila --- test/testactivitylistmodel.cpp | 41 +++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/test/testactivitylistmodel.cpp b/test/testactivitylistmodel.cpp index 24c75941e6..027a84956f 100644 --- a/test/testactivitylistmodel.cpp +++ b/test/testactivitylistmodel.cpp @@ -139,11 +139,18 @@ public: QJsonArray actionsArray; + QJsonObject replyAction; + replyAction.insert(QStringLiteral("label"), QStringLiteral("Reply")); + replyAction.insert(QStringLiteral("link"), QStringLiteral("")); + replyAction.insert(QStringLiteral("type"), QStringLiteral("REPLY")); + replyAction.insert(QStringLiteral("primary"), true); + actionsArray.push_back(replyAction); + QJsonObject primaryAction; primaryAction.insert(QStringLiteral("label"), QStringLiteral("View chat")); primaryAction.insert(QStringLiteral("link"), QStringLiteral("http://cloud.example.de/call/9p4vjdzd")); primaryAction.insert(QStringLiteral("type"), QStringLiteral("WEB")); - primaryAction.insert(QStringLiteral("primary"), true); + primaryAction.insert(QStringLiteral("primary"), false); actionsArray.push_back(primaryAction); QJsonObject secondaryAction; @@ -185,11 +192,18 @@ public: QJsonArray actionsArray; + QJsonObject replyAction; + replyAction.insert(QStringLiteral("label"), QStringLiteral("Reply")); + replyAction.insert(QStringLiteral("link"), QStringLiteral("")); + replyAction.insert(QStringLiteral("type"), QStringLiteral("REPLY")); + replyAction.insert(QStringLiteral("primary"), true); + actionsArray.push_back(replyAction); + QJsonObject primaryAction; primaryAction.insert(QStringLiteral("label"), QStringLiteral("View chat")); primaryAction.insert(QStringLiteral("link"), QStringLiteral("http://cloud.example.de/call/9p4vjdzd")); primaryAction.insert(QStringLiteral("type"), QStringLiteral("WEB")); - primaryAction.insert(QStringLiteral("primary"), true); + primaryAction.insert(QStringLiteral("primary"), false); actionsArray.push_back(primaryAction); QJsonObject secondaryAction; @@ -222,11 +236,18 @@ public: QJsonArray actionsArray; + QJsonObject replyAction; + replyAction.insert(QStringLiteral("label"), QStringLiteral("Reply")); + replyAction.insert(QStringLiteral("link"), QStringLiteral("")); + replyAction.insert(QStringLiteral("type"), QStringLiteral("REPLY")); + replyAction.insert(QStringLiteral("primary"), true); + actionsArray.push_back(replyAction); + QJsonObject primaryAction; primaryAction.insert(QStringLiteral("label"), QStringLiteral("Call back")); primaryAction.insert(QStringLiteral("link"), QStringLiteral("http://cloud.example.de/call/9p4vjdzd")); primaryAction.insert(QStringLiteral("type"), QStringLiteral("WEB")); - primaryAction.insert(QStringLiteral("primary"), true); + primaryAction.insert(QStringLiteral("primary"), false); actionsArray.push_back(primaryAction); QJsonObject secondaryAction; @@ -612,6 +633,10 @@ private slots: QVERIFY(index.data(OCC::ActivityListModel::AccountConnectedRole).canConvert()); QVERIFY(index.data(OCC::ActivityListModel::DisplayActions).canConvert()); + QVERIFY(index.data(OCC::ActivityListModel::TalkNotificationConversationTokenRole).canConvert()); + QVERIFY(index.data(OCC::ActivityListModel::TalkNotificationMessageIdRole).canConvert()); + QVERIFY(index.data(OCC::ActivityListModel::TalkNotificationMessageSentRole).canConvert()); + // Unfortunately, trying to check anything relating to filepaths causes a crash // when the folder manager is invoked by the model to look for the relevant file } @@ -658,10 +683,10 @@ private slots: const auto actionButtonsLinks = index.data(OCC::ActivityListModel::ActionsLinksForActionButtonsRole).toList(); - // both action links and buttons must contain a "WEB" verb element at the beginning - QVERIFY(actionsLinks[0].value()._verb == QStringLiteral("WEB")); - QVERIFY(actionButtonsLinks[0].value()._verb == QStringLiteral("WEB")); - + // both action links and buttons must contain a "REPLY" verb element at the beginning + QVERIFY(actionsLinks[0].value()._verb == QStringLiteral("REPLY")); + QVERIFY(actionButtonsLinks[0].value()._verb == QStringLiteral("REPLY")); + // the first action button for chat must have image set QVERIFY(!actionButtonsLinks[0].value()._imageSource.isEmpty()); QVERIFY(!actionButtonsLinks[0].value()._imageSourceHovered.isEmpty()); @@ -687,7 +712,7 @@ private slots: } } else if ((objectType == QStringLiteral("call"))) { QVERIFY( - actionButtonsLinks[0].value()._label == QStringLiteral("Call back")); + actionButtonsLinks[1].value()._label == QStringLiteral("Call back")); } } else { QVERIFY(actionsLinks[0].value()._label == QStringLiteral("Dismiss")); From 74d214726cb2a0a2010aa528028db12d5553f0c9 Mon Sep 17 00:00:00 2001 From: Camila Date: Wed, 6 Apr 2022 10:06:46 +0200 Subject: [PATCH 6/6] Remove unused function parameter. Signed-off-by: Camila --- src/gui/tray/ActivityItem.qml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gui/tray/ActivityItem.qml b/src/gui/tray/ActivityItem.qml index 3003ae667d..42e6739171 100644 --- a/src/gui/tray/ActivityItem.qml +++ b/src/gui/tray/ActivityItem.qml @@ -27,7 +27,7 @@ MouseArea { Accessible.name: (model.path !== "" && model.displayPath !== "") ? qsTr("Open %1 locally").arg(model.displayPath) : model.message Accessible.onPressAction: root.clicked() - function showReplyOptions(activityIndex) { + function showReplyOptions() { isTalkReplyOptionVisible = !isTalkReplyOptionVisible }