Skip to content

Conversation

@noctux
Copy link
Contributor

@noctux noctux commented Mar 19, 2025

Based on the discussion in #7.

As with #23 not necessarily fleshed out initial prototype.

Copy link
Owner

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

Thanks! That appears to work well enough for me, but I tested it with the other PR and that caused some non-rooms to be treated as room so give me a bit of time to test again later to see how irssi handles queries in list (but I think it just passes the output directly so this should be fine)

@martinetd
Copy link
Owner

I've made the CI hook to run on PRs, I think it'll run if you rebase on top of master -- in particular I'm checking clippy and rustfmt and I think these won't pass.

It's a bit of a pain to setup rustfmt to run automatically depending on your editor but it looks like most rust projects enforce this and I think the end result is actually quite nice, if you don't want to run it I'll update the branches before merging

@noctux
Copy link
Contributor Author

noctux commented Mar 28, 2025

I've made the CI hook to run on PRs, I think it'll run if you rebase on top of master -- in particular I'm checking clippy and rustfmt and I think these won't pass.

It's a bit of a pain to setup rustfmt to run automatically depending on your editor but it looks like most rust projects enforce this and I think the end result is actually quite nice, if you don't want to run it I'll update the branches before merging

Nah, it's alright, it's good that you call me out for being lazy after jumping machines and not having taken the time to fix the issues in my vim setup :)
Hopefully, the formatting should all be fixed now.

But you are right about the queries: Technically, they do not belong there. weechat actually has a custom view for /list, and when trying to join a Query, it even sends a JOIN to matrircd (which gets ignored, but that is a different issue).
So what is your take on that? Should I better filter queries in this command?

@martinetd
Copy link
Owner

Hopefully, the formatting should all be fixed now.

Thanks!

But you are right about the queries: Technically, they do not belong there. weechat actually has a custom view for /list, and when trying to join a Query, it even sends a JOIN to matrircd (which gets ignored, but that is a different issue).
So what is your take on that? Should I better filter queries in this command?

irssi doesn't care about /list at all, so I'm fine with this. Given the joins are ignored I think it's fine as is from what you're describing (as long as it's not creating "windows" you don't want for queries?).
If you're fine with this I can hit merge here a well :)

@noctux noctux force-pushed the add-list-command branch from 0eea7f2 to a4e7ae4 Compare March 31, 2025 19:59
@noctux
Copy link
Contributor Author

noctux commented Mar 31, 2025

irssi doesn't care about /list at all, so I'm fine with this. Given the joins are ignored I think it's fine as is from what you're describing (as long as it's not creating "windows" you don't want for queries?). If you're fine with this I can hit merge here a well :)

Hmm, I've for now added an alternative implementation that hides queries. I would potentially postpone this one, though, until we know where #23 is heading regarding queries. Depending on whether we autojoin them as "rooms" or keep them as queries seems to be a good arbitrator whether they should appear in /list, I think . That way, it's at least internally consistent.

@martinetd
Copy link
Owner

Yeah we need to be able to list queries, even if it doesn't make sense from an irc point of view.
Given matrirc handles #foo (channel) and foo (query) just the same we could also just list everything as channels, that wouldn't leave any room to tell which it is to the user but it should be obvious enough from the name for users?

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