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

Contacts with several same prioritized resources sometimes show wrong overall status #182

Closed
459below opened this issue Nov 8, 2015 · 3 comments · Fixed by #185
Closed

Comments

@459below
Copy link
Contributor

459below commented Nov 8, 2015

I'm opening this separate issue thread for the bug mentioned here and here

The bug lies here

@459below
Copy link
Contributor Author

I've implemented two alternatives.

This one changes the TreeSet to a HashMap, which then gets sorted to determine the highest priority.

But i wasn't very happy with the result. So I resolved the issue here in a different way. I haven't found a way to sort a set in a significant simpler way, than the TreeSet already used here. So instead I modified the Comparator function to make sure it will never deem two objects equal. And since there sometimes just is no way to establish a greater or less than relationship of two priorities or presences, it shouldn't matter at all in what order those same prioritized Presence objects are represented in the TreeSet.

@ibauersachs
Copy link
Member

I like the changed comparator version better because the changeset is smaller. But up to you from which you want to create a PR.

@459below
Copy link
Contributor Author

I agree. I've improved the draft implementation to make it consistent with equals. I also agree with what you said before, about a Set being the wrong choice here but rather a SortedList. I've searched the whole project for TreeSets and skimmed over the code. Some of those could also suffer from the same issue. I think in the long term those TreeSets should be weeded out.

However in the short term I want to go with the smaller fix, since it should be less prone to bugs. I will test it a little bit more and then create the PR.

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 a pull request may close this issue.

2 participants