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

Add basic m.thread support #1349

Merged
merged 13 commits into from
Aug 15, 2024
Merged

Conversation

greentore
Copy link
Contributor

@greentore greentore commented Jul 23, 2023

Description

Adds basic m.thread support as described in MSC3440.
Support in this case means that replying to a threaded message will place the reply in the thread in clients that support them.

Partially fixes #257

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@github-actions
Copy link

github-actions bot commented Jul 23, 2023

Preview: https://1349--pr-cinny.netlify.app
⚠️ Exercise caution. Use test accounts. ⚠️

@ajbura ajbura self-assigned this Jul 25, 2023
Comment on lines 289 to 292
if (replyDraft.relatesTo?.rel_type === "m.thread") {
content['m.relates_to'].event_id = replyDraft.relatesTo.event_id;
content['m.relates_to'].rel_type = "m.thread";
}
Copy link
Member

Choose a reason for hiding this comment

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

As we do not support thread in cinny we should also add "is_falling_back": true as we are replying to a message which is part of thread compare to sending a message in thread with client that support them.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the other way around. If we sent "is_falling_back": true, the message would be interpreted as posted to the thread directly with m.in_reply_to being set only for fallback purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This time I explicitly set is_falling_back to false. I think it's saner considering we're making a normal reply from Cinny's perspective.

src/app/state/roomInputDrafts.ts Outdated Show resolved Hide resolved
src/app/organisms/room/RoomInput.tsx Outdated Show resolved Hide resolved
@filipesmedeiros
Copy link

Is any help needed here? I can try to contribute if you think I could help :)

@JuliaSoriaSmith
Copy link

Can the reviewers with write access please look at this? This has been open for quite some time and threads are a noticeable missing feature

@greentore
Copy link
Contributor Author

I added threaded reply indicators inspired by element-x. Should resolve #1871.

@williamkray
Copy link

williamkray commented Aug 7, 2024

i'm not an approver but i've briefly tested the netlify deployment of this PR and it looks fantastic, i'm very eager to see this merged.

seems like the quoted message doesn't get truncated with elipses (...) properly, and runs off the right hand side of the screen.

image

@williamkray
Copy link

same messages in stable cinny:

image

@greentore
Copy link
Contributor Author

@williamkray looks like I broke it again. Thanks for the report.

@williamkray
Copy link

looks good after the fix!

@greentore greentore marked this pull request as draft August 7, 2024 22:07
Copy link
Member

@ajbura ajbura left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it looks great to me.

Could you please mark the PR as ready for review.

src/app/components/message/Reply.css.ts Outdated Show resolved Hide resolved
@greentore greentore marked this pull request as ready for review August 14, 2024 17:59
@greentore
Copy link
Contributor Author

I converted rem units to equivalent toRem calls as requested.
I also considered disabling thread support in sdk, but resulting unthreaded read receipts mark all threads in the room as read in threaded clients, which may be considered undesired behavior.

@ajbura ajbura merged commit 830d05e into cinnyapp:dev Aug 15, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Threads
5 participants