Skip to content

Conversation

@noctux
Copy link
Contributor

@noctux noctux commented Mar 19, 2025

Pull request based on the discussion/request from #8.

Please consider this more as a "testable draft", this might not be fully fleshed out yet :)

Some clients do not remember and autojoin previously connected channels.
Sending JOINs for these channels on connect helps to rediscover these
channels.
@noctux noctux mentioned this pull request Mar 19, 2025
@martinetd
Copy link
Owner

Thanks!

I think this initiates the join too fast? It seems to consider the rooms as "LeftChan", but some should be joined as far as I understand

the join is processed as soon as the matrix handle is created but we need to wait for the First sync in src/matrix/mod.rs, possibly it makes more sense to join in or after sync_room on the matrix side

@noctux
Copy link
Contributor Author

noctux commented Mar 24, 2025

Thanks for testing and feedback! Yes, your observations sounds correct and matches with some of the "erratic" behaviour I have observed when initially testing. Moving the call seems like a good idea; I'll take a shot at implementing and testing it. It might, however, be end of week until I can get to it.

@noctux
Copy link
Contributor Author

noctux commented Mar 28, 2025

Done, it seems to work a lot more reliable now, including better (heuristic) detection of queries vs channel names (I've also fixed a small bug in the prefix assignment, obviously prefix # should be used for channels, not queries).

@martinetd
Copy link
Owner

Great!

I've tested and yes I get names working now, this is much better.
On my clients though having the join events for queries creates windows I don't want (I'm a weird gus with -all- queries going in the same shared window unless I explicitly open one for someone), so if you need the join events for queries I think I'd like that to be an option perhaps...?
How does weechat handle queries if you don't join them on server join?

Another thing I don't get is, regular servers (e.g. libera.chat or whatever) don't do this. Perhaps if we didn't ignore joins then the "normal" weechat mechanism would work?

@noctux
Copy link
Contributor Author

noctux commented Mar 31, 2025

I've tested and yes I get names working now, this is much better. On my clients though having the join events for queries creates windows I don't want (I'm a weird gus with -all- queries going in the same shared window unless I explicitly open one for someone), so if you need the join events for queries I think I'd like that to be an option perhaps...? How does weechat handle queries if you don't join them on server join?

Hmm, so technically speaking, when sending a JOIN, weechat silently upgrades the queries to a full channel. The rest of our code works fine because the room mapping does not really care what RoomTargetType we have. But it nevertheless all feels hacky, and I don't really like hacks...

On the other hand, the IRC-protocol does not offer a builtin way to silently open/push a query, there is only privmsg.
The new revision therefore now sends a privmsg to every query it encounters.
This is of course intrusive, but some clients (such as my weechat, filter can be found below) allow to silence any notifications for that string, so I personally don't care.
To keep the disturbance of shenanigans like this as low as possible, I've hidden all joins behind a cli-flag which can select between "joining" nothing, channel, queries, or both.
Nevertheless, the query workaround does not feel really clean...

Another thing I don't get is, regular servers (e.g. libera.chat or whatever) don't do this. Perhaps if we didn't ignore joins then the "normal" weechat mechanism would work?

Well, normally, queries are never opened on connect, only on incoming messages or manually via the /query command. Unfortunately, I've found no option to query matrirc for the available nicks to query, so automatically opening them is my workaround for this issue.
So what is your workflow for determining the nick for /query $nick when you want to initiate a conversation?

Before trying out matrirc, I was using https://github.com/poljar/weechat-matrix which has the same UX (so all queries are opened; but being a weechat plugin weechat-matrix can "silently" open them), so this workaround seemed as a natural substitute to me.

For completeness, here's my weechat filter:

/trigger addreplace no_notify_matrirc_reconnect line '' '${tg_tag_notify} == private && ${tg_message_nocolor} == ${raw:* <Resumed connection to matrirc>}' '/.*/-1/notify_level /.*/0/highlight /,notify_[^,]+/,notify_none/tags /^(.*)$/${color:gray}${re:1}/message'

@martinetd
Copy link
Owner

Sorry for the slow reply, I'm quite busy IRL (moving next week!) and will be without internet for a couple of weeks probably so taking a few minutes to reply quickly now.

To keep the disturbance of shenanigans like this as low as possible, I've hidden all joins behind a cli-flag which can select between "joining" nothing, channel, queries, or both.

I think this is acceptable as a first step (perhaps until , but I agree we probably want to aim for better/cleaner here -- let's discuss a bit more alternatives.

Another thing I don't get is, regular servers (e.g. libera.chat or whatever) don't do this. Perhaps if we didn't ignore joins then the "normal" weechat mechanism would work?

Well, normally, queries are never opened on connect, only on incoming messages or manually via the /query command. Unfortunately, I've found no option to query matrirc for the available nicks to query, so automatically opening them is my workaround for this issue.
So what is your workflow for determining the nick for /query $nick when you want to initiate a conversation?

So that's a very good question, as the name available for query isn't the same as the nicks in individual channels there's just no way to know.
Because we cannot create a channel (initiate a new chat) from matrirc currently, my current workflow is to open element web in a browser when I need to initiate a truly new conversation.
If there already is one, then I'll have received a message from it at some point and the names turned out to be stable enough for me even if I reconnect to matrirc so I'll either just guess it right or have it still available from a recent log somewhere or I'll just give up and open element to send a first line and continue from irc -- far from optimal.
[ With the new list function I'd probably use that (but then we need to decide what to do with queries :P), although I'd probably quickly want something that allows filtering so might fallback to a matrirc channel as I had initially discussed (so e.g. I could /msg matrirc list foo to list only chans that contain foo).. But I agree with your point on client integrations with /LIST, so /LIST is probably more correct as a first step. ]

But my question has another side, how does it work on normal servers for regular channels?
I believe servers don't send any join on connect, so weechat must remember them somehow? looks up doc Ah, you're supposed to add them with /autojoin add?
I guess the main difference is that for a normal server you'd need to join explicitly so it's not much work but for matrirc it'd forcefully join you on first message so it's less natural? OTOH /allchan -current /autojoin add ought to work just fine if chans are open in the first place.
I think with /LIST working, for channels we don't really need this?

(one problem that might happen is that we recompute channel names everytime, with two channels having the same display name the irc-side channel name for ircd might change from one boot to the next.. but that's a problem you'd have with this anyway, we'd really need to remember the name as some kind of matrirc state in a db like I started thinking about in #8. Or I guess I could start leaving rooms I don't use more aggressively...)

So that leaves the question for queries... I honestly don't have much idea here; I think if we have a way to list them it's enough as it's pretty much how it works with normal servers but with your current implementation that makes the autojoins optional I'm fine with that for now.
I'll have a look at the code while I don't have internet :)

@noctux
Copy link
Contributor Author

noctux commented May 3, 2025

Sorry for the slow reply, I'm quite busy IRL (moving next week!) and will be without internet for a couple of weeks probably so taking a few minutes to reply quickly now.

No worries, life happens to all of us :) I Hope all things realted to your move work(ed) out well :)

To keep the disturbance of shenanigans like this as low as possible, I've hidden all joins behind a cli-flag which can select between "joining" nothing, channel, queries, or both.

I think this is acceptable as a first step (perhaps until , but I agree we probably want to aim for better/cleaner here -- let's discuss a bit more alternatives.

Another alternative that I came up with: We could promote the matrirc query to a full channel, and autojoin all "query-contacts" there. This enables the user to see all his direct 1:1 contacts there, and as a bonus, we get /query-completion when in this buffer. I've prototyped this functionality in the latest revision. Seems to be acceptable complexity, code-wise and we get to keep the idiomatic distinction between channels and queries.

But my question has another side, how does it work on normal servers for regular channels? I believe servers don't send any join on connect, so weechat must remember them somehow? looks up doc Ah, you're supposed to add them with /autojoin add? I guess the main difference is that for a normal server you'd need to join explicitly so it's not much work but for matrirc it'd forcefully join you on first message so it's less natural? OTOH /allchan -current /autojoin add ought to work just fine if chans are open in the first place. I think with /LIST working, for channels we don't really need this?

Yeah, irc expects the client to know which channels it wants to join, with optional invites if anyone wants to see anyone else.
But there are probably two points here, one usage-related and one implementation related:
I think matrix chat is different from a random irc-network: usually my matrix rooster should only contain channels I'm actually interested in, so I'm fine with autojoining them (optionally), instead of being manually forced to join them a second time in matrirc in case I've first added them in e.g. Element and hopefully remembering to add them to my autojoin.
The second implementation-related one: In this case, I believe we would technically be forced to have some sort of "presence"-management for channels; right now we seem to relay messages even for messages that we currently have not explicitly joined. But clients (at least my weechat, and probably your irssi) seem to handle this just fine, and autojoining does only fix this issue as long as the user does not /part, so this is probably a minor issue at best.

(one problem that might happen is that we recompute channel names everytime, with two channels having the same display name the irc-side channel name for ircd might change from one boot to the next.. but that's a problem you'd have with this anyway, we'd really need to remember the name as some kind of matrirc state in a db like I started thinking about in #8. Or I guess I could start leaving rooms I don't use more aggressively...)

So that leaves the question for queries... I honestly don't have much idea here; I think if we have a way to list them it's enough as it's pretty much how it works with normal servers but with your current implementation that makes the autojoins optional I'm fine with that for now. I'll have a look at the code while I don't have internet :)

Hope the code does not spoil your "digital detox" :)

@noctux noctux force-pushed the autojoin-on-connect branch from 4cc8cfd to 4933a74 Compare May 3, 2025 19:34
@noctux
Copy link
Contributor Author

noctux commented Jul 2, 2025

@martinetd: Meanwhile developed any strong feelings towards one or the option?

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.

Another alternative that I came up with: We could promote the matrirc query to a full channel, and autojoin all "query-contacts" there. This enables the user to see all his direct 1:1 contacts there, and as a bonus, we get /query-completion when in this buffer. I've prototyped this functionality in the latest revision. Seems to be acceptable complexity, code-wise and we get to keep the idiomatic distinction between channels and queries.

That's interesting, so it's a channel you cannot talk to but that just shows a list of all possible names for queries -- I actually like the idea!
That'd work well with irssi as well, and we can repurpose the channel for admin commands (such as join a new chan or whatever like how bitlbee or appservice management bots work) later if we want to.

I'm still way behind on "fun" dev time, but I'm happy to trust you on this; there's a couple of minor nits clippy found but it's probably fine to merge as is; thank you and sorry for the wait!

I'll be running this branch a couple of days and let's get this merged out of the way

use tokio::sync::mpsc;
use tokio_util::codec::Framed;

use crate::args::{args, AutoJoinOptions};
Copy link
Owner

Choose a reason for hiding this comment

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

AutoJoinOptions is unused since you've used the join_queries()/channels() methods

roomtarget.join_chan(irc).await;
} else if chantype == RoomTargetType::Query {
let name = roomtarget.target().await;
let _ = irc.send(join(Some(format!("{}!{}@matrirc", name, name)), "matrirc".to_string())).await?;
Copy link
Owner

Choose a reason for hiding this comment

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

clippy warns on this on my machine: you're using ? so all that's left here is an Unit ();

Either the error is safe to error and there should be no ? or we should drop the let _ =

Looking around the rest of this loop I think we should be consistent: either we expect errors and we should perhaps continue; or keep going, or we should bail out with ? on the join queries case as well.

I think that irc.send has no reason to fail in a way that's recoverable, so I'd make drop the let _ and use ? below as well, but happy either way tbh

The other comment I have reading this diff is that the abstraction for join() is rather annoying when making someone join when we don't have a string ready for their nick!user@host triplet, so perhaps it's make sense to make a second join helper, but I'm happy to let you judge on that as well

@martinetd
Copy link
Owner

I'll be running this branch a couple of days and let's get this merged out of the way

Or so I thought, but irssi doesn't like the "matrirc promoted into a channel" without a hash prefix :/

I've tried renaming to #matrirc consistently in a new autojoin-wip branch of this repo (rebased your patches + small nits I pointed out fixed + that channel rename work), but it's not working properly and I've already spent more time on it than I have; I'll try to get back to it soonish... If you can figure it out that'd be welcome.
In particular room_mappings.rs' matrirc_query() is wrong (apparently can't just change the room type and have this work...), and some notice somewhere had a double # as well, and I must have messed something up because irssi never considers the server to have been connected with this branch (it reconnects every 5 mins; not sure if that behaved the same in your branch or if it's something I did)

So... yeah... more later.

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