Formatting options feature

[llmII]

This adds initial support for formatting options for messages towards Discord (for the case where join/quit/part is shown) and towards IRC (when in simple mode).

Some of this raises a question how to document it (I don't think it quite makes sense to try to fit it into the table).

Currently my #75 and #72 feature branches would need updates to support a few more format keys here.

Please let me know how to move forward with this.

[llmII]

Realized viper's set defaults doesn't apply that well for sub-keys seemingly. I'll do this without relying on viper's default setting capability.

[llmII]

See this for information on what needs doing.

Done in 4cc8b30.

[llmII]

If this is commited, would we need show_joinquit any more? Could basically default all the formats to " " and that means disabled. Then granularly enable by giving it a format. I think irc_format would need a non-blanking default but the others could have a blank default. This means for #72 no need to change show_joinquit to something else if this is merged. It also means that merging #75 and #72 would need a merge of #79 and #78. We'd also gain a "Disable PM's" switch in the process. That said PMs would need a default-non-blanking as well.

[llmII]

We probably should review the default formats for things where possible as well. I think for most things where we're using the hostmask we could drop it or use ${DISCRIMINATOR} instead. It's nice to have the option to show more information, but I don't know that it makes sense to show the entire IRC hostmask since we don't accept commands like "!op" or "!ban" Discord side with effects on IRC side.

That said, I think it would be best to discuss the default formats but commit this as is and handle all formatting options for all merged formatting related things in another PR.

[qaisjp]

Can we make m.formatIRCMessage return a slice (so that it only needs to be passed msg)?

[llmII]

You mean move m.bridge.ircListener.Privmsg(channel, m.formatIRCMessage(msg, line)) out of the "for range" and have it just take msg? Probably better to do it that way actually, instead of formatting per channel. Could you explain what you mean by return a slice however?

[llmII]

Wait, rereading the code, but basically the for range wrapping this splits the line on '\n' and that's why I made it take a line argument. The function is to format lines, not to split lines out and format them. That said the formatting works on things besides lines anyway so still moving that outside of the for range and letting it then be split on '\n' works too I would think.

[llmII]

Thinking through this more, it wouldn't work, formatIRCMessage would need to be aware of new lines '\n' and apply each line as content.

Say the format is "\x02${USER}\x02 ${CONTENT}" - if we treat the entire msg.Content as the singular content then only the first line will have the "\x02${USER}\x02" part and the rest of the lines will have no prefix of the user.

That or I'm misunderstanding what is wanted to be done...

The reason formatIRCMessage needs to remain unaware that it may be handling more than one line is if I add support for including the referenced content in the message (for edit/reply) then this will also be applying for IRCConnections when pushing into their messages channel.

[qaisjp]

I think it would be good to return an error if an unknown key is present in the map.

• If it's the first load, we can use the error to fatally exit: log.WithError(err).Fatal("discord_format setting is invalid")
• If it's a live reload, use the error to print an error message (log.WithError(err).Error("discord_format setting is invalid, this setting has not been updated")), and ignore the change (don't assign to dib.Config.DiscordFormat)

[llmII]

You mean log that format setting is invalid and if there isn't an old format setting use default but if there is use the old format?

[qaisjp]

Kind of, I meant changing the return type of setupDiscordFormat to (map[string]string, error). And if there's no old format setting, i.e. at the very start, prevent the bot from starting (hence, "fatal").

[llmII]

Hmmm, fatal first load exit. This wouldn't allow for the show_joinquit to be supplanted though and for a default format of " " to basically be "disable" and anything else to be enable that line. Let me know what you think you'd really prefer here, because if #72 and #79 get merged then show_joinquit doesn't exactly make sense as the configuration setting name any longer and being able to granularly enable/disable what events you want to write towards Discord might be a good thing.

[qaisjp]

This would only be if an unknown key is present in the config, i.e. if you accidentally type "quiiit" instead of "quit".

[llmII]

Ah, that would be fine I think, I'll see about detecting unknown keys and propagating an error.

[llmII]

Have added code that should do basically what is mentioned here.

[llmII]

Since #75 was merged need to hold up on this and merge in the work from #78.

[llmII]

#78 is merged into this feature now, closing PR #78. Note that it's not yet tested though.

[llmII]

Hold tight, #79 will be merged into this soon.

[llmII]

This is now untested, depends on #72 and #95.

EDIT:
Tested, works as it should.

[qaisjp]

Merge conflict :)

[llmII]

This one likely still needs a discussion about defaults.

[qaisjp]

I'd still like the default to be off (empty string), so maybe we should move this to config.yml, but commented out.

Additionally, please could you try to match the original format that was created programmatically, if possible? The original ones were designed to match irccloud's output 1:1, which I believe to be a reasonable default for good UX.

[llmII]

Because I hate typing all caps and special symbols pretend the following are the interpolators...

$n == ${NICK}
$d == ${DISCRIMINATOR}
$i == ${IDENT}
$h == ${HOST}
$t == ${TARGET}
$c == ${CONTENT}
$s == ${SERVER}

Now may be a good time to discuss what we want the interpolators to actually be. I'm not picky, and seeing as I only must type them once, the super-long-all-caps ones are okay with me. Discussing them and having to reference them is a pain though.

For join, part, quit, kick, nick I can definitely supply commented out config.yml entries for them that match the original ones before formatting. I actually think the only thing I changed was adding in surrounding '_' for italics and possibly changing the separator between content and metadata to be '-' (I always find that more visually appealing, the '-', and I'm going to at this moment guess I changed that though it may be part of mirroring the original).

For pm we run into an issue. I see 3 options.

1. Drop metadata ($i$h$s) out and use "$n@$d: $c"
2. Keep and find a way to format all the data that is appealing and makes it easy for a person to figure out how to reply...
3. Complicate PM parsing and create a new way for discord users to reply that is compliant with whatever we come up with here as the default.

So, 1st question - Do you want PM to default to "" (empty) (I don't think so but good to figure this out now).

An example of 2 and why it leads to 3 is... consider "$n/$d!$i@$h: $c"... Looks like a good idea but I've seen pylinks use '/' in nicks. So even formatting appealingly will lead to 3 unless we pick a symbol that is impossible to be found in a hostmask to set as a seperator between $n and $d and from there we need to change PM reply parsing to match so it's a simple "copy/paste" vs having to type and pick and figure out which part is which. If we went with '/' then PM reply parsing would need a heuristic or a "try few different ways until truly fail" both of which would lead to suprises when you have "llmII/pylink/actual_network!ident@host" and "llmII/same_actual_network!ident@host". I'd also argue for dropping out $s (why would that be needed any more with network discriminators?) $s has the potential to leak information if the puppet connects to a "hidden" server over the public internet to some degree.

[qaisjp]

In what way are you personally planning to use the PM format? It seems like an outlier, and does not seem to be necessary for configuration, from my point of view.

[llmII]

		msg := fmt.Sprintf(
			"%s,%s - %s@%s: %s", e.Connection.Server, e.Source,
			e.Nick, i.manager.bridge.Config.Discriminator, e.Message())

Right now is how it looks so the output would look like (for every message)
irc.bl.ah,llmII!llmII@llmII llmII@blah: msg I sent

The way I would end up using it in the long run is to strip off the part before the first space, bold the part before the colon:

"llmII@blah: msg I sent" would be the end result.

I understand for the most part italics/bold isn't wanted as a default (so am quite ok with changing the format for myself).

I still view the server part,nick as a lot of text on each line, and the server part as a possible "leak" dependent upon if the local discord bridge is remote to the IRCd and for instance the discord bridge was connecting direct to a hub. As per above, figuring out how to include nick,ident,host and discriminator all together in an appealing format is difficult barring finding a truly special character that IRC will not allow in nicks. I'd say using '*' or '?' would work possibly but might be a little befuddling to those who understand hostmasks ban patterns.

[qaisjp]

Now that we have discriminator, we can remove the server part from the default.

Bolding seems reasonable and a good idea.

Generally I'm not strongly tied to it having ident and host. The reason I added it is so users have full information about who they are talking to — maybe that could be replaced with a 🔒 or ⚠️ symbol that indicates if someone is identified.

Maybe we could also implement Slash commands and provide a separate /whois command service.

[llmII]

Now that we have discriminator, we can remove the server part from the default.

Ok, so change to a more appealing statically configured one.

Bolding seems reasonable and a good idea.

Awesome, that nearly nixes my needs for formatting.

Generally I'm not strongly tied to it having ident and host. The reason I added it is so users have full information about who they are talking to — maybe that could be replaced with a lock or warning symbol that indicates if someone is identified.

Tracking who is identified or not could be an easier said than done scenario. IIRC charybdis and ircu used different numerics to indicate this (and most ircu derivatives track identity separate from nick)... However...

Maybe we could also implement Slash commands and provide a separate /whois command service.

If these were to be implemented would work out quite well I believe.

Maybe until user identified support or slash commands support is in go-discord-irc something like...
"llmII@discriminator ([email protected]): msg I sent" could be done static to keep it where a user knows who they are talking to? (Can change formatting, I just tend to use bold/italics where I figure it'll help the eye skip over the part the person doesn't always care about).

One reason to keep PM's formattable (albeit perhaps with a static format instead of allowing it to be read from a config for now) is to keep handling formatting of messages all in one place (eventually).

The only other reason we would want to continue with support for format strings for PM's is if we were to support format strings for edit/reply which possibly should be a case of "when/if that happens or not, then return to discussion about it". That said, the reason behind that is the flow in my mind for handling such is...

1. get referenced line
2. format referenced line
3. format content with ${REFERENCE} interpolator and an argument to the formatter that is the string content of the referenced line.

Which would of sorts obviate the need for formatting to be applied at the PM level.

With all that said, would the approach to keep it formattable, but with a static formatter for the time being be a good solution for now? No point in having a configurable option without the other having ever been created.

I'm also not yet completely certain I want replies with referenced content. I can see the benefit (mimics Discord of sort) but I can see it being annoying in some instances as well. Where in IRC one might do "this is a message (re: this is what is replied to)" or "this is a message (re: this is what...)" a Discord user would never do that (would put the referenced content in above the message they sent and in the content additionally so a lot of duplication of referenced content). The captured content only becomes useful if more than a few people are discussing several subjects simultaneously and a reply to a line doesn't match the "last 3 lines of subject" of sorts. I'm still thinking on if it's really that important and worth pursuing or would 2 more lines of "what" and the reply to "what" as to what was actually being replied to isn't that big a deal.