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

MSC3973: Search users in the user directory with the Widget API #3973

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dhenneke
Copy link

@dhenneke dhenneke commented Mar 1, 2023

…rectory

Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
@turt2live turt2live added proposal A matrix spec change proposal kind:feature MSC for not-core and not-maintenance stuff widgets anything to do with widgets needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Mar 1, 2023
@dhenneke dhenneke marked this pull request as ready for review March 1, 2023 18:13
Comment on lines +11 to +14
specification. It should provide the same features that the
[“User Directory search” endpoint](https://spec.matrix.org/v1.6/client-server-api/#post_matrixclientv3user_directorysearch)
of the Client-Server API provides. It should further provide the same results as the invite dialog of
the hosting client (e.g. Element) so the same search terms lead to the same results in all components.
Copy link
Author

Choose a reason for hiding this comment

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

There are two options to solve this:

  1. Just return the response from the CS-API.
  2. Return whatever the client is doing to provide the user list in their invite dialogs (and injecting them transparently into the results list without the widget noticing).

For 2., Element does many additional things that is currently implemented in the InviteDialog. If we would want to provide the client-enhanced list instead of the raw server response, this would require refactoring in the react-sdk. But for the long term (if MSC3008 becomes a reality) it might be questionable how useful that would be. It could for example be an option to move the code to the js-sdk and expecting that future widgets will keep using that one. If the MSC is formulated openly enough, it could also leave other client authors the option to not do any additional processing here.

Copy link

Choose a reason for hiding this comment

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

I agree with the thought, that the widget api should not be unnecassarly complex to implement for other clients. I think the better solution for a better user list would be to use the js-sdk for the widget and run the user list modifications to be done inside the widget.


## Proposal

We will add a new interface to the widget API to search users in the user directory. We will introduce
Copy link

Choose a reason for hiding this comment

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

I am wondering if using "we" is common in MSC's. I have not seen it before. I am not aware if there are any rules. But I think rephrasing without "we" would not hurt?

Copy link

Choose a reason for hiding this comment

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

(this is very nitpicky indeed...)

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I'm used to write ADRs for our own products which are written from our teams perspective. I will rewrite it later to better fit the MSC styling

dhenneke added 2 commits March 6, 2023 09:18
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
send an error response (as required currently by the capabilities system for widgets).

The client SHOULD NOT modify the data of the request. The widget is responsible for producing valid
events - the client MUST pass through any errors, such as permission errors, to the widget using the
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it was copied from MSC2762 but doesn't quite fit here. Maybe the sentence "The widget is responsible..." should be removed completely?

Copy link
Author

Choose a reason for hiding this comment

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

This is is a copy from MSC2762 because it asks for the same behavior. MSC2762 is mainly defining the actions read_event and send_event but I would de-facto see it as a more general that kind-of defines the general behavior of the contract between the homeserver, the client, and the widget. But I'm fine with skipping it in this proposal.


While this MSC is not present in the spec, clients and widgets should:

- Use `org.matrix.msc3973.` in place of `m.` in all new identifiers of this MSC.
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? Doesn't the following bullet point cover everything? (Or is this a standard sentence we include in this section?)

Copy link
Author

Choose a reason for hiding this comment

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

Not necessarily. So far, actions in the Widget API (like read_event, send_event, ...) are not prefixed with m.*, so the second point would not apply here. The first point mainly references the new permission m.user_directory_search while the second references the action user_directory_search.

Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal widgets anything to do with widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants