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.

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

Properly split long IRC messages #29

Merged
merged 12 commits into from Jul 7, 2014

Conversation

Projects
None yet
3 participants
@sebth
Contributor

sebth commented Jun 13, 2014

This pull request adds handling of long IRC messages to the core. In contrast to the "splitlong.pl" plugin, multi-byte encoded and recoded messages are properly split.

To allow for this, a new function has been added to the server struct: "split_message". "split_message" returns a string array with the message splitted to substrings of a length that the server can handle. If a protocol module doesn't have any limit, it can simply set "split_message" to NULL.

The "MSG" chat command now calls "split_message" before "send_message", and emits "message own_public" / "message own_private" with each substring, so that the string splitting will be visible in the UI. In the IRC module, the "ME" and "ACTION" commands work in a similar way.

"split_message" in the IRC module uses "recode_split" which in turn uses iconv to properly split multi-byte encoded (and recoded) messages.

This pull request also adds two new settings: "split_line_start" and "split_line_end", analogous to "splitlong_line_start" and "splitlong_line_end" in "splitlong.pl".

This fixes issue #1.

@ahf

This comment has been minimized.

Member

ahf commented Jun 13, 2014

Nice, thanks - overall it looks good - I will give it some testing and some more review during the weekend.

@coekie

View changes

src/core/chat-commands.c Outdated
g_strfreev(splitmsgs);
} else {
signal_emit("message own_private", 4, server, msg, target,
origtarget);

This comment has been minimized.

@coekie

coekie Jun 17, 2014

Contributor

why don't you split this one?

This comment has been minimized.

@sebth

sebth Jun 17, 2014

Contributor

Because "msg" doesn't get sent to the server in this case. To be honest I'm not sure when this function is called with "target = NULL". Should msg really be split in this case?

This comment has been minimized.

@sebth

sebth Jun 17, 2014

Contributor

Alright, I understand now. "target" is NULL when "parse_special" fails. We still need to signal "message own_private" so that "sig_message_own_private" in "src/fe-common/core/fe-messages.c" prints an error message. There's no need to split "msg" in this case because nothing gets sent to the server. (In fact, currently it doesn't matter at all what "msg" is on line 395, because none of the signal handlers uses it when "target" is NULL.)

@ahf

View changes

src/irc/core/irc-servers.c Outdated
* 63 is the maxmium hostname length defined by the protocol. 10 is
* a common username limit on many networks. 1 is for the `@'.
*/
int userhostlen = 63 + 10 + 1;

This comment has been minimized.

@ahf

ahf Jun 18, 2014

Member

This calculation is made twice. It should be split out as a shared constant with a short description as comment.

@ahf

View changes

src/irc/core/irc-servers.c Outdated
const char *data)
{
/* See split_message() on how the maximum length is calculated. */
int userhostlen = 63 + 10 + 1;

This comment has been minimized.

@ahf

ahf Jun 18, 2014

Member

Constant! :-)

@ahf

View changes

src/irc/core/irc-servers.c Outdated
@@ -867,6 +977,8 @@ void irc_servers_init(void)
settings_add_str("misc", "usermode", DEFAULT_USER_MODE);
settings_add_time("flood", "cmd_queue_speed", DEFAULT_CMD_QUEUE_SPEED);
settings_add_int("flood", "cmds_max_at_once", DEFAULT_CMDS_MAX_AT_ONCE);
settings_add_str("misc", "split_line_start", "");
settings_add_str("misc", "split_line_end", "");

This comment has been minimized.

@ahf

ahf Jun 18, 2014

Member

Nit: Move these up to the other "misc" setting.

@ahf

This comment has been minimized.

Member

ahf commented Jun 29, 2014

Have there been any further discussion about this whilst I have been on holiday?

@sebth

This comment has been minimized.

Contributor

sebth commented Jun 29, 2014

Not that I've seen.

@ahf

View changes

src/core/misc.c Outdated
int n = strlen(str) / len;
int i;
if (strlen(str) % len)

This comment has been minimized.

@ahf

ahf Jul 6, 2014

Member

Minor thing, but you could easily cache the value of the first strlen() call and reuse it here.

@ahf

View changes

src/core/recode.c Outdated
if (to == NULL)
/* default outgoing charset if set */
to = settings_get_str("recode_out_default_charset");
if (to && *to != '\0') {

This comment has been minimized.

@ahf

ahf Jul 6, 2014

Member

Nitpick: to != NULL.

@ahf

View changes

src/core/recode.c Outdated
int n = 0;
char **ret;
if (!str)

This comment has been minimized.

@ahf

ahf Jul 6, 2014

Member

g_return_val_if_fail() and explicit NULL please.

@ahf

This comment has been minimized.

Member

ahf commented Jul 6, 2014

Really nice work here @sebth - fix the few nitpicks and let's get the code in and let's get the release out for people to test it :-)

sebth added some commits Jun 13, 2014

Properly split long IRC messages
This commit adds handling of long IRC messages to the core. In contrast
to the `splitlong.pl' plugin, multi-byte encoded and recoded messages
are properly split.

To allow for this, a new function has been added to the server struct:
`split_message'. `split_message' returns a string array with the message
splitted to substrings of a length that the server can handle. If a
protocol module doesn't have any limit, it can simply return a singleton
array with a copy of the message.

The `MSG' chat command now calls `split_message' before `send_message',
and emits `message own_public' / `message own_private' with each
substring, so that the string splitting will be visible in the UI.

`split_message' in the IRC module uses `recode_split' which in turn uses
iconv to properly split multi-byte encoded (and recoded) messages.
Add configurable split line prefixes and suffixes
Add settings `split_line_start' and `split_line_end' analogous to
`splitlong_line_start' and `splitlong_line_end' in `splitlong.pl'. The
prefixes and suffixes are concatenated with a wrapper function to keep
`recode_split' and `strsplit_len' simple.
Avoid unnecessary splitting of lines
`split_line_end' could force lines to be unnecessarily split. This
commit fixes the problem by making sure that the last line isn't shorter
than `split_line_end'.
Fix the `userhostlen' fallback in `split_message'
ferret, the author of `splitlong-safe.pl' pointed out that `userhostlen'
should not only contain the maximum length of the hostname, but also the
maximum length of the username. Now 10 is used as the maximum username
length as a fallback. (`splitlong-safe.pl' uses the same limit.)

The username limit isn't defined in the standard, but 10 is common on
many networks. The odds that something goes wrong here is low, as
 1) the fallback limit is only used when the user has not yet joined a
    channel
 2) the maximum hostname length (63) gives some error margin as the
    hostname usually is shorter
Split long IRC `ACTION' messages
Add line splitting logic to commands `/me' and `/action'.
Allow `server.split_message' being NULL
Now a module can set `server.split_message = NULL' to disable message
splitting, instead of having to implement the function.
@ahf

This comment has been minimized.

Member

ahf commented Jul 7, 2014

Very nice work, Sebastian! Let's get this in for some proper testing :-)

ahf added a commit that referenced this pull request Jul 7, 2014

Merge pull request #29 from sebth/master
Properly split long IRC messages

@ahf ahf merged commit 99b629a into irssi:master Jul 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment