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

MSC2773: Room kinds #2773

Closed
wants to merge 3 commits into from
Closed

MSC2773: Room kinds #2773

wants to merge 3 commits into from

Conversation

turt2live
Copy link
Member

@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal labels Sep 11, 2020
proposals/2773-room-kinds.md Outdated Show resolved Hide resolved
proposals/2773-room-kinds.md Outdated Show resolved Hide resolved
proposals/2773-room-kinds.md Outdated Show resolved Hide resolved
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
@bwindels
Copy link
Contributor

bwindels commented Oct 8, 2020

Exciting to see work on this happening! One point of early feedback: it is somewhat unclear still whether the room kind would be immutable or not, but I would very much argue that it should be, as explained on MSC 1840. If we want the DM-ness of a conversation to be mutable, that would mean using a different flag...

@turt2live
Copy link
Member Author

One point of early feedback: it is somewhat unclear still whether the room kind would be immutable or not, but I would very much argue that it should be

@bwindels good news! I've made it mutable in the most complicated way possible.

@turt2live turt2live changed the title [WIP] MSC2773: Room kinds MSC2773: Room kinds Oct 20, 2020
@turt2live turt2live marked this pull request as ready for review October 20, 2020 02:25
@turt2live
Copy link
Member Author

I've put this up for review - please leave comments on the diff.

@turt2live turt2live added this to Awaiting SCT input in Spec Core Team Backlog Oct 20, 2020
about instead of persisting the whole event, though in practice this is not how clients operate.

3. **Clients sometimes forget or miss state events**. During normal operation, a client might hit a point
where a state event ends up in a gap over `/sync`, or might simply have storage reliability issues

Choose a reason for hiding this comment

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

Servers make sure that all state event updates are delivered to the client using the state field in the /sync response

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, no state events should be missed during a limited sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's quite a few clients out there which don't use the field, unfortunately.

* `m.conversational` - General conversational room. This is the default value.
* `m.protocol` - A "protocol" room where the information contained within is not intended to be consumed
by humans. Further identification of the room is left to the individual functionality of the room. For
example, a profile room might be identified by the presence of a `m.profile` state event.

Choose a reason for hiding this comment

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

Just some ideas for alternative (and shorter) names:

  • m.chat
  • m.talk
  • m.magic
  • m.meta

Choose a reason for hiding this comment

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

@timokoesters Why not a suggest a change?

* `m.conversational` - General conversational room. This is the default value.
* `m.protocol` - A "protocol" room where the information contained within is not intended to be consumed
by humans. Further identification of the room is left to the individual functionality of the room. For
example, a profile room might be identified by the presence of a `m.profile` state event.

Choose a reason for hiding this comment

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

a profile room might be identified by the presence of a m.profile state event

This could be problematic because a bad client might accidentally send that event into the wrong room, maybe breaking it for everyone else and there's no way to revert it because state events can't be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't be broken? It would just become a case 3 room, or ignored.

non-conversational data might be present. Typically this could be avoided by using "protocol"
(non-conversational) rooms only on dedicated accounts, however this isn't always possible. MSC1840
solves the problem by specifying a state event which indicates a type of room with an opportunity to
filter them out.

Choose a reason for hiding this comment

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

Do I understand it correctly, that the problem is that non-conversation events are sent to "conversation clients", leading to unnecessary networking and computations.

This problem seems like it can be solved without typed rooms by simply defining a new state event that is "opt-in" (as opposed to opt-out like filters work currently).

Choose a reason for hiding this comment

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

The problem of "but why do I have all these rooms with nothing happening in them?" could be solved by improving the /createRoom to automatically create low-priority/hidden rooms when a new flag in the request is set. These rooms can then still be used to communicate with the user (e.g. show errors that the drone outputs,or "battery low" notifications)

Rooms have their `m.kind` baked into the room state in one of two ways:

1. As `m.kind` in the `m.room.create` event's `content`.
2. As an `m.room.kind` state event in the room, with `content` of `{"m.kind": "m.protocol"}` and empty string
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I still think this is putting too much complexity on the clients, and clients might implement this inconsistently (e.g. only support the kind being transitioned for certain kinds, and assume some kinds like profiles, ... will always be of the immutable type).

Instead, why not have a main immutable kind through the m.room.create event and have a mutable subkind (where applies) through a different state event? This could for example be used to set a room as a DM or not with kind of m.conversation and m.kind.sub (horrible name) of DM

Using a "subkind" would ensure that transitions are only possibly within a given kind. This:

  • seems easier for clients to implement
  • seems easier to explain and understand
  • might also force us to think more in advance whether something should be a kind or a subkind wrt to future extensions... Although if you implemented something as a kind and you want to be able to transition into something new, you could just define the default subkind as "oldthing" with the possibility of transitioning it to "newthing", if that makes any sense.

Alternatively, you could have two unrelated "kinds", one mutable and one immutable.


2. **State events are heavy for clients**. Each state event a client has to keep track of is lost memory,
storage, and CPU time to store a simple flag. Instead of expecting clients to haul around the heavyweight
state events (like `m.room.create` and `m.room.kind`), we can let them get on by just storing the
Copy link
Contributor

Choose a reason for hiding this comment

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

The m.room.create event seems to be one of the smaller state events though? Not sure I find any of the stated reasons compelling enough to create a new field in the summary rather than just putting it in the m.room.create event which all clients already should store. State events are an integral part of the protocol and starting to provide a denormalized view on top of it is not completely new but the only precedent I can think off (summary for lazy loading) has a way stronger argument for it IMO.

about instead of persisting the whole event, though in practice this is not how clients operate.

3. **Clients sometimes forget or miss state events**. During normal operation, a client might hit a point
where a state event ends up in a gap over `/sync`, or might simply have storage reliability issues
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, no state events should be missed during a limited sync.


### Possibility 2: Hybrid rooms (preferred by this MSC)

Instead of separate rooms for a group and moderation policy, it is possible to use a common `m.kind`
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the same as option 1 and just comes down to what kinds you define and what state events are expected to be found in a given kind?

In any case, as a client developer, I would much prefer option 1/2 over option 3.


Specified types are currently:

* `m.conversational` - General conversational room. This is the default.
* `m.conversational` - General conversational room. This is the default value.
* `m.protocol` - A "protocol" room where the information contained within is not intended to be consumed
Copy link
Contributor

Choose a reason for hiding this comment

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

As a client developer, it would at least be nice to have distinct kinds for use cases where chronological information of events needs to be kept track off, or whether the room is just a key-value store through state events. m.protocol might be too broad to ensure the latter can be assumed for all use cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

A custom kind is always possible. What are the use cases for a chronological store?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, IoT sensor data rooms that you don't want to show up in your room list?
But my point was more that m.protocol seems overly broad and I would expect other MSCs rather than this one to define the (more specific) kinds where needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, the general idea is that this MSC defines something generic for people to use and that indeed other MSCs would be responsible for their own kinds, if needed.

Copy link
Member Author

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

GitHub mobile is awful to review MSCs and requires me to provide this comment.

Comment on lines +62 to +63
as minimally as possible over `/sync`. IoT devices typically achieve this with a strict filter, which
would almost certainly exclude the expensive state events. Similarly, IoT devices typically want to
Copy link
Member

Choose a reason for hiding this comment

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

surely if they are interested in m.room.kind events, they shouldn't filter them out? What am I missing?

Comment on lines +67 to +71
2. **State events are heavy for clients**. Each state event a client has to keep track of is lost memory,
storage, and CPU time to store a simple flag. Instead of expecting clients to haul around the heavyweight
state events (like `m.room.create` and `m.room.kind`), we can let them get on by just storing the
purpose for the room and moving on. A client could theoretically rip out the information it cares
about instead of persisting the whole event, though in practice this is not how clients operate.
Copy link
Member

Choose a reason for hiding this comment

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

... isn't that a thing that could be easily fixed?


## Proposal

In the `summary` of `/sync`, a field named `m.kind` is introduced. Like other fields in the summary,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In the `summary` of `/sync`, a field named `m.kind` is introduced. Like other fields in the summary,
In the `summary` field of the response to [`/sync`](https://matrix.org/docs/spec/client_server/r0.6.1#get-matrix-client-r0-sync), a field named `m.kind` is introduced. Like other fields in the summary,

Comment on lines +110 to +113
As mentioned, MSC1840 solves a similar problem. This MSC might work well in conjunction with it rather
than against it, though this MSC does define some potential downfalls of MSC1840. The downfalls are
niche and limited in practicality, though may be extremely important for the potential use cases of
a system like this outside of groups-as-rooms, profiles-as-rooms, etc.
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the main way that this proposal differs from MSC1840 is that the server is expected to map from the state event to an extra m.kind field in the sync summary.

I'm yet to be convinced of the value of doing so, but could we just reframe this MSC as exactly that: an extension to MSC1840 and move all the discussion about which types/kinds we need over there?

Copy link
Member

Choose a reason for hiding this comment

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

it seems that this is not an accurate understanding of the differences between this and #1840. The core difference is in the number of available "types/kinds".

In this MSC, there are exactly three possible kinds:

  • m.conversational - Generic chat room
  • m.conversational.dm - Part of the effort to make DMs a first-class citizen
  • m.protocol - All the things that shouldn't be in a room list (including all the X-as-rooms rooms)

... whereas MSC1840 allows much more flexibility.

The addition of data to the summary is an orthogonal problem which might be better considered as a separate MSC extending either this MSC or MSC1840.

@ara4n ara4n mentioned this pull request Oct 21, 2020
@turt2live
Copy link
Member Author

I'm not confident in this as an author - closing.

@turt2live turt2live closed this Oct 21, 2020
@turt2live turt2live added abandoned A proposal where the author/shepherd is not responsive and removed proposal-in-review labels Oct 21, 2020
@turt2live turt2live removed this from Awaiting SCT input in Spec Core Team Backlog Oct 21, 2020
@turt2live turt2live deleted the travis/msc/room-kinds branch October 21, 2020 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned A proposal where the author/shepherd is not responsive kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants