Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC3869: Read event relations with the Widget API #3869
base: main
Are you sure you want to change the base?
MSC3869: Read event relations with the Widget API #3869
Changes from 1 commit
6d5a6a0
06c0639
accc575
4c63cf7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm failing to find the
direction
parameter in either MSC2675 or the specification.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I mixed up the MSC numbers. I meant MSC3715.
It is available as an experimental option in synapse:
https://github.com/matrix-org/synapse/blob/3dd175b628bab5638165f20de9eade36a4e88147/synapse/rest/client/relations.py#L59-L67
And also part of the
fetchRelations
function in thematrix-js-sdk
where I come across it. Though the name of the query parameter doesn't match with the synapse implementation 🤔:https://github.com/matrix-org/matrix-js-sdk/blob/8502759e3ee3a2b244a442b8dac2b67809d9270e/src/client.ts#L7298
We don't have a real use case for this feature, yet. So we could as well skip it for now. It also seems like the MSC in question isn't final yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, well I guess it's ultimately up to you whether to include the parameter, since MSC3715 is fairly close to FCP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then let's keep it in and hope that it becomes stable in synapse and get's fixed in the
matrix-js-sdk
so we can actually use it. It is also useful because then therelations(...)
endpoint in theRoomWidgetClient
will also be fully functioning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is element-hq/element-web#22501, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite like it. So if we are adding it to the widget api, it would probably best to then name it
dir
. The client can then decide whether to use the unstable or stable parameter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client is obligated to return what the widget requested and also to fetch all new events?
I think that should be made extra clear because that is something which usually is not the case. The client does not need to fulfill any guarantees in the other MSC's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that the client could also respond to this with a local cache of the relations? I implied that it should always get it from CS-API because it's also how it is (and was already) implemented in the
matrix-js-sdk
/Element. But it might be a good idea to clarify it here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something along the lines of:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should mention that this is the first widget api call that can force the client to trigger new cs-api calls. (all other msc's are phrased in a way, that the client can decide if it does not send the events.)
(I also think it would be nice to be more explicit what the clients responsibilities are. see other comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another alternative could be listed. Just to track the idea, that if the client also is capable of setting filters + use
/messages
custom event types could be used for a similar optimization. But I am not sure how expensive setting filters is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here. Filters won't work for e2e encryption because one would need to read and decrypt every message in the room to decide it which is not practical. But we could add a comment to clarify it.