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

Improve list updates #522

Merged
merged 20 commits into from
Jun 28, 2024
Merged

Improve list updates #522

merged 20 commits into from
Jun 28, 2024

Conversation

laevandus
Copy link
Contributor

@laevandus laevandus commented Jun 21, 2024

🔗 Issue Link

Resolves PBE-4291

🎯 Goal

Smoother and more performant channel and message list updates

🛠 Implementation

  • Do not reload message cells when message changes, instead let SwiftUI to update subviews (messageId change)
  • Remove skipping message list updates

🧪 Testing

Smoke testing of channel and message list (e.g. what we do for regression testing)

☑️ Checklist

  • I have signed the Stream CLA (required)
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Affected documentation updated (docusaurus, tutorial, CMS (task created)

@laevandus laevandus requested a review from a team as a code owner June 21, 2024 07:32
@laevandus laevandus marked this pull request as draft June 21, 2024 07:32
@laevandus laevandus changed the title Improve list updates [WIP] Improve list updates Jun 21, 2024
@laevandus laevandus added 🔧 WIP A PR that is Work In Progress swiftui-repo labels Jun 21, 2024
Comment on lines +231 to +237
let message: ChatMessage
private let adjustedText: String

init(message: ChatMessage) {
self.message = message
adjustedText = message.adjustedText
}
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 don't know why exactly but SwiftUI does not redisplay the view when message.adjustedText changes. Did a workaround fix here.
Case of editing an existing message.

@testableapple testableapple changed the base branch from main to develop June 21, 2024 14:47
Copy link
Contributor

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

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

I think we should limit it to these changes and any possible bug fixes.

@laevandus laevandus removed the 🔧 WIP A PR that is Work In Progress label Jun 26, 2024
@laevandus laevandus marked this pull request as ready for review June 26, 2024 11:11
@laevandus laevandus changed the title [WIP] Improve list updates Improve list updates Jun 26, 2024
message.reactionScoresId != messages[index.row].reactionScoresId
&& utils.messageListConfig.messageDisplayOptions.shouldAnimateReactions
) {
&& animateReactions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverted the extra animation of reactions, didn't look good after all.

scrolledId = nil
return true
}
let alreadyLoaded = messages.map(\.id).contains(baseId)
if alreadyLoaded && baseId != messageId {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this check since before it was always the case, baseId was never equal to messageId

@laevandus
Copy link
Contributor Author

laevandus commented Jun 28, 2024

@martinmitrevski I have been testing message list quite a bit today and there seems to be separate issue with jumping to a message. It is also not working correctly in develop. Therefore, I am proposing limiting changes in this PR to the ones we have and I will work in a separate PR to fix jump to message (it seems that scroll id is not used as it should be).

e.g. if I change the limit to 50 in ChatChannelDataSource.swift:147 then it is visible that after jumping, view is scrolled to the last loaded message, not to the middle message what should happen when jumping to the unread message.

Steps:

  1. Mark really old message as unread and set limit to 50 (better for debugging)
  2. Go to channel list
  3. Go back to channel
  4. Tap on the unread message banner > first unread message is not visible after jumping

Copy link

sonarcloud bot commented Jun 28, 2024

Copy link
Contributor

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@martinmitrevski martinmitrevski merged commit 9938819 into develop Jun 28, 2024
10 checks passed
@martinmitrevski martinmitrevski deleted the list-improvements branch June 28, 2024 14:17
@martinmitrevski martinmitrevski mentioned this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants