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

CHATHISTORY TARGETS #450

Merged
merged 4 commits into from
Jun 2, 2021
Merged

CHATHISTORY TARGETS #450

merged 4 commits into from
Jun 2, 2021

Conversation

slingamn
Copy link
Contributor

@slingamn slingamn commented Apr 6, 2021

This comes from discussion on #444 (cc @kylef, @emersion, @hhirtz). In short:

  1. CHATHISTORY needs a way to surface DMs with previously unknown users. The previous proposal for this was the * target (which "contained" all DMs sent to or from the user).
  2. Client developers don't like the * target; it has the problem that high-priority DMs from friends can get blocked behind spam
  3. Bouncer developers don't like the * target; in a typical bouncer implementation, there is no global time index over all DMs (nor any need for one)

So, this proposal adds an explicit way to list DM correspondents, with pagination support, and removes the * target. It's marked as a draft; the intent is that Oragono 2.6.0 will include some version of this API, but will continue to include the * target for the time being.

1. Remove the * target
2. Add LISTCORRESPONDENTS
Always include the target in the CHATHISTORY pseudo-invocation
@slingamn
Copy link
Contributor Author

slingamn commented Apr 6, 2021

Summarizing some discussion, the tentative plan is:

  1. The subcommand should be LIST_TARGETS (LISTTARGETS?) and the response subcommand should be TARGET
  2. Channels should be included in the output

@slingamn slingamn changed the title draft of CHATHISTORY LISTCORRESPONDENTS draft of CHATHISTORY TARGETS Apr 8, 2021
slingamn added a commit to ergochat/irctest that referenced this pull request Apr 15, 2021
CHATHISTORY * is being removed here:

ircv3/ircv3-specifications#450

znc.in/playback *self was an Oragono extension that didn't catch on:

ergochat/ergo#1205
progval pushed a commit to progval/irctest that referenced this pull request Apr 17, 2021
CHATHISTORY * is being removed here:

ircv3/ircv3-specifications#450

znc.in/playback *self was an Oragono extension that didn't catch on:

ergochat/ergo#1205
@slingamn slingamn changed the title draft of CHATHISTORY TARGETS CHATHISTORY TARGETS Apr 18, 2021
@slingamn slingamn marked this pull request as ready for review April 18, 2021 20:43

Unlike the other subcommands, `TARGETS` does not return message history. Instead, it lists channels the user has visible history in, together with users with which the user has exchanged direct messages, ordered by the time of the latest message in the channel history or direct message conversation (i.e. sent from or to the other user). This allows the client to discover missed direct message conversations with users it is not currently aware of.

CHATHISTORY TARGETS <timestamp=YYYY-MM-DDThh:mm:ss.sssZ> <timestamp=YYYY-MM-DDThh:mm:ss.sssZ> <limit>
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the time here the "until" timestamp is the current timestamp, would it make sense to allow the client to send a placeholder so it does not need to figure out the server's current time?

CHATHISTORY TARGETS <timestamp=YYYY-MM-DDThh:mm:ss.sssZ> * *

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be careful wrt. race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that the most common case of this is analogous to CHATHISTORY LATEST: list all targets starting from the current time and paginating backwards, possibly until a lower bound is reached (estimated from the time of the last message received on the connection).

Here's what I thought: unlike the other commands, TARGETS can only handle timestamps, not message IDs (because the spec is designed for compatibility with systems where there is no global ordering on message IDs). In this context, it makes sense to simplify the specification of the command as much as possible --- a single command, with parameters of a single form.

The intended way to express the current time is as in the pseudocode example: take the local current time and add some constant.

Copy link
Contributor

@emersion emersion May 18, 2021

Choose a reason for hiding this comment

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

My recommendation is to use the time tag from the last received message from the server as the upper bound (e.g. from RPL_WELCOME). That way, if another user decides to send us a PRIVMSG while we're still in the process of fetching chat history, we won't get the message twice. Additionally, this side-steps clock synchronization issues between the client and the server.

Copy link
Contributor

Choose a reason for hiding this comment

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

My recommendation is to use the time tag from the last received message from the server as the upper bound (e.g. from RPL_WELCOME).

The welcome can be replayed with older timestamps than the current connection. For example with the ZNC bouncer, the time attached to 001/005/MOTDs numerics can be the time in which the ZNC server received the message from its upstream server. This can be days, months, years before the current server time. I don't see a way I can trust any time message from connection handshake as they are sometimes replayed in parts, and cannot be treated as "current" server time.

For the "until time" in this command, I don't see as a client we'd have anything to put here other than making up a date in the servers future time which isn't anymore useful than the server using its own time upon receiving the message (because at this point its the only part of this that knows the server's time).

Copy link
Contributor

Choose a reason for hiding this comment

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

For example with the ZNC bouncer, the time attached to 001/005/MOTDs numerics can be the time in which the ZNC server received the message from its upstream server.

Sounds like a ZNC bug to me.

emersion added a commit to emersion/soju that referenced this pull request May 18, 2021
@emersion
Copy link
Contributor

I've implemented this in soju and in gamja. I'm pretty happy with the client-side implementation, and can confirm that sending both nicknames and channels was a good call.

emersion added a commit to emersion/soju that referenced this pull request May 25, 2021
@slingamn
Copy link
Contributor Author

@jwheare motion to merge this? It now has a server and a client implementation, and we didn't find any spec bugs.

Right now, no one is implementing the * target (which is removed by this PR), so it would be nice to update ircv3.net to remove it before someone tries to implement it.

@hhirtz
Copy link

hhirtz commented May 27, 2021

I'll also implement it in senpai, I just need to finish the work on soju.im/bouncer-networks first since soju doesn't support this spec in multi-upstream mode (one connection to the bouncer to show all upstream channels/users)

@jwheare
Copy link
Member

jwheare commented May 28, 2021

Sounds good. I'm guessing no one thinks a version bump is necessary? Any other backwards incompatible changes other than removing the * target?

@slingamn
Copy link
Contributor Author

That should be the only backwards-incompatible change. (This change also improves compatibility with the published chathistory batch type spec, since there is no longer any ambiguity about the value of the BATCH parameter.)

+1 for merging with no version bump.

@emersion
Copy link
Contributor

I'm fine with merging without a version bump, even if it's a backwards-incompatible change.

@jwheare jwheare merged commit 516f6ed into ircv3:master Jun 2, 2021
emersion added a commit to emersion/soju that referenced this pull request Jun 2, 2021
k4bek4be pushed a commit to k4bek4be/gamja that referenced this pull request Jul 4, 2021
The main motivation is to avoid missing direct messages coming from
other users.

A nice side-effect is that we no longer need to issue CHATHISTORY
queries for each channel we JOIN: instead, we can only fetch
history for targets known to have new messages available (as indicated
by CHATHISTORY TARGETS).

We use read receipts instead of delivery receipts, so that reloading
the webapp restores the exact same state (ie, unread messages are
re-fetched).

References: ircv3/ircv3-specifications#450
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.

5 participants