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

Add read-marker extension #489

Merged
merged 5 commits into from
Jun 30, 2022
Merged

Add read-marker extension #489

merged 5 commits into from
Jun 30, 2022

Conversation

emersion
Copy link
Contributor

@emersion emersion commented Mar 4, 2022

This is adapted from the soju extension. Ergo has expressed interest in standardizing this extension.

Related, but different: #347

@jwheare
Copy link
Member

jwheare commented Mar 4, 2022

Worth making it more explicit that this is for bouncers or bouncer-integrated servers. The command and cap names are too general at the moment and suggest that this is for read receipts between different users.

@emersion
Copy link
Contributor Author

emersion commented Mar 4, 2022

This isn't just for bouncers, it's for any server supporting multiple connections with the same nick. Or is this what you're calling "bouncer-integrated servers"?

@jwheare
Copy link
Member

jwheare commented Mar 4, 2022

Yes

@emersion
Copy link
Contributor Author

emersion commented Mar 4, 2022

Okay. I don't really like using the term "bouncer" here -- Ergo can't connect to other servers for instance. But yeah, they use the term "bouncer-like features" to describe this in their README. I wonder if we can find a better name to describe this category of servers? Or maybe we could go with a name like self-read?

Copy link
Contributor

@progval progval left a comment

Choose a reason for hiding this comment

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

Nice spec, thanks again.

Name bikeshedding: I'd rather have a more explicit name for the cap, such as draft/read-marker, so it is not confused with a potential draft/read-receipt specification. (And you really need to rename the PR and the spec title, #347 already uses the name "read receipt")

The command name is fine though, as a read-receipt spec would most likely either build upon this command or use client tags.

extensions/read.md Outdated Show resolved Hide resolved
extensions/read.md Outdated Show resolved Hide resolved
extensions/read.md Outdated Show resolved Hide resolved
extensions/read.md Outdated Show resolved Hide resolved
extensions/read.md Outdated Show resolved Hide resolved
@slingamn
Copy link
Contributor

slingamn commented Mar 4, 2022

But yeah, they use the term "bouncer-like features" to describe this in their README.

This is tricky. I used "bouncer functionality" and "bouncer-like features" in the README to describe the combination of three different features:

  1. Server-side chat history
  2. Multiple connections per nickname: the end user guide, operator manual, config files, online help, etc. refer to this specific functionality as "multiclient"
  3. Remaining present on the server in the absence of an active TCP connection: the end user guide etc. refer to this as "always-on"

I think "multiclient" is probably a suboptimal name for this, but I don't really have a better candidate right now.

@DanielOaks
Copy link
Member

I like draft/read-marker, feels like it makes sense.

wrt positioning, some words like this should do the trick: "This is for bouncers or servers providing chat history. It lets chat history providers know what a user has read, and provide that to other clients that user has, so that they can adjust their behaviour accordingly (e.g. not send highlight notifications for messages that have already been read by the user). It's a local read marker, not sent between different users."

@emersion emersion changed the title Add read receipt extension Add read-marker extension Mar 15, 2022
@slingamn
Copy link
Contributor

This spec is great! The "implementation considerations" section was very helpful.

The server MUST reply to a successful MARKREAD client set command using a MARKREAD server command, or using an error message.

As I understand it, this entails that in the case of a a no-op update (i.e. a syntactically valid update with a time earlier than the server's stored value), the server MUST reply (to the originating client, but not to other connected clients) with a MARKREAD server command sending the server's stored value. If this is correct, it would probably be helpful to make it explicit.

Comment on lines 74 to 76
<prefix> MARKREAD <target> <timestamp>

The `prefix` is the prefix of the client the message is sent to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this:

  1. <prefix> is syntactically the source/prefix of the IRC message, correct? e.g. :n!u@h MARKREAD #test 2022-03-31T01:32:21.380Z
  2. I don't understand the purpose of specifying the prefix here. The prefix typically indicates the origin of the message, not the recipient, right? My sense is that this should not be specified, and server implementations should probably use the server name as the source/prefix of these messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally this spec was introducing a READ command, and we figured that the same command might get re-used by a future spec to send read receipts between users. In that case it would've made sense in the new spec to allow the source/prefix to be different from the current user's. Note, other commands like JOIN indicate success by having the server reply with a JOIN message with the current user's source/prefix.

Although I agree it's a bit premature at this point, we're not even sure it's a good idea to re-use the same message for read receipts. I'll drop this requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, thanks. I support dropping the requirement for now.

My sense is that read receipts would be better implemented as TAGMSG.

@slingamn
Copy link
Contributor

I have a draft implementation of this at ergochat/ergo#1926; it's on my staging server at ircs://files.stronghold.network:6697 . I also have a rough irctest covering the server behavior (I'll post that once the implementation is merged).

@emersion
Copy link
Contributor Author

emersion commented Apr 1, 2022

All done!

@slingamn
Copy link
Contributor

slingamn commented Apr 1, 2022

My bad actually, I missed this language in the earlier draft, which is still present and covers my concern about no-op updates:

The last read timestamp of a target SHOULD only ever increase. If a client sends a MARKREAD command with a timestamp that is below or equal to the current known timestamp of the server, the server SHOULD reply with a MARKREAD command with the newer, previous value that was stored and ignore the client timestamp.

so now it's a MUST in one section and a SHOULD in another. Some consolidation might be in order? Either MUST or SHOULD is fine by me.

@emersion
Copy link
Contributor Author

emersion commented May 1, 2022

Hm, it might be handy in the future to allow the timestamps to decrease when the user manually marks some messages as unread. However this should not be done by just accepting blindly MARKREAD commands with a lower timestamp, because of race conditions between clients. So let's switch this to a MUST in this spec. A future spec can always add another way to make the timestamps decrease.

@emersion
Copy link
Contributor Author

emersion commented May 6, 2022

Here's a branch with support for Goguma: https://git.sr.ht/~emersion/goguma/log/read-marker. I will merge it once this spec is merged.

To test, grab the APK from this CI build (once it finishes): https://builds.sr.ht/~emersion/job/752458

@emersion
Copy link
Contributor Author

emersion commented Jun 1, 2022

Gentle ping

@slingamn
Copy link
Contributor

slingamn commented Jun 2, 2022

An implementation of this was released in Ergo 2.10.0. So I think that makes two server implementations (ergo and soju) and two client implementation (gamja and goguma)?

@emersion
Copy link
Contributor Author

emersion commented Jun 2, 2022

Yup! And you can also add senpai to the list of client impls.

@slingamn
Copy link
Contributor

Are there any remaining blockers here for draft merge?

@jwheare
Copy link
Member

jwheare commented Jun 27, 2022

I don't think so. Seems to have a decent amount of approval. Will merge in a few days.

@jwheare jwheare merged commit 20f525e into ircv3:master Jun 30, 2022
@emersion emersion deleted the read branch July 1, 2022 11:33
emersion added a commit to emersion/soju that referenced this pull request Jul 1, 2022
kkartaltepe pushed a commit to kkartaltepe/gamja that referenced this pull request Sep 5, 2022
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

5 participants