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
MSC3440: Threading via
m.thread
relation #3440MSC3440: Threading via
m.thread
relation #3440Changes from all commits
beda89e
2fa27ac
d8a0a94
7d39887
6e37911
c142b17
c578f75
33acdf4
7102165
f84f949
f02dc8d
3e46728
44e967f
65d0d55
2b76a6e
99c5b2e
4ee42b1
fc81bbd
91e6ec7
eaeef00
26fb5f2
a23c795
6b1a368
f227592
b493f21
46e1e9b
e40efa0
23928e7
0880a86
1bbb021
3c977f7
0140454
700464c
0035202
e3cb699
847f468
c8ffa62
a7cbf8d
5896d69
ee5df80
00daf64
a5d8aab
d7ed3c4
5c04906
d667a0b
b157dfd
3162bea
fa232f4
68d9c42
5bbb015
707af2b
b28a365
8f82dfa
8f8be64
b6d8076
cd671ef
a61c01e
362e661
b831fb3
e2dde8e
e640f6b
bda3a1e
89c4b5e
61bb518
9159a5a
a97307a
f541dab
82b4c62
75f4cb2
54ce185
6d6baa2
893cf1f
641e326
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.
So one thing I noticed when looking at the push rules again: I expected this key to be in the "m.in_reply_to" relation, not on the thread relation. Is that intentional or not? Could this possibly be moved to the relation, that is actually falling back, so in this case the reply? Otherwise this seems to be a bit ambiguous.
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.
Moving it at this point would mean having to handle it in both cases indefinitely to not break the semantics of existing history.
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.
Well, the only consequence of that change would be that some old events would render with a reply. Considering the short timeframe that the use of m.thread has been legal and that this is not in any spec release yet and won't be for 3 months, I think fixing that now would be the much, much smaller pain point. Clients could accept both for a few months in the transition period and after that old threads might have some extra replies in them, but since they are still marked as beta, that will be justifyable. If we don't fix it, we will have to introduce a new field to mark other relations as fallbacks in the future, which will be then something we will actually have to have compatibility with for years.
So no, I don't agree we would need to handle that indefinitely. Implementing a feature before it is in a spec release has some risk and the problems with that will be minimal. I can write an MSC for that though, if it is something we need to argue about.
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 think @deepbluev7 has a point, but as I just wrote on #3664: please can you open a new issue for it.
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 wrote an MSC instead: #3825
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.
just to ack this here too: we (the SCT) are actively committing to v1.3 having threads, so are okay with this.