fix: Added `SystemMessagePinnedMessage` contentType #10496

igor-sirotin · 2023-05-01T16:10:30Z

What does the PR do

This PR addresses a change made in status-im/status-go#3414.

I simply added the new ContentType so that it could be correctly processed.
Also, I hided the reply with ContentType=14.

Opened a further request: #10495

Affected areas

Chat

status-im-auto · 2023-05-01T16:15:29Z

Jenkins Builds

Click to see older builds (17)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`02548eb`	#1	2023-05-01 16:15:29	~4 min	`tests/imports`	📄`log`
✔️	`02548eb`	#1	2023-05-01 16:17:34	~6 min	`tests/nim`	📄`log`
✔️	`02548eb`	#1	2023-05-01 16:19:44	~8 min	`macos/aarch64`	🍎`dmg`
✔️	`02548eb`	#1	2023-05-01 16:22:23	~11 min	`macos/x86_64`	🍎`dmg`
✔️	`02548eb`	#1	2023-05-01 16:24:21	~13 min	`linux/x86_64`	📦`tgz`
✔️	`02548eb`	#1	2023-05-01 16:32:47	~21 min	`tests/e2e`	📄`log`
✔️	`02548eb`	#1	2023-05-01 16:44:01	~33 min	`windows/x86_64`	💿`exe`

✔️	`8c0ea64`	#2	2023-05-02 10:39:19	~5 min	`tests/imports`	📄`log`
✔️	`8c0ea64`	#2	2023-05-02 10:40:12	~6 min	`tests/nim`	📄`log`
✔️	`8c0ea64`	#2	2023-05-02 10:41:08	~7 min	`macos/aarch64`	🍎`dmg`
✔️	`8c0ea64`	#2	2023-05-02 10:43:04	~9 min	`macos/x86_64`	🍎`dmg`
✔️	`8c0ea64`	#2	2023-05-02 10:46:08	~12 min	`linux/x86_64`	📦`tgz`
✔️	`8c0ea64`	#2	2023-05-02 10:52:59	~18 min	`tests/e2e`	📄`log`
✔️	`8c0ea64`	#2	2023-05-02 11:02:57	~28 min	`windows/x86_64`	💿`exe`

✖️	`8aff6bf`	#3	2023-05-02 14:24:53	~5 min	`tests/nim`	📄`log`
✔️	`8aff6bf`	#3	2023-05-02 14:25:47	~6 min	`macos/aarch64`	🍎`dmg`
✔️	`8aff6bf`	#3	2023-05-02 14:26:00	~6 min	`tests/imports`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`29e996c`	#4	2023-05-02 14:34:15	~5 min	`tests/imports`	📄`log`
✖️	`29e996c`	#4	2023-05-02 14:34:28	~5 min	`tests/nim`	📄`log`
✖️	`29e996c`	#4	2023-05-02 14:35:35	~7 min	`tests/e2e`	📄`log`
✔️	`29e996c`	#4	2023-05-02 14:38:04	~9 min	`macos/aarch64`	🍎`dmg`
✔️	`29e996c`	#4	2023-05-02 14:42:45	~14 min	`macos/x86_64`	🍎`dmg`
✔️	`29e996c`	#4	2023-05-02 14:44:38	~16 min	`linux/x86_64`	📦`tgz`

✔️	`2245185`	#5	2023-05-02 14:51:58	~5 min	`tests/imports`	📄`log`
✔️	`2245185`	#5	2023-05-02 14:52:12	~5 min	`macos/aarch64`	🍎`dmg`
✔️	`2245185`	#5	2023-05-02 14:52:19	~5 min	`tests/nim`	📄`log`
✔️	`2245185`	#5	2023-05-02 14:56:19	~9 min	`macos/x86_64`	🍎`dmg`
✔️	`2245185`	#5	2023-05-02 14:58:45	~12 min	`linux/x86_64`	📦`tgz`
✔️	`2245185`	#5	2023-05-02 15:06:53	~20 min	`tests/e2e`	📄`log`
✔️	`2245185`	#5	2023-05-02 15:23:55	~37 min	`windows/x86_64`	💿`exe`

jrainville

It wasn't part of the ticket, but making it resilient to future crashes would be great

jrainville · 2023-05-01T16:15:34Z

ui/imports/shared/views/chat/MessageView.qml

@@ -204,6 +204,8 @@ Loader {
            return fetchMoreMessagesButtonComponent
        case Constants.messageContentType.systemMessagePrivateGroupType:
            return privateGroupHeaderComponent
+        case Constants.messageContentType.systemMessagePinnedMessage:
+            return null


Do you think it was crashing because we were trying to create a messageCompoennt when it was actually a systemMessagePinnedMessage?

If that's the case, we'll have crashes each time a new MessageType is added.

So we should make the default return null, because then we just show nothing because we didn't implement it yet, and we need to explicitly list the types that should use messageComponent.

It was crashing on the Nim side in the code like message.contentType.ContentType. I.e. when trying to convert integer to enum. 😐

I agree that it would be great fix that on another level, but thought that it's not easy to implement.
But didn't check that. Let me take a look 🙂

igor-sirotin · 2023-05-02T07:24:20Z

Ok, there's another bug coming from this then. @username pinned a message is actually a message in chat. And therefore you might get to this situation, which is definitely strange 🙂

I'll push a fix for this.

caybro

Modulo what Jo said about the default returning null

igor-sirotin · 2023-05-02T11:43:53Z

About the newMessagesMarker: I implemented a dumb UI for the new contentType.
We definitely want keep this behaviour of the marker to follow Discord UI.

Further UI implementation is tracked here: #10495

jrainville

Nice fix on the System message.

Are you planning on returning null like I suggested or do we add another issue?
It should be simple, just default to null and list all the types that use messageComponent?

igor-sirotin · 2023-05-02T13:32:00Z

@jrainville
Explicitly listing all types for messageComponent is a good idea, will add that.
But that's not the source of the crash we had, it won't prevent further possible bugs on adding a new ContentType in status-go.

I'm currently adding a robust fix fix for Nim side to check the content type when converting to enum.
Should be fine as part of this PR I think.

igor-sirotin · 2023-05-02T14:21:59Z

Enum fix is pushed. Will quickly implement messageComponent explicit check now.

igor-sirotin · 2023-05-02T14:28:30Z

ok, there we are.
@jrainville check it out please 🙂

caybro

Much better!

And robust 🤣

Seriously now, much better

caybro · 2023-05-02T14:32:12Z

src/app_service/service/message/dto/message.nim

@@ -138,7 +139,7 @@ proc toDiscordMessageAuthor*(jsonObj: JsonNode): DiscordMessageAuthor =
  discard jsonObj.getProp("localUrl", result.localUrl)


-proc toDiscordMessageAttachment*(jsonObj: JsonNOde): DiscordMessageAttachment =


ROFL! What the actual...

When are we getting rid of this NIM nonsense?

jrainville

Let's go 👏

jrainville · 2023-05-02T15:42:05Z

ui/imports/shared/views/chat/MessageView.qml

            return messageComponent
+        case Constants.messageContentType.unknownContentType:
+            // NOTE: We could display smth like "unknown message type, please upgrade Status to see it".
+            return null


I think you remove the return, as it would use the default one instead, but I could be wrong. Anyway, it's not important

Yeah I know, I added it explicitly because with this comment it looked quite strange without a ’return’ 😑

fix: Added SystemMessagePinnedMessage contentType

02548eb

igor-sirotin requested review from caybro, jrainville and a team May 1, 2023 16:10

jrainville reviewed May 1, 2023

View reviewed changes

caybro approved these changes May 2, 2023

View reviewed changes

Added PinnedMessage simple implementation

8c0ea64

jrainville reviewed May 2, 2023

View reviewed changes

Added toContentType function

8aff6bf

igor-sirotin requested a review from jrainville May 2, 2023 14:20

fix(MessageView): Explicit type check for messageComponent

29e996c

caybro approved these changes May 2, 2023

View reviewed changes

test fix

2245185

igor-sirotin mentioned this pull request May 2, 2023

System message "pinned message" doesn't make the chat bold in the chats list #10515

Closed

jrainville approved these changes May 2, 2023

View reviewed changes

jrainville merged commit 2e6ccea into master May 2, 2023

jrainville deleted the fix/issue-10485 branch May 2, 2023 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Added `SystemMessagePinnedMessage` contentType #10496

fix: Added `SystemMessagePinnedMessage` contentType #10496

igor-sirotin commented May 1, 2023

status-im-auto commented May 1, 2023 •

edited

Loading

jrainville left a comment

jrainville May 1, 2023

igor-sirotin May 1, 2023

igor-sirotin commented May 2, 2023 •

edited

Loading

caybro left a comment

igor-sirotin commented May 2, 2023

jrainville left a comment

igor-sirotin commented May 2, 2023 •

edited

Loading

igor-sirotin commented May 2, 2023

igor-sirotin commented May 2, 2023

caybro left a comment

caybro May 2, 2023

caybro May 2, 2023

jrainville left a comment

jrainville May 2, 2023

igor-sirotin May 2, 2023

		@@ -138,7 +139,7 @@ proc toDiscordMessageAuthor*(jsonObj: JsonNode): DiscordMessageAuthor =
		discard jsonObj.getProp("localUrl", result.localUrl)


		proc toDiscordMessageAttachment*(jsonObj: JsonNOde): DiscordMessageAttachment =

fix: Added SystemMessagePinnedMessage contentType #10496

fix: Added SystemMessagePinnedMessage contentType #10496

Conversation

igor-sirotin commented May 1, 2023

What does the PR do

Affected areas

status-im-auto commented May 1, 2023 • edited Loading

Jenkins Builds

jrainville left a comment

Choose a reason for hiding this comment

jrainville May 1, 2023

Choose a reason for hiding this comment

igor-sirotin May 1, 2023

Choose a reason for hiding this comment

igor-sirotin commented May 2, 2023 • edited Loading

caybro left a comment

Choose a reason for hiding this comment

igor-sirotin commented May 2, 2023

jrainville left a comment

Choose a reason for hiding this comment

igor-sirotin commented May 2, 2023 • edited Loading

igor-sirotin commented May 2, 2023

igor-sirotin commented May 2, 2023

caybro left a comment

Choose a reason for hiding this comment

caybro May 2, 2023

Choose a reason for hiding this comment

caybro May 2, 2023

Choose a reason for hiding this comment

jrainville left a comment

Choose a reason for hiding this comment

jrainville May 2, 2023

Choose a reason for hiding this comment

igor-sirotin May 2, 2023

Choose a reason for hiding this comment

fix: Added `SystemMessagePinnedMessage` contentType #10496

fix: Added `SystemMessagePinnedMessage` contentType #10496

status-im-auto commented May 1, 2023 •

edited

Loading

igor-sirotin commented May 2, 2023 •

edited

Loading

igor-sirotin commented May 2, 2023 •

edited

Loading