Change sorting of channels in sidebar #623
Conversation
|
hm, this PR seems to have performance issues. The sort code is being called a lot of the time edit: I profiled several times and only came across the perf issue once. It also seems to be mostly an issue in existing code (the code that sorts users, not channels). |
|
@tarkah I was asked to tag you for a review 😸 |
|
Hey @4e554c4c I just got back from vacation, I'll do some review this week. Thanks! |
| fn compare_channels(&self, a: &str, b: &str) -> Ordering { | ||
| let (Some(a_chantype), Some(b_chantype)) = (a.chars().nth(0), b.chars().nth(0)) else { | ||
| return a.cmp(b); | ||
| }; | ||
|
|
||
| if [a_chantype, b_chantype].iter().all(|c| self.chantypes().contains(c)) { | ||
| let ord = a.trim_start_matches(a_chantype).cmp(b.trim_start_matches(b_chantype)); | ||
| if ord != Ordering::Equal { | ||
| return ord; | ||
| } | ||
| } | ||
| a.cmp(b) | ||
| } |
There was a problem hiding this comment.
I think we're long overdue for a Channel type in our data crate. We're exposing a ton of the complexities of parsing / dealing w/ channel prefix vs names across the codebase instead of encapsulating it within a channel module / type. We can then move this logic to an Ord implementation and because Channel will already have prefixes parsed out, the implementation can be much simpler.
I'm not sure we should block this work on it as it'd be a much bigger refactor, but I think it'd be really nice to clean this up along w/ dealing w/ channels and "targets" (channel vs query "nick") throughout the codebase instead of just String like we've been doing.
There was a problem hiding this comment.
one issue is that Ord (as in hashmap) and Ord as in channel sorting are (and should be!) different
we really want a locale-dependent sort on channels and i don't think this is suitable for Ord, which the BTreeMap is based on (for an example of this see my gamja PR)
I wasn't entirely sure on how to rectify this so i decided to not work on it in this PR... so maybe we should push it to another PR?
There was a problem hiding this comment.
I opened #626 to track this
|
@andymandias would love your eyes on this as well since you've got great insight into ISUPPORT and these nuances now |
There was a problem hiding this comment.
It's definitely helpful to have someone look at this who actually uses these features. I don't really use many of these more esoteric features, so things get missed or put off that wouldn't make sense to a frequent user. These look like good additions/extensions to the ISUPPORT support to me.
|
Fixed the merge conflicts! @tarkah, do you mind checking this out before it accumulates too many more? |
This commit changes the sorting of channels to more resemble the weechat autosort plugin, which I use. In the future more sort methods could be added.
This depends on determining the character that is used to denote a channel. Instead of using our existing logic, I implemented
CHANTYPESfollowing the modern irc docs. For good measure, I also added support for theSTATUSMSGisupport.As discussed on the #ircdocs channel, there is a parsing ambiguity if the
STATUSMSGandCHANTYPESisupports intersect (which they rarely do in practice). I do not define our behaviour in this case, but chose a fairly simple heuristic where the firstCHANTYPESchar always begins a halloy channel.I've run
rustfmton one file, and also fixed some odd assumptions that there is only one status prefix on a message. Currently a channel can have multiple status prefixes and I've changed the code to reflect this.