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

Cleanup after #2002 #2043

Merged
merged 7 commits into from
Sep 3, 2020
Merged

Cleanup after #2002 #2043

merged 7 commits into from
Sep 3, 2020

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Sep 1, 2020

Following what have been done on #2002, tests had to be updated.

@bmarty
Copy link
Member Author

bmarty commented Sep 1, 2020

@MexHigh I'm not sure that removing \n from HTML code in #2002 was source of bugs. Could you confirm please?

@tulir
Copy link
Contributor

tulir commented Sep 1, 2020

Any bugs that appear to be caused by removing \n from outgoing HTML are actually caused by whatever breaks when receiving HTML without \n. Other clients might remove \n even if you don't, so the bug needs to be fixed in the handling, not the sending.

Also as I noted in #2002 (comment), at least two of the four mentioned issues were not fixed.

@bmarty
Copy link
Member Author

bmarty commented Sep 1, 2020

The more I think about it, the more I have the feeling that having a body and a formatted_body is really a bad solution. The text itself is duplicated (and can even be totally different...), and it's not possible to handle all the possible cases without bug: save draft, edit message, etc.

@bmarty
Copy link
Member Author

bmarty commented Sep 2, 2020

Anyway @tulir I think you are right, I will revert some change and reopen closed issues by #2002, until we can confirm that they are fixed.

@bmarty bmarty force-pushed the feature/cleanup_after_2002 branch from e04a3c0 to d0532bb Compare September 2, 2020 07:17
@MexHigh
Copy link
Contributor

MexHigh commented Sep 2, 2020

One of the main problems with removing all \n from a message was that if it contains not only code, but normal text as well, the linebreaks in the plaintext would be removed too. It actually causes two inline code blocks with a linebreak between them to appear side by side. Another solution would be to replace \n with <br> tags but they are not used in "normal" messages either.
Linebreaks don't get removed on Element Web too, so I've oriented myself towards that.
As @tulir mentioned, the two issues (#350 and #1375) were not fixed, because I misunderstood what the actual problems were. My bad.

@bmarty
Copy link
Member Author

bmarty commented Sep 2, 2020

@MexHigh thanks.
I will revert the change on line break then (in this PR). And add unit test to cover the case you are talking about.

@bmarty bmarty merged commit d2d372d into develop Sep 3, 2020
@bmarty bmarty deleted the feature/cleanup_after_2002 branch September 3, 2020 09:15
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.

3 participants