Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Show updated relation reply from edited message #6809

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Show updated relation reply from edited message #6809

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25e454e
Show updated relation replies from edited message
MadLittleMods Sep 14, 2021
faa80bd
Remove debug logs
MadLittleMods Sep 14, 2021
235fdfa
Merge branch 'develop' into madlittlemods/10391-show-relations-in-new…
MadLittleMods Sep 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/components/views/elements/ReplyThread.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ export default class ReplyThread extends React.Component<IProps, IState> {
// could be used here for replies as well... However, the helper
// currently assumes the relation has a `rel_type`, which older replies
// do not, so this block is left as-is for now.
const mRelatesTo = ev.getWireContent()['m.relates_to'];
//
// We're using ev.getContent() over ev.getWireContent() to make sure
// we grab the latest edit with potentially new relations.
const mRelatesTo = ev.getContent()['m.relates_to'];
Copy link
Contributor Author

@MadLittleMods MadLittleMods Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This works because event.getContent() is smart enough to pull from the latest replacing event (message edit), this._replacingEvent.getContent()["m.new_content"] .

We probably want to also update matrix-js-sdk event.getRelation() to pull from getContent() as well, src/models/event.ts#L1077-L1087. I can create a follow-up PR after this is deemed acceptable 🙇

Copy link
Member

Choose a reason for hiding this comment

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

Updating the js-sdk makes sense, though I'm a bit concerned that the impact could be greater than we anticipate. Until it's reported to cause bugs, we should leave it alone I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know what you mean 🤔

I feel like we may be leaving other clients in the dust if we don't change it though. Someone making a client, bridge, etc just falls into the pitfall of not supporting this use case.

Copy link
Member

Choose a reason for hiding this comment

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

We do have a breaking change queued up for the js-sdk's next release, so squeezing it in there would be the safest bet I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I'll skip on adjusting it there because trying to make the change and I run into the rel_type and event_id constraint which isn't present for m.in_repy_to and not sure of the downstream effects.

if (mRelatesTo && mRelatesTo['m.in_reply_to']) {
const mInReplyTo = mRelatesTo['m.in_reply_to'];
if (mInReplyTo && mInReplyTo['event_id']) return mInReplyTo['event_id'];
Expand Down