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

Improve thread partitioning for 2nd degree relations #2165

Merged
merged 17 commits into from
Feb 10, 2022

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Feb 8, 2022

Fixes element-hq/element-web#20966
Fixes element-hq/element-web#20915
Fixes element-hq/element-web#20921
Fixes element-hq/element-web#20282
Fixes element-hq/element-web#20848

  • Cater for 2-nd degree relations in thread partitionning
  • Fix event edits in a thread

This PR currently has no changelog labels, so will not be included in changelogs.

Add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is plus X-Breaking-Change if it's a breaking change.

@germain-gg germain-gg requested a review from a team as a code owner February 8, 2022 17:06
@germain-gg germain-gg marked this pull request as draft February 9, 2022 11:22
Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good - minor comments above.

spec/integ/matrix-client-methods.spec.js Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
@andybalaam
Copy link
Contributor

@gsouquet I see that you are marking a lot of bugs fixed with this change. Did you get a chance to test all those cases? I have been testing only the responses-to-the-thread-root part.

@germain-gg
Copy link
Contributor Author

Did you get a chance to test all those cases?

I did test all of them, and the partitioning was the root of all those issues.
There's a couple more issues that could be solved by this, but did not include them because I have trouble reproducing them reliably

@germain-gg germain-gg merged commit 6b822cc into develop Feb 10, 2022
@germain-gg germain-gg deleted the gsouquet/thread-partition-redactions branch February 10, 2022 15:09
Comment on lines -168 to -170
if (this.filter) {
return this.filter.filterRoomTimeline(pendingEvents);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

@germain-gg I'm trying to understand this change. I note it changed the behaviour of this method so that it no longer applies the TimelineSet's filter (despite what the documentation says), but really my question is: why doesn't the calling code just call room.getPendingEvents, if it actually wants all the pending events rather than going through the filter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants