Improve whitespace in diagnostics popup #168
Conversation
fixes #159 Directly replacing whitespace with &nbps; works only, if the result is passed to mdpopups as html directly. The solution is to use `\u00A0` as fixed whitespace instead, which mdpopups is designed to use for markdown as well.
|
Ehm, I think this also needs to be applied to the signature popup. I could extract and share the styling between them. |
Agreed, but we're definitely going to have merge conflicts ;) I suggest to wait and see how the pull requests get merged in their current state and then go from there. |
|
@rwols hurry up a bit with those PR’s then 😉 |
|
It looks a lot better! I am not convinced this is a maintainable approach to getting good-looking hovers, hopefully it's okay if we take a more holistic look at mdpopups / minihtml limitations after the signature help changes are merged. |
Yeah, you kinda end up fighting against markdown, mdpopups and minihtml all at the same time. The html and css we need is actually very limited. I could mock one up that relies less on markdown and mdpopups' built-in templates? |
|
This is as good as I can get it without any hacky attempts to deal with the blank line after the fenced code block. It's a cleaner look overall and without the nested margins and borders it takes hardly any css to look good.
I read some more about what mdpopus provides and I definitely like how it handles things like indentation in Basically the only thing I don't like about this setup right now is how a line break between block elements is rendered as a blank line, and that really sounds like a minihtml limitation that perhaps mdpopups can provide a solution for. It's not the worst thing in the world either, so perhaps it's better not to muck about with it. |
|
Your changes totally break python hover tooltips! Result with your change (broken):
Before (ok):
You MUST decide whether to create your own html and disable markdown processing or let mdpopups do its job. You MUST NOT mix up markdown and html. |
| contents = contents.replace('\t', ' ' * 4) | ||
| contents = contents.replace(' ', ' ' * 2) | ||
| contents = contents.replace('\n\n', '\n \n') | ||
| contents = contents.replace('\r\n', '\n') |
There was a problem hiding this comment.
Which language-server returns \r\n?
There was a problem hiding this comment.
Should be all of them, according to protocol?
There was a problem hiding this comment.
jsts in my case at least
main.py
Outdated
| else: | ||
| formatted.append(value) | ||
| formatted.append("<p class='description'>{}</p>".format(value)) |
There was a problem hiding this comment.
This line mixes html with markdown and breaks mdpopups' markdown processing.
There was a problem hiding this comment.
But markdown can have embedded HTML, right?
There was a problem hiding this comment.
That’s what I thought. Since when exactly can’t you have html in your markdown? Is that an mdpopups thing?
Also, you don’t have to capitalize “must”, I can read just fine and it just makes you come across as angry and frustrated. If you are, please let me know what I did to have that effect.
I’ll look into what happens with the python tool tips.
There was a problem hiding this comment.
Not at all, frustrated. And yes it seems an mdpopups thing to not support embedded html this way as well as forcing <br> rather than using paragraphs.
A possible solution was to disable markdown processing md=False, create all lines as you did here and use mdpopups.syntax_highlight() to get highlighted html for source code embedding it into a <div class="highlight"> manually. A little more effort was needed to do so, but it's solvable.
But I am concerned about all of this might grow quickly beyond the desired scope as it might create more and more per language edge cases needed to struggle with?
The only thing I am not too fine with is enforcing font-faces on a per package base.
There was a problem hiding this comment.
Not at all, frustrated.
Pfieuw 😄
A little more effort was needed to do so, but it's solvable.
Yeah, but if the content is in markdown, wouldn't that mean we would also have to convert all that? You use mdpopups in GitGutter right? Is that a custom HTML?
But I am concerned about all of this might grow quickly beyond the desired scope
It kinda feels like we can't get away with doing nothing (i.e. just push everything through mdpopups) and we definitely don't want do do a lot. I think we can go two ways here:
- get mdpopups / @facelessuser involved to tweak mdpopups based on our findings
- find a middle ground where we process content as little as possible and do as little styling as possible
The only thing I am not too fine with is enforcing font-faces on a per package base.
Well, it's not a font-face but just a generic family, but I understand your point. I personally see no harm in using sans-serif here, but it's not my call.
There was a problem hiding this comment.
I should also mention that setting that setting md=False when calling show popup will just pass through HTML without applying markdown.
Anyways, if you are hoping to have docstrings parsed as markdown-ish, you may have to process yourselves as markdown-ish may not work. It's possible you could write your own extensions for Python Markdown and use those though....
Anyways, it sounds like you guys have a lot to figure out. If you have more specific questions, I'd be happy to answer them.
There was a problem hiding this comment.
Maybe we should handle this server-side?
There was a problem hiding this comment.
@rwols in what sense exactly? Getting the servers to standardize their output?
There was a problem hiding this comment.
Learned so much in the above. Adding the markdown='1' attribute allows us to wrap the content in something without losing the markdown processing. Also, minihtml isn't as weird as I feared it might be, but I am getting a <br> where I don't want one.
I'll fiddle with it some more.
<p><div class="highlight"><pre>function scaleOrdinal<string>(range?: string[]): ScaleOrdinal<string, string> (+1 overload)<br></pre></div><br /><div class='description' markdown='1'><strong>function</strong> <em>(exported, ambient)</em></div><div class='description' markdown='1'>Constructs a new ordinal scale with an empty domain and the specified range.<br />If a range is not specified, it defaults to the empty array; an ordinal scale always returns undefined until a non-empty range is defined.<br /><br />By default, the domain is configured to generate implicitly, if the scale is invoked with an unknown value.<br />See the "unknown(...)" method of the scale to change this behavior.<br /><br />The generic corresponds to the data type of range elements.</div></p>
There was a problem hiding this comment.
in what sense exactly? Getting the servers to standardize their output?
Yes. Different languages have different traditions for documentation. e.g. Python has its own ascii style, C++ has Doxygen, Java has JavaDoc, etc.
|
This is waiting for other PR’s, so please just consider it an investigation into what gives the best level of control over layout and readability vs. effort and maintainability. Currently it seems like servers don’t return clean markdown all the time, mdpopus doesn’t convert lines into paragraphs as I'd expect from markdown, somehow mixing html into the markdown breaks things, and minihtml doesn't deal with whitespace the way a browser would. It's a chain of elements that are all slightly out of spec and simply don't connect well. Perhaps the best we can get is what we already have. The engineer in me doesn't like that idea ;) edit: whoops, wrong button, didn't intend to close this. |
|
So, what I decided to do now is pass all contents through I removed the The I'm new to mypy, any help with the errors? |
|
Indeed, looks well for most python docstrings. The extra empty line is actually appreciated with python as it separates "paragraphs" very clearly. There are some issues with signatures including line breaks in the argument list, but it appears for little functions only. |
main.py
Outdated
| sublime.active_window().active_view(), | ||
| "<div class='description' markdown='1'>{}</div>".format(preserve_whitespace(value)) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I am not sure about @tomv564 's coding style wishes, but I'd suggest
if language:
formatted.append(mdpopups.md2html(
self.view, "```{}\n{}\n```\n".format(language, value)))
else:
formatted.append(mdpopups.md2html(
self.view, "<div class='description' markdown='1'>{}</div>".format(preserve_whitespace(value))))... to save some lines of code.
Btw.: you can use self.view inside ViewEventListeners instead of sublime.active_window().active_view()
main.py
Outdated
| contents = contents.replace('\r\n', '\n') | ||
| contents = contents.replace('\t', '\u00A0' * 4) | ||
| contents = contents.replace(' ', '\u00A0' * 2) | ||
| contents = contents.replace('\n\n', '\n\u00A0\n') |
There was a problem hiding this comment.
I am no longer sure about that line. I originally added it as I experienced absolutely no spaces between paragraphs. Indeed it breaks the creation of <p>paragraph1</p><p>paragrah2</p> The line causes markdown to create <p>paragraph1<br><br>paragrah2</p>.
If we remove this line and tweak the CSS a little bit to add proper padding, we may not even need the md2html() functions as merging the lines into divs is effectively nothing else then creating several paragraphs. The solution would be far easier then anyone of us thought yesterday 😄
You may try ...
...
for item in contents:
if isinstance(item, str):
value = item
language = None
else:
value = item.get("value")
language = item.get("language")
if language:
formatted.append("```{}\n{}\n```\n".format(language, value))
else:
formatted.append(preserve_whitespace(value))
mdpopups.show_popup(
self.view,
"".join(formatted),
css='''
.mdpopups .lsp_hover {
margin: 0 0 0 0.5rem;
}
.mdpopups .lsp_hover .highlight {
border: none;
margin-bottom: 0;
}
.mdpopups .lsp_hover p {
padding: 0.5rem 0.5rem 0 0;
}
''',
md=True,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
wrapper_class="lsp_hover",
max_width=800)
def preserve_whitespace(contents: str) -> str:
"""Preserve empty lines and whitespace for markdown conversion."""
contents = contents.strip(' \t\r\n')
contents = contents.replace('\r\n', '\n')
contents = contents.replace('\t', '\u00A0' * 4)
contents = contents.replace(' ', '\u00A0' * 2)
return contents... may need to have a look onto other stylesheets, too.
There was a problem hiding this comment.
That's also pretty clean. However, it looks a lot better if the description text aligns with the code text. Now it aligns with the code box:
Some screenshots to compare:
Also note in the first screenshot how there is whitespace missing after function _(exported, ambient)_. But disregarding that, I'm leaning towards the second or third style.
As an aside, I don't think we have to prefix css selectors with .mdpopups (e.g. .mdpopups .lsp_hover). Our css follows after mdpopup's so there is no need to increase the specificity.
There was a problem hiding this comment.
As for sticking closely to mdpopups, this is in line with @deathaxe's suggestion, but with some fixes and even less lines of code:
for item in contents:
value = ""
language = None
if isinstance(item, str):
value = item
else:
value = item.get("value")
language = item.get("language")
if language:
formatted.append("```{}\n{}\n```\n".format(language, value))
else:
formatted.append(preserve_whitespace(value))
mdpopups.show_popup(
self.view,
"\n".join(formatted),
css='''
.lsp_hover {
margin: 0.5rem 0.5rem 0 0.5rem;
}
.lsp_hover p {
margin-bottom: 0.5rem;
}
''',
md=True,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
wrapper_class="lsp_hover",
max_width=800)
def preserve_whitespace(contents: str) -> str:
"""Preserve empty lines and whitespace for markdown conversion."""
contents = contents.strip(' \t\r\n')
contents = contents.replace('\r\n', '\n')
contents = contents.replace('\t', '\u00A0' * 4)
contents = contents.replace(' ', '\u00A0' * 2)
return contents
I still think it's prettier to align the text, but I also very much like how little code this is.
To align text, it's now enough to add padding: 0 0.5rem; to .lsp_hover p.
I also kinda like removing the border around the code, by adding .lsp_hover .highlight { border-width: 0; border-radius: 0; }
There was a problem hiding this comment.
LGTM, you probably want to experiment around with the new signature help browser now, too.
There was a problem hiding this comment.
I don't think we have to prefix css selectors with .mdpopups
agree.
However, it looks a lot better if the description text aligns with the code text.
Can be tweaked by setting the margin of <p>, i think. The highlight is a <div> and is therefore not effected. I just didn't see as pyls does return the signature as ordinary text unfortunately.
there is whitespace missing after
I saw such issues with signatures, too. Would be interesting how the raw text looks like, and if there is a way to fix it before passing it to mdpopups.
There was a problem hiding this comment.
Changed my mind about the border around the code box. I'll push an update and will look into some of the other popups later on. Gotta watch a football match right now, will get back to the code as soon as we're a couple of goals behind ;)
There was a problem hiding this comment.
I see, it might be caused by applying preserve_whitespace() to each line separately. It strips tailing newlines, and therefore should be called with the joined lines.
TOOLTIP_CSS = """
.lsp_hover {
margin: 0;
padding: 0;
}
.lsp_hover .highlight {
border-radius: 0;
border-style: none;
font-size: 1.2rem;
}
.lsp_hover p {
padding: 0.5rem 0.5rem 0 0.5rem;
}
"""
...
for item in contents:
if isinstance(item, str):
value = item
language = None
else:
value = item.get("value")
language = item.get("language")
if language:
formatted.append("```{}\n{}\n```\n".format(language, value))
else:
formatted.append(value)
mdpopups.show_popup(
self.view,
preserve_whitespace("\n".join(formatted)),
css=TOOLTIP_CSS,
md=True,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
wrapper_class="lsp_hover",
max_width=800)
|
Alrighty, that's the final push from my end, ready for feedback.
Nice clean, uniform popups all around. And in the end it wasn't even that hard. Why did it look so hard a week ago 🤷♂️ ? I did notice this with pyls:
Gif compression makes it hard to see, but the hover popup doesn't pick up that first line as a code block, but the signature popup does. I don't think that's new or related, but I thought it was a bit weird. |
as per jps's recommendation
|
It might be because recently I switched to using the Sublime highlighter by default, but that was I think back around June. Either way, bonus! |
|
Well, I removed my fork and let ST download the dependency again and highlighting is gone again... it was fun while it lasted 😞 Edit: perhaps this is just me and we should do the debugging elsewhere? |
|
I've never seen highlighting in fenced code blocks personally, too. Maybe investigate this further @facelessuser ? |
|
Really? What version of mdpopups is on your system: import mdpopups
mdpopups.version()Maybe you guys picked up my secret version 😏 ...yeah, there's no secret version...or is there? |
|
|
|
Hmm, that's the latest...can you show me the fence source (raw markdown you use)? Or do you render it directly via the some other method? |
|
Can you guys install this and see if you get highlighting in the fences as well? https://github.com/facelessuser/mdpopup_test. Just run the |
|
The attached file is what comes out of |
|
Yup it should get highlighted. Let me know how mdpopups_test runs. It's possible I accidentally broke and then fixed something without even knowing 😕 . |
|
Version 2.1.1 here as well, and running |
|
Ruh roh. I done broke something. I guess I need to cut a release. |
|
eh, nevermind, I get the same thing as Raoul |
|
Weird... |
|
I'll probably merge the popup PR this evening and cut a release. I'll make sure my version of mdpopups_test is pristine and up to date with what's on tip and ensure everything looks good. You guys can then update the dependency and mdpopups_test, and if after that something is still broken, I'll dig deeper. |
|
Just wanted to note that both @braver and me are on Mac, maybe the problem is there? Just guessing. |
|
Nah, I'm a mac user too...just not at work. I probably broke something and there aren't any plugins relying on the highlighting, so no one reported it to me. |
|
Do either of you have this in your preferences file set to False? |
|
Nope, didn't know that was a thing |
|
Just making sure. If it's false it will use pygments. But even pygments should highlight it. All of my machines seem to be running mdpopups tip, so I at least know that that mdopups_tests is working, so it looks like I just need to release mdpopups latest. Hopefully that will fix it. |
|
It might be nothing, but I noticed with master that I also get a mdpopups output panel that I can select
It's not there when installed via "satisfy dependencies". BTW. what I did now was download a zip of master, put it in Packages instead of the original and copy the |
|
Probably because it hasn't used the sublime highlighter to highlight code. It actually puts the code snippet in a hidden output (I should disable it showing up in the menu though) and renders it with Sublime. It then uses logic I ripped from my ExportHtml plugin to extract the real colors from the view and construct the HTML. If it doesn't go down that path (which clearly it isn't for you before tip), it won't create the output panel. |
I point to my own channel which pulls the latest commit instead of tag. |
|
Alright. Mdpopups 2.2.0 released. Let me know if that fixes things. |
|
Yes, it works now! For Python at least. Also in GitGutter by the way, never had highlighting there before. What determines if it works for a language? I get no highlighting in TypeScript for something like this: ```typescript |
|
It's kind of late here, so I'll answer and then go to bed 🙂 . The Sublime Text highlighter is the default highlighter. And I've created a mapping (https://github.com/facelessuser/sublime-markdown-popups/blob/master/st3/mdpopups/st_mapping.py) for most of the default syntaxes, but maybe not all, and definitely not for a lot of the 3rd party sytnaxes out there. Feel free to add new ones if needed. In general I try to match names to what Pygments uses so if a user switches between them, it will still work. One day, I may drop Pygments for mdpopups, but I will most likely always use something like Pygments as a reference for a known sytnax convention for fences. |
|
Nice! Colors everywhere!
Edit: @tomv564 sorry about the noise. Major collab cross-pollination going on here ✨ |
|
Well, my work here is done. Don't forget to update dependencies: {
"*": {
">=3124": [
"pygments",
"python-markdown",
"mdpopups",
"python-jinja2",
"markupsafe",
"pymdownx",
"pyyaml"
]
}
}I don't have an exact date, but sometimes soonish I plan make |
|
Replaced by #176 |
Fixes #105
Currently, descriptive text just sits in the parent element and cannot be targeted with CSS without also affecting code snippets. This PR solves that by wrapping it in a
div.text.However, if there is a newline between
divs it is rendered as a blank line. This shouldn’t happen in regular HTML or Markdown, but this isn’t regular. This causes two problems:\nbefore adiv. Fenced Code blocks should be followed by a newline so that’s ensured when the block is added.I had a little side-issue with any text after a
\r\nnot being rendered. I dunno why, also don’t know why there would be a\r\n, must be something to do with TypeScript being a Microsoft thing and giving us Windows line endings or something. It also caused double newlines to not be respected. I solved that by converting\r\nto\n.Content in a
div.textis now also rendered with a sans-serif font, and thereby fixes #82Before
After
(replaces #167)