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

feat(sdk): Remove SlidingSyncRoomInner::inner #3085

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Feb 1, 2024

This patch removes the need to store and to update the
SlidingSyncRoom::inner field (of type v4::SlidingSyncRoom). Now,
we just need to handle the prev_batch, which is the only data we care
about.

Bonus: it reduces the size of the frozen sliding sync room quite much.


This patch removes the need to store and to update the
`SlidingSyncRoom::inner` field (of type `v4::SlidingSyncRoom`). Now,
we just need to handle the `prev_batch`, which is the only data we care
about.

Bonus: it reduces the size of the frozen sliding sync room quite much.
@Hywan Hywan force-pushed the fix-sdk-sliding-sync-room-inner branch from 02c879a to 43bed28 Compare February 1, 2024 16:45
@Hywan Hywan marked this pull request as ready for review February 2, 2024 09:35
@Hywan Hywan requested a review from a team as a code owner February 2, 2024 09:35
@Hywan Hywan requested review from bnjbvr, poljar and a team and removed request for a team and bnjbvr February 2, 2024 09:35
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b951d0d) 83.71% compared to head (f2b61eb) 83.72%.
Report is 3 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk/src/sliding_sync/room.rs 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3085      +/-   ##
==========================================
+ Coverage   83.71%   83.72%   +0.01%     
==========================================
  Files         224      224              
  Lines       23497    23478      -19     
==========================================
- Hits        19671    19658      -13     
+ Misses       3826     3820       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Look, after two years of pain it's almost dead. Great work.

/// Internal state of `Self`.
state: RwLock<SlidingSyncRoomState>,

/// The token for back-pagination.
prev_batch: RwLock<Option<String>>,
Copy link
Contributor

@poljar poljar Feb 2, 2024

Choose a reason for hiding this comment

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

There's actually a field in RoomInfo for this as well:

/// The prev batch of this room we received during the last sync.
pub(crate) last_prev_batch: Option<String>,

If the field in RoomInfo makes sense is debatable, but I did want to let you know about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm interesting. I'll update this PR to use the Roominfo::last_prev_batch field instead. It seems more relevant. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to move this to a separate PR, there has been talk that this should not be part of the RoomInfo, though I can't remember the specific complaint anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done here #3092.

@Hywan Hywan merged commit 87a07d9 into matrix-org:main Feb 5, 2024
51 checks passed
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.

2 participants