From 5953031bfce4bd41cb15a4317c63c584ad1dc32b Mon Sep 17 00:00:00 2001 From: emizzle Date: Tue, 8 Dec 2020 10:38:53 +1100 Subject: [PATCH] fix: YouTube unfurling YouTube link unfurling was not working for a couple reasons. There were two main parts fixed: 1. QML context for messages pertaining to linkUrls and imageUrls was changed from implicit to explicit. By this, I mean that any time we referenced linkUrls/imageUrls, we were relying on the knowledge that those values would be populated by some parent context several levels up. Now, we are referring to properties that have been explicitly defined on the components. This offers the ability to reuse components, and makes reading the code and debugging much easier. 2. Error handling has been added to getting link preview data. An unhandled "error" was thrown each time a link that wasn't whitelisted was passed in, causing the app to crash. For example, when a link to a tenor gif was posted in the chat, that URL was not whitelisted, causing the app to crash. --- src/app/chat/view.nim | 5 +- src/status/libstatus/chat.nim | 11 ++- src/status/libstatus/types.nim | 8 ++ .../Chat/ChatColumn/ChatMessages.qml | 2 + ui/app/AppLayouts/Chat/ChatColumn/Message.qml | 18 +++-- .../MessageComponents/CompactMessage.qml | 47 +++++++----- .../MessageComponents/ImageLoader.qml | 3 +- .../MessageComponents/ImageMessage.qml | 9 ++- .../MessageComponents/LinksMessage.qml | 22 +++--- .../MessageComponents/NormalMessage.qml | 74 +++++++++++-------- .../ChatColumn/MessageComponents/Sticker.qml | 4 +- vendor/nim-status | 2 +- 12 files changed, 130 insertions(+), 75 deletions(-) diff --git a/src/app/chat/view.nim b/src/app/chat/view.nim index 385f1f948e..43938c7c04 100644 --- a/src/app/chat/view.nim +++ b/src/app/chat/view.nim @@ -312,7 +312,10 @@ QtObject: setClipBoardText(content) proc getLinkPreviewData*(self: ChatsView, link: string): string {.slot.} = - result = $self.status.chat.getLinkPreviewData(link) + try: + $self.status.chat.getLinkPreviewData(link) + except RpcException as e: + $ %* { "error": e.msg } proc joinChat*(self: ChatsView, channel: string, chatTypeInt: int): int {.slot.} = self.status.chat.join(channel, ChatType(chatTypeInt)) diff --git a/src/status/libstatus/chat.nim b/src/status/libstatus/chat.nim index b6ffccd946..56f5b3965e 100644 --- a/src/status/libstatus/chat.nim +++ b/src/status/libstatus/chat.nim @@ -1,4 +1,4 @@ -import json, times, strutils, sequtils, chronicles, json_serialization, algorithm +import json, times, strutils, sequtils, chronicles, json_serialization, algorithm, strformat import core, utils import ../chat/[chat, message] import ../signals/messages @@ -200,4 +200,11 @@ proc unmuteChat*(chatId: string): string = result = callPrivateRPC("unmuteChat".prefix, %*[chatId]) proc getLinkPreviewData*(link: string): JsonNode = - result = callPrivateRPC("getLinkPreviewData".prefix, %*[link]).parseJSON()["result"] \ No newline at end of file + let + responseStr = callPrivateRPC("getLinkPreviewData".prefix, %*[link]) + response = Json.decode(responseStr, RpcResponseTyped[JsonNode], allowUnknownFields = false) + + if not response.error.isNil: + raise newException(RpcException, fmt"""Error getting link preview data for '{link}': {response.error.message}""") + + response.result diff --git a/src/status/libstatus/types.nim b/src/status/libstatus/types.nim index 42376d7871..e906de6955 100644 --- a/src/status/libstatus/types.nim +++ b/src/status/libstatus/types.nim @@ -78,6 +78,14 @@ type result*: string id*: int error*: RpcError + # TODO: replace all RpcResponse and RpcResponseTyped occurances with a generic + # form of RpcReponse. IOW, rename RpceResponseTyped*[T] to RpcResponse*[T] and + # remove RpcResponse. + RpcResponseTyped*[T] = object + jsonrpc*: string + result*: T + id*: int + error*: RpcError proc toAccount*(account: GeneratedAccount): Account = result = Account(name: account.name, photoPath: account.photoPath, keyUid: account.address) diff --git a/ui/app/AppLayouts/Chat/ChatColumn/ChatMessages.qml b/ui/app/AppLayouts/Chat/ChatColumn/ChatMessages.qml index cb6bc0a26f..c8b5ca28a6 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/ChatMessages.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/ChatMessages.qml @@ -299,6 +299,8 @@ ScrollView { imageClick: imagePopup.openPopup.bind(imagePopup) messageId: model.messageId emojiReactions: model.emojiReactions + linkUrls: model.linkUrls + imageUrls: model.imageUrls prevMessageIndex: { // This is used in order to have access to the previous message and determine the timestamp // we can't rely on the index because the sequence of messages is not ordered on the nim side diff --git a/ui/app/AppLayouts/Chat/ChatColumn/Message.qml b/ui/app/AppLayouts/Chat/ChatColumn/Message.qml index d9b532b430..790a5555df 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/Message.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/Message.qml @@ -24,6 +24,7 @@ Item { property int prevMessageIndex: -1 property bool timeout: false property string linkUrls: "" + property string imageUrls: "" property string authorCurrentMsg: "authorCurrentMsg" property string authorPrevMsg: "authorPrevMsg" @@ -47,7 +48,7 @@ Item { property var imageClick: function () {} property var scrollToBottom: function () {} - id: messageItem + id: root width: parent.width anchors.right: !isCurrentUser ? undefined : parent.right height: { @@ -161,7 +162,7 @@ Item { Component { id: channelIdentifierComponent ChannelIdentifier { - authorCurrentMsg: messageItem.authorCurrentMsg + authorCurrentMsg: root.authorCurrentMsg } } @@ -192,7 +193,7 @@ Item { horizontalAlignment: Text.AlignHCenter anchors.horizontalCenter: parent.horizontalCenter textFormat: Text.RichText - topPadding: messageItem.prevMessageIndex === 1 ? Style.current.bigPadding : 0 + topPadding: root.prevMessageIndex === 1 ? Style.current.bigPadding : 0 } } @@ -200,7 +201,11 @@ Item { Component { id: messageComponent NormalMessage { - clickMessage: messageItem.clickMessage + clickMessage: root.clickMessage + linkUrls: root.linkUrls + imageUrls: root.imageUrls + isCurrentUser: root.isCurrentUser + contentType: root.contentType } } @@ -208,7 +213,10 @@ Item { Component { id: compactMessageComponent CompactMessage { - clickMessage: messageItem.clickMessage + clickMessage: root.clickMessage + linkUrls: root.linkUrls + imageUrls: root.imageUrls + contentType: root.contentType } } diff --git a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/CompactMessage.qml b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/CompactMessage.qml index bb46329d9f..3fdee479b3 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/CompactMessage.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/CompactMessage.qml @@ -6,9 +6,12 @@ Item { property var clickMessage: function () {} property int chatHorizontalPadding: 12 property int chatVerticalPadding: 7 - property bool showImages: appSettings.displayChatImages && imageUrls != "" + property string imageUrls: "" + property bool showImages: appSettings.displayChatImages && root.imageUrls != "" + property string linkUrls: "" + property int contentType: 2 - id: chatTextItem + id: root anchors.top: parent.top anchors.topMargin: authorCurrentMsg != authorPrevMsg ? Style.current.smallPadding : 0 height: childrenRect.height + this.anchors.topMargin @@ -29,7 +32,7 @@ Item { UsernameLabel { id: chatName - anchors.leftMargin: chatTextItem.chatHorizontalPadding + anchors.leftMargin: root.chatHorizontalPadding // anchors.top: dateGroupLbl.visible ? dateGroupLbl.bottom : parent.top anchors.top: parent.top anchors.left: chatImage.right @@ -42,21 +45,21 @@ Item { id: chatReply // anchors.top: chatName.visible ? chatName.bottom : (dateGroupLbl.visible ? dateGroupLbl.bottom : parent.top) anchors.top: chatName.visible ? chatName.bottom : parent.top - anchors.topMargin: chatName.visible && this.visible ? chatTextItem.chatVerticalPadding : 0 + anchors.topMargin: chatName.visible && this.visible ? root.chatVerticalPadding : 0 anchors.left: chatImage.right - anchors.leftMargin: chatTextItem.chatHorizontalPadding + anchors.leftMargin: root.chatHorizontalPadding anchors.right: parent.right - anchors.rightMargin: chatTextItem.chatHorizontalPadding + anchors.rightMargin: root.chatHorizontalPadding } ChatText { id: chatText anchors.top: chatReply.bottom - anchors.topMargin: chatName.visible && this.visible ? chatTextItem.chatVerticalPadding : 0 + anchors.topMargin: chatName.visible && this.visible ? root.chatVerticalPadding : 0 anchors.left: chatImage.right - anchors.leftMargin: chatTextItem.chatHorizontalPadding + anchors.leftMargin: root.chatHorizontalPadding anchors.right: parent.right - anchors.rightMargin: chatTextItem.chatHorizontalPadding + anchors.rightMargin: root.chatHorizontalPadding } Loader { @@ -71,7 +74,7 @@ Item { ChatImage { imageSource: image imageWidth: 200 - onClicked: chatTextItem.clickMessage(false, false, true, image) + onClicked: root.clickMessage(false, false, true, image) } } } @@ -81,7 +84,7 @@ Item { active: contentType === Constants.stickerType anchors.left: chatText.left anchors.top: chatName.visible ? chatName.bottom : parent.top - anchors.topMargin: this.visible && chatName.visible ? chatTextItem.chatVerticalPadding : 0 + anchors.topMargin: this.visible && chatName.visible ? root.chatVerticalPadding : 0 sourceComponent: Component { Rectangle { @@ -90,15 +93,17 @@ Item { border.color: Style.current.grey border.width: 1 radius: 16 - width: stickerId.width + 2 * chatTextItem.chatVerticalPadding - height: stickerId.height + 2 * chatTextItem.chatVerticalPadding + width: stickerId.width + 2 * root.chatVerticalPadding + height: stickerId.height + 2 * root.chatVerticalPadding Sticker { id: stickerId anchors.top: parent.top - anchors.topMargin: chatTextItem.chatVerticalPadding + anchors.topMargin: root.chatVerticalPadding anchors.left: parent.left - anchors.leftMargin: chatTextItem.chatVerticalPadding + anchors.leftMargin: root.chatVerticalPadding + contentType: root.contentType + container: root.parent } } } @@ -133,7 +138,7 @@ Item { Loader { id: imageLoader - active: showImages + active: root.showImages anchors.left: chatText.left anchors.leftMargin: 8 anchors.top: chatText.bottom @@ -142,22 +147,26 @@ Item { ImageMessage { color: Style.current.transparent chatHorizontalPadding: 0 + imageUrls: root.imageUrls onClicked: { - chatTextItem.clickMessage(false, false, true, image) + root.clickMessage(false, false, true, image) } + container: root.parent } } } Loader { id: linksLoader - active: !!linkUrls + active: !!root.linkUrls anchors.left: chatText.left anchors.leftMargin: 8 anchors.top: chatText.bottom sourceComponent: Component { - LinksMessage {} + LinksMessage { + linkUrls: root.linkUrls + } } } diff --git a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageLoader.qml b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageLoader.qml index 8b6fb4f767..0640c8ec14 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageLoader.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageLoader.qml @@ -11,6 +11,7 @@ Item { property bool playing: true property bool isAnimated: !!source && source.toString().endsWith('.gif') signal clicked(var image) + property var container id: imageContainer width: loadingImage.visible ? loadingImage.width : imageMessage.width @@ -61,7 +62,7 @@ Item { imageMessage.visible = false } else if (imageMessage.status === Image.Ready) { loadingImage.visible = false - scrollToBottom(true, messageItem) + scrollToBottom(true, root.container) } } diff --git a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageMessage.qml b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageMessage.qml index 1db5228842..95cc6e909a 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageMessage.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/ImageMessage.qml @@ -4,8 +4,10 @@ import "../../../../../imports" Rectangle { property int chatVerticalPadding: 12 property int chatHorizontalPadding: 12 - property bool isCurrentUser: bool + property bool isCurrentUser: false signal clicked(var image) + property var container + property string imageUrls: "" id: imageChatBox height: { @@ -32,11 +34,11 @@ Rectangle { Repeater { id: imageRepeater model: { - if (!imageUrls) { + if (!root.imageUrls) { return [] } - return imageUrls.split(" ") + return root.imageUrls.split(" ") } ImageLoader { @@ -49,6 +51,7 @@ Rectangle { onClicked: { imageChatBox.clicked(image) } + container: root.container } } diff --git a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/LinksMessage.qml b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/LinksMessage.qml index ddd06837a9..0e8311e97a 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/LinksMessage.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/LinksMessage.qml @@ -6,7 +6,9 @@ import "../../../../../shared/status" import "../../../Profile/LeftTab/constants.js" as ProfileConstants Item { - id: linksItem + id: root + property string linkUrls: "" + height: { let h = 0 for (let i = 0; i < linksRepeater.count; i++) { @@ -27,11 +29,11 @@ Item { Repeater { id: linksRepeater model: { - if (!linkUrls) { + if (!root.linkUrls) { return [] } - return linkUrls.split(" ") + return root.linkUrls.split(" ") } @@ -82,15 +84,15 @@ Item { id: unfurledLinkComponent Loader { property var linkData: { - try { - const data = chatsModel.getLinkPreviewData(linkString) - return JSON.parse(data) - } catch (e) { - console.error("Error parsing link data", e) - return undfined + const data = chatsModel.getLinkPreviewData(linkString) + const result = JSON.parse(data) + if (result.error) { + console.error(result.error) + return undefined } + return result } - enabled: linkData !== undefined && !!linkData.title + active: linkData !== undefined && !!linkData.title sourceComponent: Component { Rectangle { id: rectangle diff --git a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/NormalMessage.qml b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/NormalMessage.qml index 0930557ea1..49edddd0cc 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/NormalMessage.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/NormalMessage.qml @@ -4,9 +4,13 @@ import "../../../../../imports" Item { property var clickMessage: function () {} - property bool showImages: appSettings.displayChatImages && imageUrls !== "" + property string imageUrls: "" + property bool showImages: appSettings.displayChatImages && root.imageUrls !== "" + property string linkUrls: "" + property bool isCurrentUser: false + property int contentType: 2 - id: chatTextItem + id: root anchors.top: parent.top anchors.topMargin: authorCurrentMsg !== authorPrevMsg ? Style.current.smallPadding : 0 height: childrenRect.height + this.anchors.topMargin + (dateGroupLbl.visible ? dateGroupLbl.height : 0) @@ -18,7 +22,7 @@ Item { UserImage { id: chatImage - visible: chatsModel.activeChannel.chatType !== Constants.chatTypeOneToOne && isMessage && authorCurrentMsg != authorPrevMsg && !isCurrentUser + visible: chatsModel.activeChannel.chatType !== Constants.chatTypeOneToOne && isMessage && authorCurrentMsg != authorPrevMsg && !root.isCurrentUser anchors.left: parent.left anchors.leftMargin: Style.current.padding anchors.top: dateGroupLbl.visible ? dateGroupLbl.bottom : parent.top @@ -27,7 +31,7 @@ Item { UsernameLabel { id: chatName - visible: chatsModel.activeChannel.chatType !== Constants.chatTypeOneToOne && isMessage && authorCurrentMsg != authorPrevMsg && !isCurrentUser + visible: chatsModel.activeChannel.chatType !== Constants.chatTypeOneToOne && isMessage && authorCurrentMsg != authorPrevMsg && !root.isCurrentUser anchors.leftMargin: 20 anchors.top: dateGroupLbl.visible ? dateGroupLbl.bottom : parent.top anchors.topMargin: 0 @@ -57,7 +61,7 @@ Item { if (isImage) { return "transparent" } - return isCurrentUser ? Style.current.primary : Style.current.secondaryBackground + return root.isCurrentUser ? Style.current.primary : Style.current.secondaryBackground } border.color: isSticker ? Style.current.border : Style.current.transparent border.width: 1 @@ -100,11 +104,11 @@ Item { } radius: 16 - anchors.left: !isCurrentUser ? chatImage.right : undefined - anchors.leftMargin: !isCurrentUser ? 8 : 0 - anchors.right: !isCurrentUser ? undefined : parent.right - anchors.rightMargin: !isCurrentUser ? 0 : Style.current.padding - anchors.top: authorCurrentMsg != authorPrevMsg && !isCurrentUser ? chatImage.top : (dateGroupLbl.visible ? dateGroupLbl.bottom : parent.top) + anchors.left: !root.isCurrentUser ? chatImage.right : undefined + anchors.leftMargin: !root.isCurrentUser ? 8 : 0 + anchors.right: !root.isCurrentUser ? undefined : parent.right + anchors.rightMargin: !root.isCurrentUser ? 0 : Style.current.padding + anchors.top: authorCurrentMsg != authorPrevMsg && !root.isCurrentUser ? chatImage.top : (dateGroupLbl.visible ? dateGroupLbl.bottom : parent.top) anchors.topMargin: 0 visible: isMessage @@ -128,7 +132,7 @@ Item { anchors.leftMargin: chatBox.chatHorizontalPadding anchors.right: chatBox.longChatText ? parent.right : undefined anchors.rightMargin: chatBox.longChatText ? chatBox.chatHorizontalPadding : 0 - textField.color: !isCurrentUser ? Style.current.textColor : Style.current.currentUserTextColor + textField.color: !root.isCurrentUser ? Style.current.textColor : Style.current.currentUserTextColor Connections { target: appSettings.compactMode ? null : chatBox onLongChatTextChanged: { @@ -155,8 +159,8 @@ Item { id: chatImageComponent imageSource: image imageWidth: 250 - isCurrentUser: messageItem.isCurrentUser - onClicked: chatTextItem.clickMessage(false, false, true, image) + isCurrentUser: root.isCurrentUser + onClicked: root.clickMessage(false, false, true, image) } } } @@ -183,6 +187,8 @@ Item { anchors.top: parent.top anchors.topMargin: chatBox.chatVerticalPadding color: Style.current.transparent + container: root.parent + contentType: root.contentType } MessageMouseArea { @@ -197,16 +203,16 @@ Item { ChatTime { id: chatTime - anchors.top: showImages ? imageLoader.bottom : chatBox.bottom + anchors.top: root.showImages ? imageLoader.bottom : chatBox.bottom anchors.topMargin: 4 anchors.bottomMargin: Style.current.padding - anchors.right: showImages ? imageLoader.right : chatBox.right - anchors.rightMargin: isCurrentUser ? 5 : Style.current.padding + anchors.right: root.showImages ? imageLoader.right : chatBox.right + anchors.rightMargin: root.isCurrentUser ? 5 : Style.current.padding } SentMessage { id: sentMessage - visible: isCurrentUser && !timeout && !isExpired && isMessage && outgoingStatus !== "sent" + visible: root.isCurrentUser && !timeout && !isExpired && isMessage && outgoingStatus !== "sent" anchors.top: chatTime.top anchors.bottomMargin: Style.current.padding anchors.right: chatTime.left @@ -223,12 +229,12 @@ Item { Loader { id: imageLoader - active: showImages + active: root.showImages sourceComponent: imageComponent - anchors.left: !isCurrentUser ? chatImage.right : undefined - anchors.leftMargin: !isCurrentUser ? 8 : 0 - anchors.right: !isCurrentUser ? undefined : parent.right - anchors.rightMargin: !isCurrentUser ? 0 : Style.current.padding + anchors.left: !root.isCurrentUser ? chatImage.right : undefined + anchors.leftMargin: !root.isCurrentUser ? 8 : 0 + anchors.right: !root.isCurrentUser ? undefined : parent.right + anchors.rightMargin: !root.isCurrentUser ? 0 : Style.current.padding anchors.top: chatBox.bottom anchors.topMargin: Style.current.smallPadding } @@ -236,25 +242,29 @@ Item { Component { id: imageComponent ImageMessage { - isCurrentUser: messageItem.isCurrentUser + isCurrentUser: root.isCurrentUser + container: root.parent + imageUrls: root.imageUrls onClicked: { - chatTextItem.clickMessage(false, false, true, image) + root.clickMessage(false, false, true, image) } } } Loader { id: linksLoader - active: !!linkUrls - anchors.left: !isCurrentUser ? chatImage.right : undefined - anchors.leftMargin: !isCurrentUser ? 8 : 0 - anchors.right: !isCurrentUser ? undefined : parent.right - anchors.rightMargin: !isCurrentUser ? 0 : Style.current.padding + active: !!root.linkUrls + anchors.left: !root.isCurrentUser ? chatImage.right : undefined + anchors.leftMargin: !root.isCurrentUser ? 8 : 0 + anchors.right: !root.isCurrentUser ? undefined : parent.right + anchors.rightMargin: !root.isCurrentUser ? 0 : Style.current.padding anchors.top: chatBox.bottom anchors.topMargin: Style.current.smallPadding sourceComponent: Component { - LinksMessage {} + LinksMessage { + linkUrls: root.linkUrls + } } } @@ -262,8 +272,8 @@ Item { id: emojiReactionLoader active: emojiReactions !== "" sourceComponent: emojiReactionsComponent - anchors.left: !isCurrentUser ? chatBox.left : undefined - anchors.right: !isCurrentUser ? undefined : chatBox.right + anchors.left: !root.isCurrentUser ? chatBox.left : undefined + anchors.right: !root.isCurrentUser ? undefined : chatBox.right anchors.top: chatBox.bottom anchors.topMargin: 2 } diff --git a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/Sticker.qml b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/Sticker.qml index ff5edfd8c5..45bd253b1a 100644 --- a/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/Sticker.qml +++ b/ui/app/AppLayouts/Chat/ChatColumn/MessageComponents/Sticker.qml @@ -4,6 +4,8 @@ import "../../../../../imports" Loader { property color color + property var container + property int contentType: -1 id: root active: contentType === Constants.stickerType @@ -12,7 +14,7 @@ Loader { Shared.ImageLoader { color: root.color onLoaded: { - scrollToBottom(true, messageItem) + scrollToBottom(true, root.container) } width: 140 diff --git a/vendor/nim-status b/vendor/nim-status index fcf81af89f..f56f8834d2 160000 --- a/vendor/nim-status +++ b/vendor/nim-status @@ -1 +1 @@ -Subproject commit fcf81af89f5c5f3203b9b35214dd478afec71560 +Subproject commit f56f8834d2e9cfaebcf1742bd7f74af003ef087d