RFC: Split messages exceeding maximum length

[nmeum]

This is a preliminary implementation of message splitting as proposed in #41. The implementation is far from finished and has bugs all over the place. I just experimented a bit with implementing message splitting and this is what I came up with so far. Consider this a super experimental proof of concept design proposal.

Let me know if you have any thoughts on this. I also have the feeling that implementing this algorithm correctly is super difficult. As such, this definitely needs some extensive unit tests. I only implemented splitting of PRIVMSG events so far and did some very very basic tests with my own girc-based IRC client and it seems to work a bit (at least).

[codecov-io]

Codecov Report

Merging #42 into master will increase coverage by 0.59%. The diff coverage is 91.30%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   54.62%   55.22%   +0.59%     
==========================================
  Files          13       14       +1     
  Lines        2453     2499      +46     
==========================================
+ Hits         1340     1380      +40     
- Misses        998     1002       +4     
- Partials      115      117       +2     

Impacted Files	Coverage Δ
split.go	90.69% <90.69%> (ø)
conn.go	55.00% <100.00%> (-0.24%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4fc9395...3518e0b. Read the comment docs.

[lrstanley]

Sorry for the delay. Should hopefully have time in the next week or so to review this, the spec, and other clients to see how they handle it (that do have it).

[nmeum]

Sorry for the delay. Should hopefully have time in the next week or so to review this, the spec, and other clients to see how they handle it (that do have it).

Sure, take your time. The implementation is pretty much inspired by the weechat and irssi implementation I referenced in #41. I am of cause open for suggests for improving this implementation.

[codecov-commenter]

Codecov Report

Merging #42 into master will increase coverage by 0.49%. The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   54.62%   55.12%   +0.49%     
==========================================
  Files          13       14       +1     
  Lines        2453     2507      +54     
==========================================
+ Hits         1340     1382      +42     
- Misses        998     1005       +7     
- Partials      115      120       +5     

Impacted Files	Coverage Δ
client.go	64.45% <60.00%> (-0.16%)	:arrow_down:
split.go	84.21% <84.21%> (ø)
conn.go	55.47% <100.00%> (+0.24%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4fc9395...0503d81. Read the comment docs.

[nmeum]

Refactored the architecture a bit, integrated everything more closely with client.go. Most importantly, the maximum prefix length is now only determined once per connection. Additionally, client.Send now returns a list of actually send events. This allows processing splitted events further in application code. For instance, to echo them locally as separate messages.

I think the code is decent enough to be integrated now. I also started using the splitting code in my IRC client and will report back if I encounter any issues.

[lrstanley]

Thanks for keeping up with this. Unfortunately, current world events are drastically limiting my time, via both physical and mental capacity. Hopefully it's not too crazy urgent.

[nmeum]

Thanks for keeping up with this. Unfortunately, current world events are drastically limiting my time, via both physical and mental capacity. Hopefully it's not too crazy urgent.

No, it's not urgent. I am just using my fork for now. Take your time! :relaxed:

[jwflory]

Hi all, I am a downstream user with the RITlug/teleirc project. This would be a useful feature to have integrated upstream as we are trying to figure out splitting messages in our client. It would save us a lot of work for this to merge upstream. Appreciate any time folks can put into this. :pray: Thank you!

cc: @Tjzabel @Zedjones

[nmeum]

I've been using this for the past months with own girc-based IRC client and haven't encountered any issues so far. I would still be open to suggestions for improvements of the current implementation :)

[lrstanley]

I've finally got some time, and I'm looking into this now.

[lrstanley]

An update -- looks like the implementation you have doesn't take a few things into consideration, like:

• If server specifies a larger line limit (LINELEN), everything needs to shift.
• We should probably not take the servers option if it's lower than what our default is (on the safe side, e.g. is the server misconfigured? I've seen NICKLEN inaccurate before).
• If server uses MAXNICKLEN rather than NICKLEN.
• NICKLEN default isn't the best (ircdocs.horse actually specifies a different recommended default, which their ISUPPORT docs seem to back up).
• Should do these some of these calculations as soon as we get the ISUPPORT lines, not during each send event.
• IRCv3 tags shouldn't be included in the calculation, but with this implementation they are. As girc has a handful of default tags, this would cut the length to less than 50% of what it could be. As they have their own 512 char limit (i.e. it doesn't mean we can use 1024, it's separate), they should be excluded from some of these checks.
• CTCP and ACTIONS should be split properly (each split version should still have the necessary prefixes/suffixes and command).
• We should try to split on word boundaries, and if we can't easily (word is >25 chars for example), then switch to just basic character splitting.
• There are currently a few situations where we're capping things inside of girc to 512 (510), but those need to logically adjust to what the server specifies as the line limit. E.g. if we join many channels at once, we can split less if we know there is a higher limit.

As much of this is going to require a different structural layout, I don't think I'll be using the PR directly, so I'll be pushing to another branch so ya'll can test. I'll let you know when it's pushed (about half of it is written right now, just have to figure out the complexities of splitting CTCP/ACTIONS).

[lrstanley]

Almost forgot to mention https://ircv3.net/specs/extensions/multiline. This will be a thing in the future, so it will have influence on this too for servers that support it. I don't want to implement until it's out of a draft/WIP stage, but..

[lrstanley]

Take a look at the split messages branch:

https://github.com/lrstanley/girc/compare/split-messages

Still a few TODOs:

• Handle colors when splitting
• Special treatment for URLs in word-boundary splitting
• Prevent unneeded whitespace truncation in word-boundary splitting (i.e. don't collapse multiple whitespace into one) (and a few other minor things)
• Tests

Feel free to test, the rest isn't too difficult, so shouldn't take too long.

[lrstanley]

Another note: where I originally thought there may be need for panics when users of the library provide invalid events that have too many commands, or are simply way longer than they should be (and can't be split), I think it's better to simply return the original event to put it onto the user of the library and/or to let the server truncate or return an error.

[nmeum]

Thanks for your effort! I will test your branch shortly and provide feedback! :)

[jwflory]

Thanks for this work! We will definitely use it downstream in TeleIRC once implemented. 🚀

[nmeum]

@lrstanley can you open a PR for your changes to allow for inline comments on the code?

[nmeum]

Closed in favor of #43.