Skip to content

Fix markdown removal in hover handling whitespace weirdly #13988

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

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Jan 20, 2023

Fixes #10028

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 20, 2023
@Veykril
Copy link
Member Author

Veykril commented Jan 20, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 20, 2023

📌 Commit c5b1e3f has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 20, 2023

⌛ Testing commit c5b1e3f with merge ce67dea...

@bors
Copy link
Contributor

bors commented Jan 20, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing ce67dea to master...

@bors bors merged commit ce67dea into rust-lang:master Jan 20, 2023
@sclu1034
Copy link
Contributor

As mentioned in #10028, line wraps are also messed up. kak-lsp has implemented Markdown by now, so I have nothing set up to test this with right now, but looking at the code, I don't see anything changed in this regard.
Specifically, soft wraps (Event::SoftBreak) probably shouldn't be turned into hard wraps (by inserting \n), as rust-analyzer cannot make assumptions about how the text will eventually be rendered.

E.g. the original Markdown might use soft wraps to limit the line length to 100 columns, but when the client only has 80 columns to render the result, you'd get breaks mid-screen:

# soft wraps at 100 columns
Lorem ipsum dolor sit amet, consectetur adipisicing elit. Eligendi non quis exercitationem culpa
nesciunt nihil aut nostrum explicabo reprehenderit optio amet ab temporibus asperiores quasi
cupiditate. Voluptatum ducimus voluptates voluptas?
# turned into hard breaks and rendered at 80 columns
Lorem ipsum dolor sit amet, consectetur adipisicing elit. Eligendi non quis
exercitationem culpa
nesciunt nihil aut nostrum explicabo reprehenderit optio amet ab temporibus
asperiores quasi
cupiditate. Voluptatum ducimus voluptates voluptas?

This would also bring it in line with how Markdown is rendered into HTML:

A regular line ending (not in a code span or HTML tag) that is not preceded by two or more spaces or a backslash is parsed as a softbreak.
- https://spec.commonmark.org/0.30/#soft-line-breaks

Browsers then turn a softbreak into a single space character:

# soft wraps at 100 columns
Lorem ipsum dolor sit amet, consectetur adipisicing elit. Eligendi non quis exercitationem culpa
nesciunt nihil aut nostrum explicabo reprehenderit optio amet ab temporibus asperiores quasi
cupiditate. Voluptatum ducimus voluptates voluptas?
# turned into spaces and rendered at 80 columns
Lorem ipsum dolor sit amet, consectetur adipisicing elit. Eligendi non quis
exercitationem culpa nesciunt nihil aut nostrum explicabo reprehenderit optio
amet ab temporibus asperiores quasi cupiditate. Voluptatum ducimus voluptates
voluptas?

@Veykril
Copy link
Member Author

Veykril commented Jan 20, 2023

Hm, so what should we do with softbreaks? Just ignore them?

@Veykril Veykril deleted the hover-no-markdown branch January 20, 2023 22:38
@sclu1034
Copy link
Contributor

I'd turn them into spaces, like browsers do in HTML.

bors added a commit that referenced this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plaintext textDocument/hover malformed
4 participants