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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Message List Data Source Decorator #3247

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

nuno-vieira
Copy link
Member

@nuno-vieira nuno-vieira commented Jun 12, 2024

馃敆 Issue Links

N/A

馃幆 Goal

Possibility of overriding the data source in the message list.

Potential edge cages:
1- If all messages from the first page are removed (unlikely), loading more messages is impossible.
2- If from the first 25 messages, only 1 or 2 are filtered, then it will feel like there are no more pages, but there is.
3- The preview message in the CHannel list might not be in sync if last message from the message list is removed

馃摑 Summary

Provide bullet points with the most important changes in the codebase.

馃洜 Implementation

Provide a detailed description of the implementation and explain your decisions if you find them relevant.

馃帹 Showcase

Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.

Before After
img img

馃И Manual Testing Notes

Explain how this change can be tested manually, if applicable.

鈽戯笍 Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero 鈿狅笍 policy (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

馃巵 Meme

Provide a funny gif or image that relates to your work on this pull request. (Optional)

@nuno-vieira nuno-vieira added 馃帹聽SDK: StreamChatUI Tasks related to the StreamChatUI SDK 鉁吢燜eature An issue or PR related to a feature labels Jun 12, 2024
Copy link

github-actions bot commented Jun 12, 2024

1 Warning
鈿狅笍 Please be sure to complete the Contributor Checklist in the Pull Request description
1 Message
馃摉 There seems to be app changes but CHANGELOG wasn't modified.
Please include an entry if the PR includes user-facing changes.
You can find it at CHANGELOG.md.

Generated by 馃毇 Danger

@nuno-vieira nuno-vieira marked this pull request as ready for review June 17, 2024 22:00
@nuno-vieira nuno-vieira requested a review from a team as a code owner June 17, 2024 22:00
@Stream-iOS-Bot
Copy link
Collaborator

StreamChat XCMetrics

target metric benchmark branch performance status
MessageList Hitches total duration 10 ms 10.01 ms -0.1% 馃斀 馃煛
Duration 2.6 s 2.54 s 2.31% 馃敿 馃煝
Hitch time ratio 4 ms per s 3.93 ms per s 1.75% 馃敿 馃煝
Frame rate 79 fps 78.19 fps 1.03% 馃敿 馃煝
Number of hitches 1 1.2 -20.0% 馃斀 馃敶
ChannelList Hitches total duration 12.5 ms 19.19 ms -53.52% 馃斀 馃敶
Duration 2.6 s 2.55 s 1.92% 馃敿 馃煝
Hitch time ratio 5 ms per s 7.52 ms per s -50.4% 馃斀 馃敶
Frame rate 76 fps 74.17 fps 2.41% 馃敿 馃煝
Number of hitches 1.2 2.2 -83.33% 馃斀 馃敶

Copy link

sonarcloud bot commented Jun 17, 2024

@@ -21,6 +21,10 @@ open class ChatMessageListVC: _ViewController,
UIGestureRecognizerDelegate,
VoiceRecordingAttachmentPresentationViewDelegate
{
open var dataSourceDecorator: (([ChatMessage]) -> [ChatMessage])? = {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we return nil as default? This will filter system messages in the SDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, yes, I'm just testing this to see if Firebase Performance Metrics has any impact or not

listView.currentMessagesFromDataSource = messages
listView.newMessagesSnapshot = messages
if let dataSourceDecorator {
let filteredMessages = dataSourceDecorator(Array(messages))
Copy link
Contributor

Choose a reason for hiding this comment

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

the benchmark tests are failing, maybe good to run them again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is the plan, thats why I'm adding a default decorator

Copy link
Contributor

Choose a reason for hiding this comment

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

That line should be fine since we have background mapping enabled and if it is enabled, LazyCachedMapCollection maps all the items in the init. No performance hit here.

@Stream-iOS-Bot
Copy link
Collaborator

StreamChat XCMetrics

target metric benchmark branch performance status
MessageList Hitches total duration 10 ms 10.01 ms -0.1% 馃斀 馃煛
Duration 2.6 s 2.54 s 2.31% 馃敿 馃煝
Hitch time ratio 4 ms per s 3.95 ms per s 1.25% 馃敿 馃煝
Frame rate 79 fps 77.99 fps 1.28% 馃敿 馃煝
Number of hitches 1 1.0 0.0% 馃斀 馃煝
ChannelList Hitches total duration 12.5 ms 28.37 ms -126.96% 馃斀 馃敶
Duration 2.6 s 2.56 s 1.54% 馃敿 馃煝
Hitch time ratio 5 ms per s 11.08 ms per s -121.6% 馃斀 馃敶
Frame rate 76 fps 73.72 fps 3.0% 馃敿 馃煝
Number of hitches 1.2 2.8 -133.33% 馃斀 馃敶

@Stream-iOS-Bot
Copy link
Collaborator

StreamChat XCMetrics

target metric benchmark branch performance status
MessageList Hitches total duration 10 ms 8.34 ms 16.6% 馃敿 馃煝
Duration 2.6 s 2.55 s 1.92% 馃敿 馃煝
Hitch time ratio 4 ms per s 3.27 ms per s 18.25% 馃敿 馃煝
Frame rate 79 fps 78.04 fps 1.22% 馃敿 馃煝
Number of hitches 1 0.8 20.0% 馃敿 馃煝
ChannelList Hitches total duration 12.5 ms 33.38 ms -167.04% 馃斀 馃敶
Duration 2.6 s 2.55 s 1.92% 馃敿 馃煝
Hitch time ratio 5 ms per s 13.08 ms per s -161.6% 馃斀 馃敶
Frame rate 76 fps 73.75 fps 2.96% 馃敿 馃煝
Number of hitches 1.2 3.2 -166.67% 馃斀 馃敶

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
鉁吢燜eature An issue or PR related to a feature 馃帹聽SDK: StreamChatUI Tasks related to the StreamChatUI SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants