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

Fix a bunch of issues related to edition and reply #5969 #8716

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

ganfra
Copy link
Member

@ganfra ganfra commented Dec 20, 2023

This fixes #5969
It replaces this PR even if the fix is not the same.

The downside is, it doesn't fix the edition of markdown, but at least we can keep the permalink...

@ganfra ganfra requested a review from bmarty December 20, 2023 17:52
@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Dec 21, 2023
@giomfo
Copy link
Member

giomfo commented Dec 21, 2023

I tested the edition of a "reply to" message with permalink and user pills. This is working like a charm:
without the rich text editor:
image

with the rich text editor:
image

@jmartinesp
Copy link
Member

jmartinesp commented Dec 21, 2023

I tried using the RTE to edit a message in this order:

  1. Text formatting enabled: the message was edited just fine.
  2. Same message, text formatting disabled (but RTE enabled in labs):
Screenshot

I think the MD text should be displayed there instead of the HTML, and in any case the mx-reply should be gone.

@giomfo
Copy link
Member

giomfo commented Dec 21, 2023

@jmartinesp I understand your point, but I'm not sure we should spend more time on this edge case where the Text formatting is disabled between 2 editions of the same message. I will let you decide about this with the Android team.
I didn't reproduce this issue in Element-AndroidX, which is for me the most important.

Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

There is the issue mentioned above, but it was there before and will required a different fix than this, but other than that it LGTM, just a question about detecting the mx-reply tags.

if (closingTagIndex != -1) {
return repliedBody.substring(closingTagIndex + "</mx-reply>".length).trim()
return repliedBody.substring(closingTagIndex + MX_REPLY_END_TAG.length).trim()
Copy link
Member

Choose a reason for hiding this comment

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

Could we get a </ mx-reply> tag, with one or several whitespaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope not, I kept the same tags than before.

@ganfra
Copy link
Member Author

ganfra commented Dec 21, 2023

Ok, lets iterate later with the other issues.

@jmartinesp jmartinesp force-pushed the feature/fga/fix_event_edition_reply branch from e83768a to 2ada4c8 Compare January 2, 2024 07:19
Copy link

sonarcloud bot commented Jan 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

4.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@ganfra ganfra merged commit e596196 into develop Jan 2, 2024
13 of 14 checks passed
@ganfra ganfra deleted the feature/fga/fix_event_edition_reply branch January 2, 2024 13:02
@scottwallacesh
Copy link

In Element Android v1.6.10 I cannot reply to any message. The option simply doesn't exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
5 participants