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

Always send formatted content #2141

Closed
wants to merge 4 commits into from

Conversation

langleyd
Copy link
Member

@langleyd langleyd commented Nov 22, 2023

Fixes #1999

I can understand the rational behind why the code that removes formatted_body for non-RTE mode might have been added but whether we send 'formatted_body' should not be tied to the RTE, we should always send it(e.g. the web client sends it even on the legacy client which is markdown based, as do legacy mobile markdown clients).

It is the preferred field for clients that support displaying rich content, whereas body field is a plain text representation of what is in formatted_body and can be used for clients that don't support displaying rich content.

formatted_body is required for mentions to work and render correctly in the timeline, hence the bug.

@langleyd langleyd requested a review from a team as a code owner November 22, 2023 10:07
Copy link

github-actions bot commented Nov 22, 2023

Warnings
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.

Generated by 🚫 Danger Swift against 03d6086

@langleyd langleyd marked this pull request as draft November 22, 2023 10:11
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7c3ee01) 72.00% compared to head (7e97e85) 71.91%.

❗ Current head 7e97e85 differs from pull request most recent head 03d6086. Consider uploading reports for the commit 03d6086 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2141      +/-   ##
===========================================
- Coverage    72.00%   71.91%   -0.09%     
===========================================
  Files          511      511              
  Lines        34722    34721       -1     
  Branches     16673    16673              
===========================================
- Hits         25001    24971      -30     
- Misses        9105     9137      +32     
+ Partials       616      613       -3     
Flag Coverage Δ
unittests 57.23% <100.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@langleyd langleyd marked this pull request as ready for review November 22, 2023 10:36
Copy link

sonarcloud bot commented Nov 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@pixlwave
Copy link
Member

pixlwave commented Nov 22, 2023

I can understand the rational behind why the code that removes formatted_body for non-RTE mode might have been added but whether we send 'formatted_body' should not be tied to the RTE, we should always send it(e.g. the web client sends it even on the legacy client which is markdown based, as do legacy mobile markdown clients).

I don't think its that simple. We always send the formatted_body when it isn't included, but Rust builds it from the Markdown in the body and attaches it within the SDK. So with this change, sending a message with Markdown will no longer work unless the RTE is updated to parse it first.

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

An example, this is from develop:

"content": {
    "body": "This is **bold**.",
    "format": "org.matrix.custom.html",
    "formatted_body": "<p>This is <strong>bold</strong>.</p>\n",
    "m.mentions": {},
    "msgtype": "m.text"
},

And this is with the changes:

"content": {
    "body": "This is **bold**.",
    "format": "org.matrix.custom.html",
    "formatted_body": "This is **bold**.",
    "m.mentions": {},
    "msgtype": "m.text"
}

Presumably we either need to fix the pills some other way or enable MD input support in the RTE.

@stefanceriu
Copy link
Member

Handled in #2246

@stefanceriu stefanceriu deleted the langleyd/rte_always_send_formatted_content branch March 13, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mention pills fall back to plain text in markdown mode
3 participants