[CIS-2084, CIS-2085] New Message List Diffing Implementation #2226
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…always to DiffChatMessage
Since the this was actually not causing the messages to jump
nuno-vieira
added
🐞 Bug
nuno-vieira
changed the title
📏 Size AnalysisTotal install size 9.6 MB | This change: ⬆️ +45.7 kB (+0.476%)🗂 See size breakdown
🔎 See the full size analysis (c5b16c5) merging into develop (86daace)
|
Generated by 🚫 Danger |
There was a problem hiding this comment.
Great work @nuno-vieira 👏 . It seems much more stable now, and the performance is better.
I've noticed few issues that we need to fix before merging:
- bug with pagination broken (see comments)
- reactions don't work when in "skip inserts" mode (also in comments)
Additionally, we would need some tests before we can merge it.
polqf
reviewed
Aug 10, 2022
There was a problem hiding this comment.
Overall looks really good, I am just concerned with certain pieces of custom logic. We don't want this evolution to end up being like the previous one where it was full of patches and custom logic.
Also, we should be embedding DifferenceKit as part of StreamChatUI
nuno-vieira
force-pushed
the
fix/CIS-2085-Message-list-with-difference-kit
branch
from
4bdf4f1
to
2e3e59a
Compare
…nserted while skipping messages This caused some data concurrency issues because a lot of data updates are triggered. In order to avoid this reloadData() is much more safe.
It was using the data controller directly, instead of the data source messages
Closed
nuno-vieira
force-pushed
the
fix/CIS-2085-Message-list-with-difference-kit
branch
from
ceb4fae
to
eba3eae
Compare
polqf
approved these changes
Aug 23, 2022
|
Kudos, SonarCloud Quality Gate passed! |
nuno-vieira
added
the
⚡ Performance
Merged
polqf
pushed a commit
that referenced
this pull request
Sep 1, 2022
* Add cell heights cache so that new contentSize calculation is more precise * Remove `_messageListDiffingEnabled` flag and diffable data source implementation * WIP DifferenceKit * Move DiffKit logic to Message List only * Improve performance by using ChatMessage directly instead of mapping always to DiffChatMessage * Improve performance of the message list by skipping channel list updates * Disable animations when loading new messages Since the this was actually not causing the messages to jump * Fix not loading previous messages when there are skipped messages * Add `ListChange` helpers * WIP Skipping Messages * Move the skipping logic to the message list view * Improve the performance of Message Content Equality * Fix Giphy not updating the UI when moved from original position * Add code docs to cell height cache + channel list skip rendering * Add comments explaining CATransaction in DiffKit reload * Update CHANGELOG.md * Update documentation of our dependencies * When skipping messages, only skip insertions, not all updates * Improve the readability of skipping messages * Always store the original messages from the data source to avoid data inconsistencies * Provide comments for the new introduced methods * Add Sentry dependency to DemoApp and remove it from StreamChat * Properly add DifferenceKit dependency * Ignore third party dependencies when formatting code with SwiftFormat * Add required declarations changes to DiffKit dependency Also adds a function to make the removePublicDeclarations.sh more readable * Undo changes on SwiftyMarkdown done by our SwiftFormat * Add missing required declaration changes to DiffKit Source files * Move DiffKit logic to the correct files * Add missing message id comparison in ChatMessage Content Equality * Fix crash when message from same user but with difference device is inserted while skipping messages This caused some data concurrency issues because a lot of data updates are triggered. In order to avoid this reloadData() is much more safe. * Remove willUpdateMessages events since it is actually not needed * Fix message cell layout options not being correctly calculated It was using the data controller directly, instead of the data source messages * Make sure to use the messages from the data source in ChatThreadVC * When a message is hard deleted, make sure to reload the previous cell * Fix thread root message not being updated * Fix and Add missing coverage to ChannelListVC * Fix E2E Tests + First message from participant not appearing * Fix updating replyCount of the parent message in E2E Tests * Fix Xcode 12 compilation * Add E2E Test to coverage giphy message move scroll to bottom case * Fix false positives when asserting messages in E2E Tests * Add E2E Test case for when hard deleting last message from group * Add unit test coverage to when should reload skipped messages * Improve ListChange test coverage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🔗 Issue Links
CIS-2084
CIS-2085
#2094
Bonus:
#2184
CIS-1965
🎯 Goal
The goal of this PR is to fix the crashes in the message list related to data inconsistencies in
performBatchUpdatesand also to fix the message list jumps when receiving multiple messages and the user is scrolled up.📝 Summary
StreamChatUI._messageListDiffingEnabledflag, since now we only have 1 implementation of the message list and it is not experimental anymoreListChangeupdates fromStreamChat, now theStreamChatUIapplies changes from the diffing calculation of the old messages snapshot, and the newest message snapshotPerformance Improvements:
All tests were performed in an iPhone XR iOS 15.5 and for an interval of 10s.
Loading previous messages continuously
Scrolling while inserting multiple new messages at the bottom through stress test script
🎨 Showcase
RPReplay_Final1660064107.MP4
RPReplay_Final1660064236.MP4
🧪 Manual Testing Notes
☑️ Contributor Checklist
🎁 Meme