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

Fix prioritized community sort #1015

Merged
merged 3 commits into from
Jan 5, 2024

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Jan 5, 2024

Pull Request Description

In PR #984, I added the ability for favorited communities to be prioritized in lists of communities like search results. The problem is that the sorting algorithm always fell back to overall subscriber count for non-favorites, making it essentially a Top of all time sort. This is fine for the auto-complete prompt, but not for search, where a specific sort maybe chosen.

This PR fixes that algorithm to respect the existing order of communities when prioritizing favorites, rather than re-sorting the whole list.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Observe that the selected sort type is now respected, both within favorites, which remain at the top, and within the rest of the list.

qemu-system-x86_64_darBhywft5.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

? -1
: favoriteCommunities?.any((c) => c.community.id == b.community.id) == true
? 1
: b.counts.subscribers.compareTo(a.counts.subscribers),
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the problematic line from before. 😊

);
// This is the list we will return. If we don't do any prioritization,
// it will just be the initial list we received.
List<CommunityView>? prioritizedCommunities = communities;
Copy link
Member

@hjiangsu hjiangsu Jan 5, 2024

Choose a reason for hiding this comment

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

Hmm, I believe there's a less verbose way to do this, but feel free to correct me if I'm wrong!

Instead of having to loop through the list in reverse, we can just perform a filter on the original communities list to get favorited and non-favorited communities. Once we have those, we can combine them into one. I feel like this makes it a bit easier to understand what's happening.

I wrote up some code to explain what I mean (its untested so not sure if it'll work properly) Let me know what you think!

// Create a set of the favorited community ids for filtering later
Set<int> favoriteCommunityIds = Set<int>.from(favoriteCommunities!.map((c) => c.community.id));

// Filters out communities that are part of the favorites, and keeps the same order
List<CommunityView> sortedFavorites = communities.where((community) => favoriteCommunityIds.contains(community.id)).toList();

// Filters out communities that are not a part of the favorites, and keeps the same order
List<CommunityView> sortedNonFavorites = communities.where((community) => !favoriteCommunityIds.contains(community.id)).toList();

// Combine them together, with favorites at the top
List<CommunityView> prioritizedCommunities = List<CommunityView>.from(sortedFavorites)..addAll(sortedNonFavorites);

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! I had to make a few teaks, but I agree it's much more readable! 😊

@micahmo micahmo requested a review from hjiangsu January 5, 2024 19:11
Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

LGTM!

@hjiangsu hjiangsu merged commit c04a70b into thunder-app:develop Jan 5, 2024
1 check passed
@micahmo micahmo deleted the fix/community-sort branch January 5, 2024 23:45
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