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

sdk-base: Add RoomInfoNotableUpdateReasons::MEMBERSHIP #3717

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

jmartinesp
Copy link
Contributor

@jmartinesp jmartinesp commented Jul 17, 2024

This fixes left rooms not disappearing from the room list.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@jmartinesp jmartinesp requested a review from a team as a code owner July 17, 2024 09:55
@jmartinesp jmartinesp requested review from Hywan and removed request for a team July 17, 2024 09:55
@jmartinesp jmartinesp force-pushed the fix/add-membership-notable-room-info-update branch 3 times, most recently from 5fae4ed to d2bc560 Compare July 17, 2024 10:10
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.13%. Comparing base (9c809b9) to head (b45f83b).
Report is 53 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3717      +/-   ##
==========================================
- Coverage   84.13%   84.13%   -0.01%     
==========================================
  Files         263      263              
  Lines       27584    27589       +5     
==========================================
+ Hits        23208    23212       +4     
- Misses       4376     4377       +1     

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

@jmartinesp
Copy link
Contributor Author

@Hywan just double checking, do we still want to use the RoomInfoNotableUpdateReasons APIs? If so I can rebase and fix the conflicts.

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Looks almost good! Thanks for working on this!

Comment on lines 581 to 584
room_info_notable_updates.insert(
room_info.room_id.to_owned(),
RoomInfoNotableUpdateReasons::MEMBERSHIP,
);
Copy link
Member

@Hywan Hywan Aug 8, 2024

Choose a reason for hiding this comment

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

Be careful that it will override an existing entry. Instead, I recommend to use the following code:

Suggested change
room_info_notable_updates.insert(
room_info.room_id.to_owned(),
RoomInfoNotableUpdateReasons::MEMBERSHIP,
);
room_info_notable_updates
.entry(room_id.clone()) // get the entry
.or_default() // if it doesn't exist, create a default one
.insert(RoomInfoNotableUpdateReasons::MEMBERSHIP); // insert `MEMBERSHIP`

Copy link
Contributor Author

@jmartinesp jmartinesp Aug 8, 2024

Choose a reason for hiding this comment

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

Should be fixed by ea949a4

@jmartinesp jmartinesp force-pushed the fix/add-membership-notable-room-info-update branch from eeff546 to b45f83b Compare August 8, 2024 11:42
@jmartinesp jmartinesp requested a review from Hywan August 8, 2024 12:01
@Hywan
Copy link
Member

Hywan commented Aug 8, 2024

@Hywan just double checking, do we still want to use the RoomInfoNotableUpdateReasons APIs? If so I can rebase and fix the conflicts.

Yes! The idea is to restore this API, as mentioned in #3802.

@jmartinesp
Copy link
Contributor Author

So... does this need any more changes?

@Hywan
Copy link
Member

Hywan commented Aug 12, 2024

I don't think so.

@jmartinesp
Copy link
Contributor Author

Then should we approve it so it can be squashed and merged 😅 ?

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Hywan Hywan merged commit 6becbf6 into main Aug 14, 2024
39 of 40 checks passed
@Hywan Hywan deleted the fix/add-membership-notable-room-info-update branch August 14, 2024 15:38
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