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

Document how mentions (pills) work #1547

Merged
merged 5 commits into from
Aug 29, 2018

Conversation

turt2live
Copy link
Member

Rendered: see 'docs' status check


Implements the proposal over at #1067

Includes some specification for how matrix.to is structured, and how it is intended to be replaced.

Closes #1067

Implements the proposal over at matrix-org#1067

Includes some specification for how matrix.to is structured, and how it is intended to be replaced.
@turt2live turt2live requested a review from a team August 22, 2018 04:06
this may be done by changing the background color of the mention to indicate
that it is different from a normal link.

If the current user is mentioned in a message (either by a mention as defined
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we define that client's shouldn't match on formatted_body, and only on body or is that a push rule thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a push rule thing. This particular blob of text is intended to say that client should do something special about mentions, however.

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

The comment on matrix.to is the blocking one for me; the other one is obviously not critical.

----------------

In addition to using the appropriate ``matrix.to URI`` for the mention,
clients should use the following guidelines when making mentions:
Copy link
Member

Choose a reason for hiding this comment

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

Can it be "...when making mentions in events to be sent"? The first time I've read the text, this part looked ambiguous, whether it's about rendering the received mentions or creating mentions to be sent.

++++++++++++++++++++

.. NOTE:
This namespacing is in place pending a ``matrix://`` (or similar) URI scheme.
Copy link
Member

Choose a reason for hiding this comment

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

To address a concern of @non-Jedi in #1067: can we add somewhere that this URI scheme does not necessarily imply a working switchboard at an actual https://matrix.to server and that clients are explicitly discouraged from using matrix.to URIs as ordinary HTTPS URLs when it comes to internal navigation between Matrix resources?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this, it's very easy for client developers to miss that this should ideally provoke an action in the client rather than opening matrix.to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Later on the text explains how the URL is intended to work, is it not sufficient?

I'd rather not have lots of text in notes, as it no longer becomes a note.

Copy link
Member Author

@turt2live turt2live Aug 26, 2018

Choose a reason for hiding this comment

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

I added some additional text to this section to describe how it is supposed to work. The "later on in the text" part of my last comment was referring to the client-server API, which is a fair ways away from the appendices.

Copy link
Member

Choose a reason for hiding this comment

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

i agree that we should spell out here lout and clear on the first line (not as a note) that: "matrix.to" URIs are intended to be used for namespacing, not accessing the webservice at matrix.to (other than for fallback purposes).

Copy link
Member Author

Choose a reason for hiding this comment

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

@ara4n is the improved text (312799a) any better?

Copy link
Member

Choose a reason for hiding this comment

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

i think we need to hoist it higher (e.g. line 262) and be blunter. Putting it at the end makes it seem unimportant, whereas i really think we need to address it head on as otherwise everyone will immediately assume it's a normal URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I noticed that the top level note wasn't rendering in the RST at all, and have fixed that. It also includes a fairly blunt mention that it's not backed by a webservice and to read on for details.

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Thank you!


Examples of matrix.to URIs are:

* Room: ``https://matrix.to/#/!somewhere:domain.com``
Copy link
Member

@ara4n ara4n Aug 28, 2018

Choose a reason for hiding this comment

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

We should list the room alias variants before the room ID variants, and put a TODO or NOTE that the room ID ones are completely insufficient for use as URLs (unless we were to put a ?server=foo.com on them in order to identify a server which should be able to seed that ID, or unless we explicitly parse the domain out of the ID), linking perhaps to element-hq/element-web#2925

Copy link
Member

@ara4n ara4n left a comment

Choose a reason for hiding this comment

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

lgtm, other than wanting to shout louder that matrix.to is just for namespacing here; the web service is not involved and mentions have no centralised dependency on DNS on that webapp, and wanting to acknowledge that room-id based permalinks are totally broken.

Copy link
Member

@ara4n ara4n left a comment

Choose a reason for hiding this comment

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

lgtm now

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

LGTM

@turt2live turt2live merged commit 39ef845 into matrix-org:master Aug 29, 2018
@turt2live turt2live deleted the travis/c2s/pills branch August 29, 2018 15:13
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.

4 participants