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

[Bug] New message parser #22280

Closed
ankar84 opened this issue Jun 8, 2021 · 26 comments
Closed

[Bug] New message parser #22280

ankar84 opened this issue Jun 8, 2021 · 26 comments

Comments

@ankar84
Copy link

ankar84 commented Jun 8, 2021

Description:

In that issue I will try to accumulate some issues with new message parser
image

Steps to reproduce:

  1. Create some complex text
  2. Check how it parsed

Expected behavior:

All should parsed properly

Actual behavior:

_italic message followed by some letter_A not become italic

This
image
not parsed
image
But this
image
do
image

So, for now we found 2 bugs:

  • _italic message followed by some letter_A not become italic
  • code block not transform to code block in there is new line before bottom apostrophes

Server Setup Information:

  • Version of Rocket.Chat Server: 3.15.0
  • Operating System: CentOS7
  • Deployment Method: docker
  • Number of Running Instances: 25
  • DB Replicaset Oplog: Enabled
  • NodeJS Version: 12.22.1
  • MongoDB Version: 4.2.14 WiredTiger

Client Setup Information

  • Desktop App or Browser Version: 3.2.2
  • Operating System: Windows 10

Additional context

https://open.rocket.chat/channel/support?msg=StXKYkq3DbL4GQ2RE

Relevant logs:

No

@ankar84
Copy link
Author

ankar84 commented Jun 8, 2021

Another interesting issue with new parser - ascii emoji become graphic emoji and again transform in ascii
new_parser

@johncrisp
Copy link

Thanks for reporting this Anton.

Good we have someone testing it!!

@ankar84
Copy link
Author

ankar84 commented Jun 9, 2021

Another issue.
We have Lotus Notes client and all that Lotus Domino infrastructure, so I added notes,Notes to Markdown Support Schemes for Link in Admin UI - Message
image
And in legacy parser all looks fine (link works fine)
image
And with new parser it's only text
image

@ankar84
Copy link
Author

ankar84 commented Jun 9, 2021

Another issue - when you edit message, with new parser you need to reload app (CTRL+R) to get edited version of message
new_parser1

@ankar84
Copy link
Author

ankar84 commented Jul 20, 2021

Mention not open profile on click (click do nothing)
mention01

@ggazzo
Copy link
Member

ggazzo commented Sep 17, 2021

the first "bug" its not a bug, it was defined in the gramar
_italic message followed by some letter_A

I'm currently working to fix the others

@zdumitru
Copy link
Contributor

the first "bug" its not a bug, it was defined in the gramar
_italic message followed by some letter_A

I'm currently working to fix the others

What if I want to have Italic text and then colon after it that is not italic:?

It used to work correctly, we had integrations built based on how it worked. And now you say it's not a bug.

@ggazzo
Copy link
Member

ggazzo commented Sep 21, 2021

I will try to explain but its quite tricky , I know you want the previous behavior "working"

_but check t_his
and this
and check this

the raw text:

_but check t_his <--- your case
_and this_
*and check t*his

Looks the last case is what you are looking for, but Github fails on your specific case as well. Since we use _ to delimit our italic and * to define bolds we dont have a smart way on your grammar to allow what you are asking for (without changing the whole meaning of each code).

Somehow "_" are considered "text" (its not only a markup character), Imagine a case, some slugfied conversion or whatever, and then we have this sentence "imagine_that im writing_something" based on the previous behavior we would render "imaginethat im writingsomething" that is odd and there no such way to prevent that.

I will try to consider you case, but to me the best choice would be interpret * _ the same ways github does, but imagine how many people would complain about this change

@ankar84
Copy link
Author

ankar84 commented Jan 11, 2022

I'm currently working to fix the others

Hey, @ggazzo
Thank you for your great job!

Now most important issue for us is this
We have this settings in Admin UI - Messages - Markdown
image

But with new parser notes and e1c protocols not parse as a links
image
image

@dudanogueira can you prioritize that to FE team, please?

Actually because of RocketChat/Rocket.Chat.ReactNative#3592 we forced to use new parser.
And I guess in near future versions new parser will be the only one parser, but all my users use that functionality a lot.

@ankar84
Copy link
Author

ankar84 commented Jan 19, 2022

@ggazzo What do you think maybe new parser ignores Markdown Support Schemes List parameter?

@ankar84
Copy link
Author

ankar84 commented Jan 19, 2022

Mention not open profile on click (click do nothing) mention01

That issue also still persist in most recent 4.3.1 version.
Old parser opens modal windows on click.

@ankar84
Copy link
Author

ankar84 commented Jan 19, 2022

Another issue - when you edit message, with new parser you need to reload app (CTRL+R) to get edited version of message new_parser1

And that issue not fixed in 4.3.1

@ankar84
Copy link
Author

ankar84 commented Jan 19, 2022

Another interesting issue with new parser - ascii emoji become graphic emoji and again transform in ascii new_parser

And that one

@ankar84
Copy link
Author

ankar84 commented Jan 19, 2022

Actually looks like new parser ignores lots of Admin UI - Message parameters
Another example
image
image

@franck-eyraud
Copy link

franck-eyraud commented Feb 21, 2022

Hello, I encountered an issue with this option, maybe you already know it.

I enabled it out of curiosity without knowing what it was supposed to do, and the result was that OTR sessions were not working for me. All the messages (even mine) were shown as encrypted. I spent some time to find out that it was due to this option enabled days ago and which I didn't notice what it was doing.

Is there a link where to understand what the new message parser is supposed to offer ?

If this feature is not compatible with OTR, I suggest to clearly state it in the warning box, or if possible to disable it temporarily when enabling OTR.

Thank you

@ankar84
Copy link
Author

ankar84 commented Apr 25, 2022

Guess 4.7 will totally remove old (pretty) message parser and all that mentioned here issues are still not resolved in any way.
Here is a screen from early RC of 4.7
image
And here is a 4.6.3 screen
image
@ggazzo what to do with all new message parser issues?
I critically need this feature in new parser #22280 (comment)

@ggazzo
Copy link
Member

ggazzo commented Apr 25, 2022

Well, we plan to keep the support for the old message for a while until the next major, meanwhile improve and solve some open issues related with parser, I cannot guarantee all the old behaviours will remain since some make no sense. B

@ankar84
Copy link
Author

ankar84 commented Apr 26, 2022

Hey, Guilherme!
Thanks for a quick response!

we plan to keep the support for the old message for a while until the next major

That is good.
How can I enable it to my deployment since 4.7? As you can see since 4.7 there is no user option to enable old parser. There is an option for legacy messages instead.

meanwhile improve and solve some open issues related with parser

That's totally what need to be done, thank you!
As a recall - most important for us is this issue #22280 (comment)

I cannot guarantee all the old behaviours will remain since some make no sense

I understand that and agree with you!

@ankar84
Copy link
Author

ankar84 commented Sep 6, 2022

Legacy template broken in 5.1.0 #26801

@hugocostadev
Copy link
Contributor

The most important issues mentioned here were already fixed in the past releases, because of this I'm closing this issue.

If you find any other issues, please, create a separate issue to make it easier to track. Thank you so much!

@ankar84
Copy link
Author

ankar84 commented Oct 13, 2022

@hugocostadev how about that issue?
#22280 (comment)

@hugocostadev
Copy link
Contributor

@hugocostadev how about that issue?
#22280 (comment)

That issue it's fixed too, the new message parser is protocol agnostic :)

@ankar84
Copy link
Author

ankar84 commented Oct 14, 2022

@hugocostadev how about that issue?
#22280 (comment)

That issue it's fixed too, the new message parser is protocol agnostic :)

Guess you didn't get the main point of that issue. Please read that actual whole issue to get it.

@hugocostadev
Copy link
Contributor

@hugocostadev how about that issue?
#22280 (comment)

That issue it's fixed too, the new message parser is protocol agnostic :)

Guess you didn't get the main point of that issue. Please read that actual whole issue to get it.

Sorry, I didn't realized that the path didn't have the URI structure.

Could you open a separated issue for that please? Appreciate it.

I'll bring this issue internally and see if we can support it.

@ankar84
Copy link
Author

ankar84 commented Oct 14, 2022

@hugocostadev how about that issue?
#22280 (comment)

That issue it's fixed too, the new message parser is protocol agnostic :)

Guess you didn't get the main point of that issue. Please read that actual whole issue to get it.

Sorry, I didn't realized that the path didn't have the URI structure.

Could you open a separated issue for that please? Appreciate it.

I'll bring this issue internally and see if we can support it.

Sure! I will do it today, and leave here a link.
Thank you!!

@ankar84
Copy link
Author

ankar84 commented Oct 14, 2022

Here it is @hugocostadev
#27065
I eill add some details in a few hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants