Fix(@message) Album of images disappears from reply after reloading t… #12806

kounkou · 2023-11-21T01:33:58Z

Fix(@message) Album of images disappears from reply after reloading the application

Fixes #10986

This PR contains 2 changes :

1- Adding of properties for the quotedMessage
2- Update of the view with the properties with Qml binding

Design decisions :

The reason why the before reload everything works perfectly is because inside MessageView.qml

status-desktop/ui/imports/shared/views/chat/MessageView.qml

Line 669 in 7830310

replyDetails: StatusMessageDetails {

On onResponseMessageChanged signal, we call the messageStore.getMessageByIdAsJson(responseToMessageWithId) to obtain the images of the quotedMessage.

But the model is not made aware of this change at any time. Therefore, when reloading the application, the message doesn't contain the necessary quoted Images to be displayed.
However, the quotedMessage text already exists in the model.

In this PR they were 3 ways to accomplish the goal of fixing the disappearing of images after reloading the application :

[preferred]

Add Qml properties in Nim modules to be able to read them directly in the UI

[performance Impactful change as more payload to process]

Use the signal from Qml to trigger the finding of the quoted message

[slow and unreliable method]

Pass the image in the message going to status-go to be able to read the image on reload

What does the PR do

The PR adds 2 Qml properties and bind them to the View. The 2 properties are the quotedMessageAlbumMessageImages and the quotedMessageAlbumImagesCount that are necessary to display the quoted images (thumbnails) in replies along with how many of the images need to be displayed.

Affected areas

Chat screen is affected. There are only additions to the code.

Screenshot of functionality (including design for comparison)

Cheers!

status-im-auto · 2023-11-21T01:39:30Z

Jenkins Builds

Click to see older builds (5)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✖️	`c263054`	#1	2023-11-21 01:39:29	~5 min	`tests/nim`	📄`log`
✔️	`c263054`	#1	2023-11-21 01:42:08	~7 min	`macos/aarch64`	🍎`dmg`
✔️	`c263054`	#1	2023-11-21 01:44:31	~10 min	`macos/x86_64`	🍎`dmg`
✔️	`c263054`	#1	2023-11-21 01:44:56	~10 min	`tests/ui`	📄`log`
✔️	`c263054`	#1	2023-11-21 01:50:17	~16 min	`linux/x86_64`	📦`tgz`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`74ae23e`	#2	2023-11-21 01:58:36	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`74ae23e`	#2	2023-11-21 01:59:09	~5 min	`tests/nim`	📄`log`
✔️	`74ae23e`	#2	2023-11-21 02:01:03	~7 min	`macos/x86_64`	🍎`dmg`
✔️	`74ae23e`	#2	2023-11-21 02:04:37	~10 min	`tests/ui`	📄`log`
✔️	`74ae23e`	#2	2023-11-21 02:07:57	~14 min	`linux/x86_64`	📦`tgz`
✔️	`74ae23e`	#2	2023-11-21 02:26:31	~32 min	`tests/e2e`	📄`log`
✔️	`74ae23e`	#2	2023-11-21 02:31:20	~37 min	`windows/x86_64`	💿`exe`

✔️	`9a59d35`	#3	2023-11-22 06:10:38	~5 min	`tests/nim`	📄`log`
✔️	`9a59d35`	#3	2023-11-22 06:10:42	~5 min	`macos/aarch64`	🍎`dmg`
✔️	`9a59d35`	#3	2023-11-22 06:13:21	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`9a59d35`	#3	2023-11-22 06:15:40	~10 min	`tests/ui`	📄`log`
✔️	`9a59d35`	#3	2023-11-22 06:18:41	~13 min	`linux/x86_64`	📦`tgz`
✔️	`9a59d35`	#3	2023-11-22 06:27:37	~22 min	`windows/x86_64`	💿`exe`
✔️	`9a59d35`	#3	2023-11-22 06:36:07	~31 min	`tests/e2e`	📄`log`

kounkou · 2023-11-21T01:46:01Z

I am missing some nim tests, Adding them.

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

igor-sirotin · 2023-11-21T10:37:33Z

src/app/modules/main/chat_section/chat_content/messages/module.nim

+
+
+proc setQuotedMessageImages(self: Module, messages: seq[MessageDto], items: var seq[Item], albumIdToImagesMap: Table[string, seq[string]]) =
+  for message in messages:


This look quite suspicious to me that we have ffor message in messages here while setQuotedMessageImages is already called in the same message loop. You sure everything's ok here? 🙂

Good eye. We can probably move setQuotedMessageImages out of the loop in newMessagesLoaded

Let me have a second look on it. Good catch. Updating this.

src/app/modules/main/chat_section/chat_content/messages/module.nim

jrainville

Nice work!

I left a couple of suggestions and also, sadly, we'll need some more work on it in another issue to move it to status-go, since just using Nim is not good enough, since Nim doesn't have all the data.

jrainville · 2023-11-21T15:31:54Z

src/app/modules/main/chat_section/chat_content/messages/module.nim

+
+
+proc setQuotedMessageImages(self: Module, messages: seq[MessageDto], items: var seq[Item], albumIdToImagesMap: Table[string, seq[string]]) =
+  for message in messages:


Good eye. We can probably move setQuotedMessageImages out of the loop in newMessagesLoaded

src/app/modules/main/chat_section/chat_content/messages/module.nim

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

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

…he application Fixes #10986 This PR contains 2 changes : 1- Adding of properties for the quotedMessage 2- Update of the view with the properties with Qml binding

kounkou · 2023-11-22T06:15:27Z

I have added a ticket : #12821 which I believe would solve more consistently the missing image in reply problem exposed here.
Although the current ticket is a progress because :
1/ we understand better the problem (core question is how to efficiently retrieve the quoted Message images album at any time?)
2/ System design decisions are taken in this PR that would help for the ultimate solution. Essentially, we should probably not increase the payload of a message during the sendMessage to avoid increase latency for processing the images, nor rely on asynchronous call to fetch the images directly from the View.
However, it seems that the solution will end up being asynchronous as we should rely on status-go backend which has RPC calls (depending if we use blocking or non-blocking RPC here) to fetch the images of the corresponding message from the database.

From all the above, I believe it might not make more sense to dive deep further into the current implementation/PR. Please let me know if any thoughts.

jrainville

Looks good. Like discussed, this will need some work on the status-go side to make sure it works in all cases, but for now, this PR still helps mitigate the issue so I'm fine with merging it.

igor-sirotin

Nice job! 👍

kounkou requested review from borismelnik, jrainville and igor-sirotin November 21, 2023 01:34

kounkou force-pushed the album-disappearing-from-reply branch from c263054 to 74ae23e Compare November 21, 2023 01:53

igor-sirotin reviewed Nov 21, 2023

View reviewed changes

jrainville reviewed Nov 21, 2023

View reviewed changes

kounkou mentioned this pull request Nov 22, 2023

Retrieve images from Status-go to generate thumbnail images in reply #12821

Closed

Fix(@message) Album of images disappears from reply after reloading t…

9a59d35

…he application Fixes #10986 This PR contains 2 changes : 1- Adding of properties for the quotedMessage 2- Update of the view with the properties with Qml binding

kounkou force-pushed the album-disappearing-from-reply branch from 74ae23e to 9a59d35 Compare November 22, 2023 06:04

jrainville approved these changes Nov 22, 2023

View reviewed changes

igor-sirotin approved these changes Nov 22, 2023

View reviewed changes

kounkou merged commit aaa759f into master Nov 22, 2023
8 of 9 checks passed

kounkou deleted the album-disappearing-from-reply branch November 22, 2023 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(@message) Album of images disappears from reply after reloading t… #12806

Fix(@message) Album of images disappears from reply after reloading t… #12806

kounkou commented Nov 21, 2023

status-im-auto commented Nov 21, 2023 •

edited

Loading

kounkou commented Nov 21, 2023

igor-sirotin Nov 21, 2023

jrainville Nov 21, 2023

kounkou Nov 21, 2023 •

edited

Loading

jrainville left a comment

jrainville Nov 21, 2023

kounkou commented Nov 22, 2023 •

edited

Loading

jrainville left a comment

igor-sirotin left a comment



		proc setQuotedMessageImages(self: Module, messages: seq[MessageDto], items: var seq[Item], albumIdToImagesMap: Table[string, seq[string]]) =
		for message in messages:

Fix(@message) Album of images disappears from reply after reloading t… #12806

Fix(@message) Album of images disappears from reply after reloading t… #12806

Conversation

kounkou commented Nov 21, 2023

Design decisions :

What does the PR do

Affected areas

Screenshot of functionality (including design for comparison)

status-im-auto commented Nov 21, 2023 • edited Loading

Jenkins Builds

kounkou commented Nov 21, 2023

igor-sirotin Nov 21, 2023

Choose a reason for hiding this comment

jrainville Nov 21, 2023

Choose a reason for hiding this comment

kounkou Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

jrainville left a comment

Choose a reason for hiding this comment

jrainville Nov 21, 2023

Choose a reason for hiding this comment

kounkou commented Nov 22, 2023 • edited Loading

jrainville left a comment

Choose a reason for hiding this comment

igor-sirotin left a comment

Choose a reason for hiding this comment

status-im-auto commented Nov 21, 2023 •

edited

Loading

kounkou Nov 21, 2023 •

edited

Loading

kounkou commented Nov 22, 2023 •

edited

Loading