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 the handling of tabs in quickinfo Markdown code blocks #95109

Closed

Conversation

Neonit
Copy link
Contributor

@Neonit Neonit commented Apr 13, 2020

This PR fixes #95096

This alters the behavior of Markdown parsed from JSDoc documentation comments and its appearance in the IntelliSense quickinfo boxes on mouseover. Previously, tabs were replaced by four spaces each. I don't know, why. Also, I set the tab size to 4 for code blocks in the quickinfo box. Testing instructions are in the issue thread.

@Neonit
Copy link
Contributor Author

Neonit commented Apr 14, 2020

Unfortunately I think I need to recommend against my own PR. I just found out that the file marked.js is an external library. I didn't see that before, because I didn't expect an external library to be in the folder it is in nor to be tracked by the repo. I wondered why it's not in TypeScript, but I thought maybe it's some legacy code.

For the used library there exists an issue addressing this problem. The tabs are replaced by spaces, because the code apparently cannot handle tab indentation in some cases. I did a quick test and it seems tab indentation screws up nested lists. I couldn't see any other issues, though.

So maybe there should be a switch in the settings for this with the two options:

  1. Screw up my tabs in my Markdown code
  2. Screw up my nested lists

For the user to choose from.

Preferably, markdown-it looks like a much better thought-through library. I mean, marked.js didn't bother fixing this issue for almost five years. Also, wouldn't it be smart to use the same parser for both the quick info boxes and the Markdown preview? Apparently, the library used for the Markdown preview does not replace tabs so it's likely not marked.js.

@mjbvz
Copy link
Collaborator

mjbvz commented Apr 14, 2020

Thanks for investigating.

I agree that marked has many issues, however it's also pretty small. This is important because we ship it as part of the Monaco editor. So I don't think we can easily switch to another engine

Is there some way to scope your fix just to code blocks?

@Neonit
Copy link
Contributor Author

Neonit commented Apr 15, 2020

Well, the replacement is just a dumb in all the code, replace a character command with no further context information.

I thought of maybe another fix. Apart from the one informational issue that tab characters get replaced by space characters I'd say the major problem are the visual issues:

  1. Tabs are being fixed to a size of 4.
  2. Mid-text tabs do not have the expected aligning effect.

As long as the resulting length is 4 I personally do not care about 1. to be honest. I use them to align mid-text, though.

apple	red
banana	yellow
lime	green

So I am thinking about a fix for 2. I could replace the few calls to String#replace() by a custom function that does the current replacements and replaces tabs by either 2, 3 or 4 spaces depending on their offset within the line to mimic their aligning nature. This should have no impact on indentation tabs, but fix mid-text tabs.

apple   red
banana  yellow
lime    green

Currently, this would be the result:

apple    red
banana    yellow
lime    green

Why is this important for me? I haven't posted anything about it, yet, and I don't know how open you are for introducing new settings to VS Code, but I was thinking about a new setting allowing users to decide whether they want their quick infos with white-space: pre-wrap and a monospace font (configurable globally or for the workspace and for each file type as well). I asked on SO in the past whether I could enable preserving whitespace and I think there is no way currently. And as there doesn't seem to be a big demand for this feature I'd add it myself.

@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@mjbvz
Copy link
Collaborator

mjbvz commented Aug 6, 2021

I'm going to close this PR as it is out of date and the corresponding issue has not seen any further interest since it was opened

@mjbvz mjbvz closed this Aug 6, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants