Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/10150/chat marked as read without seeing all messages #10151

Merged

Conversation

osmaczko
Copy link
Contributor

@osmaczko osmaczko commented Apr 4, 2023

fixes: #10150

@osmaczko osmaczko requested review from mprakhov, a team and borismelnik and removed request for a team April 4, 2023 11:05
@status-im-auto
Copy link
Member

status-im-auto commented Apr 4, 2023

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 094c41b #1 2023-04-04 11:11:51 ~5 min imports 📄log
✔️ 094c41b #1 2023-04-04 11:12:39 ~6 min tests-nim 📄log
✔️ 094c41b #1 2023-04-04 11:14:57 ~8 min macos 🍎dmg
✔️ 094c41b #1 2023-04-04 11:18:57 ~12 min linux 📦tgz
✔️ 094c41b #1 2023-04-04 11:22:11 ~16 min linux-e2e 📄log
✔️ 094c41b #1 2023-04-04 11:35:25 ~29 min windows 💿exe

@@ -678,6 +678,7 @@ method resendChatMessage*(self: Module, messageId: string): string =
return self.controller.resendChatMessage(messageId)

method resetNewMessagesMarker*(self: Module) =
self.view.setFirstUnseenMessageLoaded(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should not reset it.
If you'll set it to false, loadingMessagesView will be showing, which is wrong.

Copy link
Contributor

@igor-sirotin igor-sirotin Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mprakhov, I think it's the intention to show the loading view 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we want to show loading messages view when we are waiting for a setFirstUnseenMessageLoaded signal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand, according to the following logic, it is desired behavior.

readonly property bool show: !messageStore.firstUnseenMessageLoaded ||
!messageStore.initialMessagesLoaded

Could you please elaborate on what use case it breaks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to test, when you are scrolling up the chat, and receiving a new message (in order new messages marker appeared). Will you UI flick with loading state or not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is desired on the first chat opening because the backend needs to load the initial message data. When it is loaded, no cense in this UI, all chats will be opened and rendered quickly

@@ -678,6 +678,7 @@ method resendChatMessage*(self: Module, messageId: string): string =
return self.controller.resendChatMessage(messageId)

method resetNewMessagesMarker*(self: Module) =
self.view.setFirstUnseenMessageLoaded(false)
Copy link
Contributor

@igor-sirotin igor-sirotin Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mprakhov, I think it's the intention to show the loading view 🤔

@glitchminer
Copy link
Contributor

@osmaczko , the behaviour is definitely improved. I've noted that when the messages have not all been read and then the app is restarted the chat is loading at the very bottom message, immediately causing the marker to be cleared. What do you think, does that need addressed as part of this issue?

2023-04-05_14-34-27.1.mp4
2023-04-05_14-44-56.mp4

@osmaczko
Copy link
Contributor Author

osmaczko commented Apr 5, 2023

@osmaczko , the behaviour is definitely improved. I've noted that when the messages have not all been read and then the app is restarted the chat is loading at the very bottom message, immediately causing the marker to be cleared. What do you think, does that need addressed as part of this issue?

Could you please check if this behavior is reproducible on current master/0.11.0 RC2? If yes, please create separate issue and assign it to me.

Copy link
Contributor

@glitchminer glitchminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osmaczko , thanks for the fixes. Issue mentioned has been moved to #10197

@jrainville jrainville merged commit ac01824 into master Apr 5, 2023
@jrainville jrainville deleted the fix/10150/chat-marked-as-read-without-seeing-all-messages branch April 5, 2023 14:40
@jrainville
Copy link
Member

jrainville commented Apr 5, 2023

Good job @osmaczko . Can you open the same PR on top of the release branch? Thanks

osmaczko added a commit that referenced this pull request Apr 14, 2023
- new messages marker is reevaluated only if chat has unviewed messages
- loading state is reevaluated only when chat is made active, this fixes
  case described here:
#10151 (comment)

fixes: #10275
osmaczko added a commit that referenced this pull request Apr 14, 2023
- new messages marker is reevaluated only if chat has unviewed messages
- loading state is reevaluated only when chat is made active, this fixes
  case described here:
#10151 (comment)

fixes: #10275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chat: chat is marked as read even without seeing all messages
6 participants