search

kiwiirc / kiwibnc

Apache License 2.0
43 stars 14 forks source link

echo-message support #25

Closed 790 closed 5 years ago

790 commented 5 years ago

This will make the BNC request echo-message support from the server, if the server does not support it then kiwibnc will provide the functionality to clients which request it.

I changed the caps from being a Array to a Set because during my debugging I was seeing the array get larger and larger over time with thousands of duplicate entries, can split that out an PR separately if needed.

prawnsalad commented 5 years ago

I'm not sure how the caps would be increasing over time as it's a function scope var? Have you got some debugging logs for that or something

prawnsalad commented 5 years ago

I'm a bit confused here https://github.com/kiwiirc/kiwibnc/pull/25/files#diff-c4bb62687192dc673f1aa860967ab06bR53

If the message being sent to a client does NOT have echo-message and it is from ourselves, why stop the message from being sent? This could leave clients without our own messages which is what the internal BNC logic tries to resolve.

And then if echo-message IS enabled but there's no msgid, then it also stops the message from being sent to the client. What about messages that don't have a msgid?

On top of the above, we need to support msgid and draft/msgid for a while yet.

790 commented 5 years ago

If the message being sent to a client does NOT have echo-message and it is from ourselves, why stop the message from being sent?

This should only be preventing a client from receiving it's own message back when that client doesn't support echo-message.

And then if echo-message IS enabled but there's no msgid, then it also stops the message from being sent to the client. What about messages that don't have a msgid?

Was a poor assumption that messages from the server would contain msgid. Updated in 8628c9e to use a different method of tracking which messages came from client. Feels a bit hacky, maybe there's a better way to tag the origin of a message.

prawnsalad commented 5 years ago

This should only be preventing a client from receiving it's own message back when that client doesn't support echo-message.

When the core BNC gets a message from a client, it reconstructs it and passes it to all connected clients already, echo-message or not. So maybe there's two cases here. 1) When the client has echo-message support - send the message. 2) When the client does NOT have echo message AND the client is NOT the source - send the message.

Feels a bit hacky, maybe there's a better way to tag the origin of a message.

Hm yea, it's got a bad smell as it is. Can't think of anything off hand though so we may need to think on this.

prawnsalad commented 5 years ago

How about adding a .source property to the message which can either be client or upstream? Basically does exactly the same as you have already but a bit more flexible

DanielOaks commented 5 years ago

Nice set of changes. Alright so it took a bit of strange checking and double-checking, but with these commits on top, this PR works 100% as expected it seems: https://github.com/790/kiwibnc/compare/echomessage...DanielOaks:echomessage

Issues we'll need to address elsewhere is that we may be sending tags to clients that don't support tags, and that we're not treating commands as case-insensitive so when we check for command === "PRIVMSG" we're not catching other casing – but it feels like that change would be best made either in irc-framework directly or in the core around here. Neither of these issues are introduced by this PR however so this is fine to merge in after the above commits are pulled on top, really nice work!

prawnsalad commented 5 years ago

@DanielOaks I've bene debating for a while if irc-framework should be uppercasing the command at all times, but I've purposely left that out just in case.. for some reason.. some IRC app using the lib wants to use the raw command sent. Thinking about it now though, we could uppercase it in irc-framework but also include a .raw property for the raw IRC line parsed incase a clients wants to do fancy checking.

prawnsalad commented 5 years ago

@790 going from the suggested changes from doaks, it might be wise to include a helper function supportsEchoMessage(ircMessage){} that applies those checks?

DanielOaks commented 5 years ago

@prawnsalad Hmm, fair enough. Purely in terms of functionality, IRC software should always treat command names as case-insensitive, so doing something like that makes sense (protects devs more than anything).