Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Flatten and simplify the memberlist sorting algorithm #2381

Merged
merged 8 commits into from
Jan 4, 2019

Conversation

turt2live
Copy link
Member

The previous algorithm had a bug where it was getting stuck on the power level comparison, leaving the memberlist incorrectly ordered. The new algorithm uses less branching to try and walk through the different cases instead. Additionally, the steps used to determine the order have changed slightly to better represent an active member list.

This change also includes changes to try and re-sort the member list more often during presence changes. Events are not always emitted, however. This may be a js-sdk bug but appears to happen prior to these changes as well.

Fixes element-hq/element-web#6953

@turt2live turt2live requested a review from a team December 20, 2018 22:07
@jryans
Copy link
Collaborator

jryans commented Dec 29, 2018

Events are not always emitted, however. This may be a js-sdk bug but appears to happen prior to these changes as well.

Would it make sense to file a separate issue to track this? It seems like something where we'd want to write a test for the correct behavior (and hopefully also fix whatever prevents it currently).

@turt2live
Copy link
Member Author

I think the emit problem may be matrix-org/matrix-js-sdk#404 or a very similar issue.

@jryans
Copy link
Collaborator

jryans commented Jan 2, 2019

Would be possible to add test coverage for this change? It seems complex enough that test coverage would help me understand what's supposed to happen as a reviewer (who isn't familiar with this area) and also help verify it continues to work that way in the future.

@turt2live
Copy link
Member Author

In theory the comments in the diff and the PR description highlight the intended behaviour (and the complexity of the problem). I'll look into some sort of testing though - not sure how we actually do testing for this kind of thing.

@turt2live turt2live removed the request for review from a team January 3, 2019 01:05
@turt2live turt2live self-assigned this Jan 3, 2019
@turt2live turt2live changed the base branch from develop to experimental January 3, 2019 18:34
@turt2live turt2live changed the base branch from experimental to develop January 3, 2019 18:34
The previous algorithm had a bug where it was getting stuck on the power level comparison, leaving the memberlist incorrectly ordered. The new algorithm uses less branching to try and walk through the different cases instead. Additionally, the steps used to determine the order have changed slightly to better represent an active member list.

This commit also includes changes to try and re-sort the member list more often during presence changes. Events are not always emitted, however. This may be a js-sdk bug but appears to happen prior to these changes as well.

Fixes element-hq/element-web#6953
They are useful to have around, but not to have enabled all the time.
@turt2live turt2live force-pushed the travis/fix-memberlist-order branch from 32f727b to 95844eb Compare January 3, 2019 19:15
@turt2live turt2live changed the base branch from develop to experimental January 3, 2019 19:15
@turt2live
Copy link
Member Author

@jryans good call on having me write a test, it actually caught a bug: 5a2113e

Also, this should be ready to review. I've left the console.log statements in the test to ease debugging when the test fails again.

Time is backwards from all the other tests: larger is older, so we want LessThanOrEqual. Also ensure all the power levels are the same to prevent the sort algorithm from running a PL ordering.
@turt2live turt2live force-pushed the travis/fix-memberlist-order branch from bd3b77b to 0ebde52 Compare January 4, 2019 05:40
@turt2live turt2live requested a review from a team January 4, 2019 05:50
@turt2live turt2live removed their assignment Jan 4, 2019
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

lgtm, and yay for testing the complex member list sorting logic (fwiw I'd have probably split out the sorting algorithm and tested it independently of react / the component, but this is fine too).

@turt2live
Copy link
Member Author

fwiw I'd have probably split out the sorting algorithm and tested it independently of react / the component, but this is fine too

The only reason I did it this way is it try and catch the result being wrong. The sorted list gets passed through several functions with the opportunity for mangling, so it seemed best to ensure the thing wasn't mangled wrongly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants