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.

Sign up for GitHub

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

Jump to bottom

Do not remove escaping from brackets #2704

Merged
merged 3 commits into from
Jul 19, 2022
Merged

Do not remove escaping from brackets #2704

merged 3 commits into from
Jul 19, 2022

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jul 10, 2022 •
edited
Loading

Summary

See #2703 for full description

Removed the erroneous replacement of escaped brackets with plain brackets as this breaks valid markdown.
Added a test case for this.

Added markdownit and tiptap extensions to tag special characters (characters which might have a special meaning for markdown) that are not parsed by markdownit, e.g. because they are extended markdown syntax. The tags are only stripped if the user edits it that part of the file.
This way we can ensure they do not get escaped by prosemirror-markdown.

@susnux susnux force-pushed the fix/link-text-escaping branch from e1e73ae to b01a1d7 Compare July 10, 2022 19:12
@susnux
Copy link
Contributor Author

susnux commented Jul 10, 2022

/compile

@susnux susnux force-pushed the fix/link-text-escaping branch 2 times, most recently from 477b1f2 to dcb2663 Compare July 11, 2022 11:46
@susnux susnux changed the title Do not remove escaping from brackets WIP: Do not remove escaping from brackets Jul 11, 2022
@susnux
Copy link
Contributor Author

susnux commented Jul 11, 2022 •
edited
Loading

Do not merge, still some work to do (WIP).... implementing some more markdown fixes.

Edit: done

@juliusknorr juliusknorr added bug Something isn't working 2. developing labels Jul 12, 2022
@susnux susnux force-pushed the fix/link-text-escaping branch from dcb2663 to ae0518b Compare July 13, 2022 13:30
@susnux
Copy link
Contributor Author

susnux commented Jul 13, 2022

/compile amend

@susnux susnux changed the title WIP: Do not remove escaping from brackets Do not remove escaping from brackets Jul 13, 2022
@susnux susnux force-pushed the fix/link-text-escaping branch from 31edbaf to 8b9f2ce Compare July 13, 2022 14:12
@susnux
Copy link
Contributor Author

susnux commented Jul 13, 2022

Added test cases, all tests pass I think this can be reviewed 😃

@vinicius73
Copy link
Member

This PR fix one or two problems @susnux ?

@susnux
Copy link
Contributor Author

susnux commented Jul 14, 2022 •
edited
Loading

This PR fix one or two problems @susnux ?

It fixes two problems + some parts of the "markdown formatting get broken"-problems

  1. I fixed Link text escaping of backslash is removed #2703 which is quite simple but solves the problem: https://github.com/nextcloud/text/pull/2704/files#diff-6591e0135928d4bf8e3856079a58f0453ddfd4a62a71d923c30cbd2913973cb2
  2. That fixes that problem but reintroduces Bracket escaping issue #325 so thats why I added the keep syntax extensions, those extension simply add marks for parts of the original markdown file that would be escaped when serializing the editor and if present when serializing they will prevent escaping of that region. Those marks stay until the user manually edits that part.
    So as an benefit this also fixes parts of Markdown files from external editors lose formatting on save #593 and Messed up entire markdown syntax #2577

Switch the dependency also fixes a third problem: Github Markdown Format allows any whitspace inside the brackets, but the currently used and orphaned markdown-it extension does not support this, so I switched to a maintained fork (which is by the way ported to TS).

@susnux
Copy link
Contributor Author

susnux commented Jul 14, 2022

@vinicius73 if preferred I can split this PR into two, one for fixing the issue and one for switching the dependency to fix the other issue.

@susnux susnux force-pushed the fix/link-text-escaping branch from 8b9f2ce to 30dc282 Compare July 14, 2022 10:30
@susnux
Copy link
Contributor Author

susnux commented Jul 14, 2022

/compile amend

@susnux susnux force-pushed the fix/link-text-escaping branch 2 times, most recently from 0193f1e to 99e532f Compare July 15, 2022 10:22
@susnux
Copy link
Contributor Author

susnux commented Jul 15, 2022

/compile

@susnux
Copy link
Contributor Author

susnux commented Jul 15, 2022

I decided to split the PR as the switch of the dependency was not really needed to fix this issue, but another.

So this in now only fixing the issues described in the comment above ( #2704 (comment) ), for the other part see: #2720

@vinicius73
Copy link
Member

Thanks for it @susnux

@susnux susnux force-pushed the fix/link-text-escaping branch from e278adf to 444e779 Compare July 15, 2022 13:39
@susnux
Copy link
Contributor Author

susnux commented Jul 15, 2022

/compile amend

susnux added 2 commits July 19, 2022 11:24
@susnux
Added markdown parsing rules to tag special syntax to be not modified…
e5f773b 
… when saved

Added a markdown-it and tiptap extension to tag special, unknown,
markdown syntax which would be escaped by prosemirror-markdown on save.
The tagged part is not touched while saving if they are not modified manually.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux
Do not remove escaping from brackets, fixes #2703
7e0728c 
Removed the erroneous replacement of escaped brackets with
plain brackets as this breaks valid markdown.
Added a test case for this.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux susnux force-pushed the fix/link-text-escaping branch from ee8ce63 to da10d8a Compare July 19, 2022 09:27
@susnux
Copy link
Contributor Author

susnux commented Jul 19, 2022

/compile amend

@nextcloud-command
Compile assets
8bdfcaf 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliusknorr juliusknorr merged commit 61c83d7 into master Jul 19, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/link-text-escaping branch July 19, 2022 10:15
@skjnldsv skjnldsv mentioned this pull request Aug 11, 2022
Merged
@juliusknorr juliusknorr mentioned this pull request Oct 28, 2022
Open
@susnux susnux mentioned this pull request Oct 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@vinicius73 vinicius73 vinicius73 approved these changes

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud

Assignees
No one assigned
Labels
4. to release bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Link text escaping of backslash is removed
4 participants
@susnux @vinicius73 @juliusknorr @nextcloud-command