-
Notifications
You must be signed in to change notification settings - Fork 269
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(ui+ffi): Add various features to RoomList
#2054
feat(ui+ffi): Add various features to RoomList
#2054
Conversation
…les. No code changes. This patch is just moving things around.
…` in `RoomList`.
5143fee
to
1c87728
Compare
RoomList
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2054 +/- ##
==========================================
+ Coverage 75.59% 75.70% +0.11%
==========================================
Files 149 151 +2
Lines 16158 16174 +16
==========================================
+ Hits 12214 12244 +30
+ Misses 3944 3930 -14
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's on the right path! A few questions below.
@@ -198,6 +198,10 @@ impl RoomListItem { | |||
RUNTIME.block_on(async { self.inner.name().await }) | |||
} | |||
|
|||
fn full_room(&self) -> Arc<Room> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by full
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a legacy term that means: gimme the matrix_sdk_ffi:Room instance. Happy to change to something more meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, as a newcomer this feels a bit strange, maybe room
would be sufficient? (no strong opinion here, so you choose)
@@ -89,7 +90,8 @@ impl Action for AddVisibleRoomsList { | |||
.add_list( | |||
SlidingSyncList::builder(VISIBLE_ROOMS_LIST_NAME) | |||
.sync_mode(SlidingSyncMode::new_selective()) | |||
.timeline_limit(20), | |||
.timeline_limit(20) | |||
.required_state(vec![(StateEventType::RoomEncryption, "".to_owned())]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also enable m.room.member
events here, to help address #2004?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe! @poljar, @stefanceriu, @manuroe, any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't damir talking only about the rooms you're subscribing to and not the full lists? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're only sending events in a room that you're subscribed to. So only there we'll need the member events.
]), | ||
]) | ||
.filters(Some(assign!(SyncRequestListFilters::default(), { | ||
is_invite: Some(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometime later, maybe when the list has been fully loaded, we need to flip this to true
, otherwise we won't ever get invites?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will create a special list dedicated to invites. As it's the case in ElementX right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'd like to re-review after seeing the answers, so making my intent clearer here :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and works well 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for the answers, lgtm!
@@ -198,6 +198,10 @@ impl RoomListItem { | |||
RUNTIME.block_on(async { self.inner.name().await }) | |||
} | |||
|
|||
fn full_room(&self) -> Arc<Room> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, as a newcomer this feels a bit strange, maybe room
would be sufficient? (no strong opinion here, so you choose)
Address #1911.
This PR must be reviewed commit by commit.