matrix-org / matrix-spec

The Matrix protocol specification
Apache License 2.0
188 stars 94 forks source link

set_presence wording is unclear #1609

Open DMRobertson opened 1 year ago

DMRobertson commented 1 year ago

Link to problem area:

Issue

From the second link:

https://github.com/matrix-org/matrix-spec/blob/8b51f1c0110384a970b777afdc11d76b17f4e97d/content/client-server-api/modules/presence.md?plain=1#L17-L25

This makes it sound like presence is a three-valued enum.

From the first link:

https://github.com/matrix-org/matrix-spec/blob/45b6aaf07ae119a0e284192d6de8a13cf668d06f/data/api/client-server/sync.yaml#L91-L105

This describes three query parameter key-values pairs: set_presence=online, set_presence=offline, and set_presence=unavilable. The wording for offline is particularly confusing:

Otherwise if the parameter is set to "offline" then the client is not marked as being online when it uses this API.

This sounds like set_presence=offline is effectively a no-op that doesn't alter the current present state of the user.

It is hard to reconcile these two views. I see two possibilities:

  1. set_presence=offline really does set your presence state to offline. If this is true,
  2. set_presence=offline is a no-op on your presence state. If this is true,
    • the query parameter name is misleading here,
    • the description of set_presence is slightly confusing, and
    • there is no way to set your presence state to offline using /sync.

Both options seem confusing and inconsistent.

I would like to understand which of these is true and update the spec to make this clearer.

DMRobertson commented 1 year ago

Synapse's logic in /sync is here: https://github.com/matrix-org/synapse/blob/d0c4257f14addbf0c9072c2e34ae1c8294716ed5/synapse/rest/client/sync.py#L204-L210

Digging in, if affect_presence is falsey then the function does nothing: see https://github.com/matrix-org/synapse/blob/199c2709479a833e0dc01d19773031c3d5fa63fb/synapse/handlers/presence.py#L973, https://github.com/matrix-org/synapse/blob/199c2709479a833e0dc01d19773031c3d5fa63fb/synapse/handlers/presence.py#L474 and https://github.com/matrix-org/synapse/blob/199c2709479a833e0dc01d19773031c3d5fa63fb/synapse/handlers/presence.py#L155-L172

So Synapse votes for option 2.

DMRobertson commented 1 year ago

Dendrite seems to vote for Option 1, looking at https://github.com/matrix-org/dendrite/blob/4594233f89f8531fca8f696ab0ece36909130c2a/syncapi/sync/requestpool.go#L248 and https://github.com/matrix-org/dendrite/blob/4594233f89f8531fca8f696ab0ece36909130c2a/syncapi/sync/requestpool.go#L125

DMRobertson commented 1 year ago

Not sure about conduit: the only match for set_presence in sync code is a TODO: https://gitlab.com/famedly/conduit/-/blob/d2bfcb018ed40eb91b6f7dd31727e24f2b992727/src/api/client_server/sync.rs#L174

DMRobertson commented 1 year ago

Related: https://github.com/matrix-org/synapse/issues/16039