Skip to content
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

Allow history playback of arbitrary events (channel join/part, quits, etc) #362

Closed
wants to merge 6 commits into from

Conversation

DanielOaks
Copy link
Member

@DanielOaks DanielOaks commented Feb 12, 2019

The diff is a bit screwy because of the rename, the changes can be seen here though.

Brought up and discussed fairly extensively in #293, there are some events that servers may wish to replay as part of the chathistory batch. I know that in Ora, we want to replay joins and parts as they can be useful for working out context.

Right now, the chathistory batch only supports PRIVMSG and NOTICE messages, which makes sense. This spec tries to extend the batch so that clients who support it can be sent any abritrary messages as part of the batch.

At the same time, we want to make totally sure that clients aren't going to run into some use case and accidentally parse an old message as something new. Because of this, as recommended by @dequis in the linked issue, we replay these events as a new message called CHEVENT (chat history event). Doing it in this way simplifies client implementations because they can simply catch the CHEVENT message and go on to displaying the contents, and it also means that clients have no way of accidentally parsing a replayed event as a real one no matter what strange edge cases they run into.

Feedback would be much appreciated!

@RyanSquared
Copy link
Contributor

I think there should be a point about clients being able to either handle or ignore verbs that they don't understand, because I'm quite sure a client author will forget about possible cases they missed otherwise. For example, someone could forget about AWAY or CHGHOST - or not interact with them at all - when writing clients.

@DanielOaks
Copy link
Member Author

DanielOaks commented Feb 12, 2019

Suresure, makes sense

edit: Resolved, thanks for the yell!

@slingamn
Copy link
Contributor

Sorry I'm a bit late to this: we're talking about making CHATHISTORY a cap for #306, so are there any downsides to folding this directly into the CHATHISTORY cap?

@DanielOaks
Copy link
Member Author

I think it makes sense for event playback to be its own very explicit capability, yeah. Getting PRIVMSG/NOTICE from played-back history is really standard and makes a lot of sense (heck, bouncers have been doing that for decades), but getting any sort of generic messages in playback (TOPIC/etc) is a pretty new feature. Having it be a specific capability, and also using the CHEVENT or a similar message to prevent clients from accidentally interpreting incoming replayed messages as real ones is a decent compromise. Right now I don't see a way of rolling it nicely into a more general chat-history capability which meaningfully simplifies things for clients.

@dequis
Copy link
Contributor

dequis commented Feb 23, 2019

So, given that this is a new cap, shouldn't it be a standalone spec in its own file instead of changes to a batch type?

@DanielOaks
Copy link
Member Author

DanielOaks commented Feb 23, 2019

It's a new cap that directly changes the batch type. We could have it as a separate spec but it would point towards the chathistory batch spec, and just the same the chathistory batch spec would point towards this spec as well. Feels like it could be more confusing to implementers. I'm open to it, but would like to hear other peeps thoughts before going through with it.

@SadieCat
Copy link
Contributor

FWIW the reason I called this batch chathistory rather than history was so that a separate history spec could be created if people wanted something with full playback later.

@DanielOaks
Copy link
Member Author

I'll make this a separate batch type and capability. tl;dr if the client's negotiated the history (or whatever) capability, and a case exists where the server would send the client a chathistory batch, it may instead send it a history batch which includes HEVENT messages (still doing that to make absolutely sure they aren't parsed raw no matter what happens to the batch framing or whatever).

@DanielOaks DanielOaks changed the title Add event-playback to the chathistory batch Allow history playback of arbitrary events (channel join/part, quits, etc) Apr 3, 2019
@DanielOaks
Copy link
Member Author

Alright, chathistory batch type put back to stock. The history batch type (enabled with the event-playback capability) is a separate batch type that can also contain HEVENT messages. The reason why HEVENT messages are used instead of anything else is to ensure that clients parse them as historical messages, even if the batch framing gets stuffed up and even if their clients are hardwired to read all incoming messages and just do actions and modify state based on the verb.

@slingamn
Copy link
Contributor

slingamn commented May 5, 2019

Suggestion: if you attach a msgid to a JOIN or PART, you should be able to attach the same msgid to the HEVENT that replays it.

@jwheare
Copy link
Member

jwheare commented May 5, 2019

That’s probably covered already by the msgid spec

However, if a message is re-transmitted as-is, for example with the chathistory batch type, the ID SHOULD be reused. As a result, clients MUST accept shared IDs.

@DanielOaks
Copy link
Member Author

Yeah I'd consider that to be correct behaviour. I'll add a note to the spec stating it explicitly though (just as 'any tags on the original message SHOULD be supplied as-is on the HEVENT message replaying it', or something like that).

:irc.host BATCH +sxtUfAeXBgNoD history #channel
@batch=sxtUfAeXBgNoD;time=2015-06-26T19:40:31.230Z :foo!foo@example.com PRIVMSG #channel :I like turtles.
@batch=sxtUfAeXBgNoD;time=2015-06-26T19:43:53.410Z :bar!bar@example.com NOTICE #channel :Tortoises are better.
@batch=sxtUfAeXBgNoD;time=2015-06-26T19:44:23.423Z :bar!bar@example.com HEVENT PART #channel :Tortises for life!
Copy link
Contributor

Choose a reason for hiding this comment

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

This typo may be intentional but it is somewhat distracting ;-)

@slingamn
Copy link
Contributor

I just noticed that TAGMSG is specified as requiring a HEVENT instead of TAGMSG. What's the rationale for that?

@prawnsalad
Copy link
Contributor

As discussed in #ircv3 yesterday, this whole HEVENT verb not only complicates parsing on the client side and is very odd, but is also redundant. Clients must opt-in to see this batch type so they will already know to only use them for display purposes.

When it comes to parsing the batch, there is no difference vs parsing chathistory which means most logic will simply be if(batchType === 'chathistory' || batchType === 'history') { handle both }. This combined with the fact that both history and chathistory batch types serve the same purpose - to replay conversation history - and also formatted the same, why not simply have this event-playback cap as the opt-in to extend the chathistory batch with events? The end result bing a more concise and simpler history spec.

@slingamn
Copy link
Contributor

I propose that the suggestion in the above comment should be adopted, and the event-playback capability should be folded into #393.

@DanielOaks
Copy link
Member Author

yeah, it'd be best to handle it that way. totally agree that HEVENT makes things annoying for no good reason really.

@slingamn
Copy link
Contributor

I believe this is fully absorbed into #393 at this point and can be closed.

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.

None yet

7 participants