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.

Sign up for GitHub

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

Change sorting of channels in sidebar #623

Merged
merged 5 commits into from
Nov 7, 2024
Merged

Change sorting of channels in sidebar #623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e69520
add CHANTYPES support
4e554c4c Oct 18, 2024
618d6c3
add STATUSMSG support
4e554c4c Oct 26, 2024
920831a
add tests
4e554c4c Oct 18, 2024
da976f9
change sorting of channels in the sidebar
4e554c4c Oct 18, 2024
bd31605
address review comments
4e554c4c Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion data/src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl Upstream {
Self::Channel(_, channel) => message::Target::Channel {
channel,
source: message::Source::Server(source),
prefix: None,
prefixes: Default::default(),
},
Self::Query(_, nick) => message::Target::Query {
nick,
Expand Down
89 changes: 72 additions & 17 deletions data/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use chrono::{DateTime, Utc};
use futures::channel::mpsc;
use irc::proto::{self, command, Command};
use itertools::{Either, Itertools};
use std::cmp::Ordering;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::fmt;
use std::time::{Duration, Instant};
Expand Down Expand Up @@ -188,6 +189,17 @@ impl Client {
}
}

fn start_reroute(&self, command: &Command) -> bool {
use Command::*;

if let MODE(target, _, _) = command {
!self.is_channel(target)
} else {
matches!(command, WHO(..) | WHOIS(..) | WHOWAS(..))
}
}


fn send(&mut self, buffer: &buffer::Upstream, mut message: message::Encoded) {
if self.supports_labels {
use proto::Tag;
Expand All @@ -204,7 +216,7 @@ impl Client {
}];
}

self.reroute_responses_to = start_reroute(&message.command).then(|| buffer.clone());
self.reroute_responses_to = self.start_reroute(&message.command).then(|| buffer.clone());

if let Err(e) = self.handle.try_send(message.into()) {
log::warn!("Error sending message: {e}");
Expand Down Expand Up @@ -896,7 +908,7 @@ impl Client {
Command::Numeric(RPL_WHOREPLY, args) => {
let target = args.get(1)?;

if proto::is_channel(target) {
if self.is_channel(target) {
if let Some(channel) = self.chanmap.get_mut(target) {
channel.update_user_away(args.get(5)?, args.get(6)?);

Expand All @@ -915,7 +927,7 @@ impl Client {
Command::Numeric(RPL_WHOSPCRPL, args) => {
let target = args.get(2)?;

if proto::is_channel(target) {
if self.is_channel(target) {
if let Some(channel) = self.chanmap.get_mut(target) {
channel.update_user_away(args.get(3)?, args.get(4)?);

Expand Down Expand Up @@ -947,7 +959,7 @@ impl Client {
Command::Numeric(RPL_ENDOFWHO, args) => {
let target = args.get(1)?;

if proto::is_channel(target) {
if self.is_channel(target) {
if let Some(channel) = self.chanmap.get_mut(target) {
if matches!(channel.last_who, Some(WhoStatus::Receiving(_))) {
channel.last_who = Some(WhoStatus::Done(Instant::now()));
Expand Down Expand Up @@ -995,7 +1007,7 @@ impl Client {
}
}
Command::MODE(target, Some(modes), Some(args)) => {
if proto::is_channel(target) {
if self.is_channel(target) {
let modes = mode::parse::<mode::Channel>(modes, args);

if let Some(channel) = self.chanmap.get_mut(target) {
Expand Down Expand Up @@ -1053,7 +1065,7 @@ impl Client {
Command::Numeric(RPL_ENDOFNAMES, args) => {
let target = args.get(1)?;

if proto::is_channel(target) {
if self.is_channel(target) {
if let Some(channel) = self.chanmap.get_mut(target) {
if !channel.names_init {
channel.names_init = true;
Expand Down Expand Up @@ -1276,8 +1288,27 @@ impl Client {
}
}

// TODO allow configuring the "sorting method"
// this function sorts channels together which have similar names when the chantype prefix
// (sometimes multipled) is removed
// e.g. '#chat', '##chat-offtopic' and '&chat-local' all get sorted together instead of in
// wildly different places.
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)
}
Comment on lines +1296 to +1308
Copy link
Member

@tarkah tarkah Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #626 to track this


fn sync(&mut self) {
self.channels = self.chanmap.keys().cloned().collect();
self.channels = self.chanmap.keys().cloned().sorted_by(|a, b| self.compare_channels(a, b)).collect();
self.users = self
.chanmap
.iter()
Expand Down Expand Up @@ -1395,6 +1426,28 @@ impl Client {
}
}
}

pub fn chantypes(&self) -> &[char] {
self.isupport.get(&isupport::Kind::CHANTYPES).and_then(|chantypes| {
let isupport::Parameter::CHANTYPES(types) = chantypes else {
unreachable!("Corruption in isupport table.")
};
types.as_deref()
}).unwrap_or(proto::DEFAULT_CHANNEL_PREFIXES)
}

pub fn statusmsg(&self) -> &[char] {
self.isupport.get(&isupport::Kind::STATUSMSG).map(|statusmsg| {
let isupport::Parameter::STATUSMSG(prefixes) = statusmsg else {
unreachable!("Corruption in isupport table.")
};
prefixes.as_ref()
}).unwrap_or(&[])
}

pub fn is_channel(&self, target: &str) -> bool {
proto::is_channel(target, self.chantypes())
}
}

#[derive(Debug)]
Expand Down Expand Up @@ -1549,6 +1602,18 @@ impl Map {
.unwrap_or_default()
}

pub fn get_chantypes<'a>(&'a self, server: &Server) -> &'a [char] {
self.client(server)
.map(|client| client.chantypes())
.unwrap_or_default()
}

pub fn get_statusmsg<'a>(&'a self, server: &Server) -> &'a [char] {
self.client(server)
.map(|client| client.statusmsg())
.unwrap_or_default()
}

pub fn get_server_handle(&self, server: &Server) -> Option<&server::Handle> {
self.client(server).map(|client| &client.handle)
}
Expand Down Expand Up @@ -1637,16 +1702,6 @@ fn remove_tag(key: &str, tags: &mut Vec<irc::proto::Tag>) -> Option<String> {
.value
}

fn start_reroute(command: &Command) -> bool {
use Command::*;

if let MODE(target, _, _) = command {
!proto::is_channel(target)
} else {
matches!(command, WHO(..) | WHOIS(..) | WHOWAS(..))
}
}

fn stop_reroute(command: &Command) -> bool {
use command::Numeric::*;

Expand Down
4 changes: 2 additions & 2 deletions data/src/history.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ pub enum Kind {
}

impl Kind {
pub fn from_target(server: Server, target: String) -> Self {
if proto::is_channel(&target) {
pub fn from_target(server: Server, target: String, chantypes: &[char]) -> Self {
if proto::is_channel(&target, chantypes) {
Self::Channel(server, target)
} else {
Self::Query(server, Nick::from(target))
Expand Down
8 changes: 5 additions & 3 deletions data/src/history/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,12 @@ impl Manager {
input: Input,
user: User,
channel_users: &[User],
chantypes: &[char],
statusmsg: &[char],
) -> Vec<impl Future<Output = Message>> {
let mut tasks = vec![];

if let Some(messages) = input.messages(user, channel_users) {
if let Some(messages) = input.messages(user, channel_users, chantypes, statusmsg) {
for message in messages {
tasks.extend(self.record_message(input.server(), message));
}
Expand Down Expand Up @@ -595,11 +597,11 @@ impl Data {
filtered
.iter()
.filter_map(|message| {
message.target.prefix().map(|prefix| {
message.target.prefixes().map(|prefixes| {
buffer_config
.status_message_prefix
.brackets
.format(prefix)
.format(prefixes.iter().collect::<String>())
.chars()
.count()
+ 1
Expand Down
6 changes: 3 additions & 3 deletions data/src/input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,13 @@ impl Input {
self.buffer.server()
}

pub fn messages(&self, user: User, channel_users: &[User]) -> Option<Vec<Message>> {
pub fn messages(&self, user: User, channel_users: &[User], chantypes: &[char], statusmsg: &[char]) -> Option<Vec<Message>> {
let to_target = |target: &str, source| {
if let Some((prefix, channel)) = proto::parse_channel_from_target(target) {
if let Some((prefixes, channel)) = proto::parse_channel_from_target(target, chantypes, statusmsg) {
Some(message::Target::Channel {
channel,
source,
prefix,
prefixes,
})
} else if let Ok(user) = User::try_from(target) {
Some(message::Target::Query {
Expand Down
68 changes: 28 additions & 40 deletions data/src/isupport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use irc::proto;
use std::str::FromStr;

// Utilized ISUPPORT parameters should have an associated Kind enum variant
Expand All @@ -9,6 +8,7 @@ pub enum Kind {
AWAYLEN,
CHANLIMIT,
CHANNELLEN,
CHANTYPES,
CNOTICE,
CPRIVMSG,
ELIST,
Expand Down Expand Up @@ -88,23 +88,21 @@ impl FromStr for Operation {
value.split(',').for_each(|channel_limit| {
if let Some((prefix, limit)) = channel_limit.split_once(':') {
if limit.is_empty() {
prefix.chars().for_each(|c| {
if proto::CHANNEL_PREFIXES.contains(&c) {
channel_limits.push(ChannelLimit {
prefix: c,
limit: None,
});
}
});
for c in prefix.chars() {
// TODO validate after STATUSMSG received
channel_limits.push(ChannelLimit {
prefix: c,
limit: None,
});
}
} else if let Ok(limit) = limit.parse::<u16>() {
prefix.chars().for_each(|c| {
if proto::CHANNEL_PREFIXES.contains(&c) {
channel_limits.push(ChannelLimit {
prefix: c,
limit: Some(limit),
});
}
});
for c in prefix.chars() {
// TODO validate after STATUSMSG received
channel_limits.push(ChannelLimit {
prefix: c,
limit: Some(limit),
});
}
}
}
});
Expand Down Expand Up @@ -139,14 +137,12 @@ impl FromStr for Operation {
parse_required_positive_integer(value)?,
))),
"CHANTYPES" => {
if value.is_empty() {
let chars = value.chars().collect::<Vec<_>>();
if chars.is_empty() {
Ok(Operation::Add(Parameter::CHANTYPES(None)))
} else if value.chars().all(|c| proto::CHANNEL_PREFIXES.contains(&c)) {
Ok(Operation::Add(Parameter::CHANTYPES(Some(
value.to_string(),
))))
} else {
Err("value must only contain channel types if specified")
// TODO validate after STATUSMSG is received
Ok(Operation::Add(Parameter::CHANTYPES(Some(chars))))
}
}
"CHATHISTORY" => Ok(Operation::Add(Parameter::CHATHISTORY(
Expand Down Expand Up @@ -331,13 +327,9 @@ impl FromStr for Operation {
let mut prefix_maps = vec![];

if let Some((modes, prefixes)) = value.split_once(')') {
modes.chars().skip(1).zip(prefixes.chars()).for_each(
|(mode, prefix)| {
if proto::CHANNEL_MEMBERSHIP_PREFIXES.contains(&prefix) {
prefix_maps.push(PrefixMap { mode, prefix })
}
},
);
for (mode, prefix) in modes.chars().skip(1).zip(prefixes.chars()) {
prefix_maps.push(PrefixMap { mode, prefix })
}

Ok(Operation::Add(Parameter::PREFIX(prefix_maps)))
} else {
Expand All @@ -350,14 +342,9 @@ impl FromStr for Operation {
parse_optional_positive_integer(value)?,
))),
"STATUSMSG" => {
if value
.chars()
.all(|c| proto::CHANNEL_MEMBERSHIP_PREFIXES.contains(&c))
{
Ok(Operation::Add(Parameter::STATUSMSG(value.to_string())))
} else {
Err("unknown channel membership prefix(es)")
}
let chars = value.chars().collect::<Vec<_>>();
// TODO validate that STATUSMSG ⊂ PREFIX after ISUPPORT ends
Ok(Operation::Add(Parameter::STATUSMSG(chars)))
}
"TARGMAX" => {
let mut command_target_limits = vec![];
Expand Down Expand Up @@ -485,6 +472,7 @@ impl Operation {
"AWAYLEN" => Some(Kind::AWAYLEN),
"CHANLIMIT" => Some(Kind::CHANLIMIT),
"CHANNELLEN" => Some(Kind::CHANNELLEN),
"CHANTYPES" => Some(Kind::CHANTYPES),
"CNOTICE" => Some(Kind::CNOTICE),
"CPRIVMSG" => Some(Kind::CPRIVMSG),
"ELIST" => Some(Kind::ELIST),
Expand Down Expand Up @@ -526,7 +514,7 @@ pub enum Parameter {
CHANLIMIT(Vec<ChannelLimit>),
CHANMODES(Vec<ChannelMode>),
CHANNELLEN(u16),
CHANTYPES(Option<String>),
CHANTYPES(Option<Vec<char>>),
CHATHISTORY(u16),
CLIENTTAGDENY(Vec<ClientOnlyTags>),
CLIENTVER(u16, u16),
Expand Down Expand Up @@ -563,7 +551,7 @@ pub enum Parameter {
SAFELIST,
SECURELIST,
SILENCE(Option<u16>),
STATUSMSG(String),
STATUSMSG(Vec<char>),
TARGMAX(Vec<CommandTargetLimit>),
TOPICLEN(u16),
UHNAMES,
Expand Down
Loading
Loading