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

Improve whitespace in diagnostics popup #168

Closed
wants to merge 13 commits into from
Closed

Improve whitespace in diagnostics popup #168

Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c0271fe
Fix   being displayed in toolip code blocks
deathaxe Oct 7, 2017
675467c
improve whitespace in diagnostics popup
braver Oct 8, 2017
0aa2ce3
Remove prints
braver Oct 8, 2017
54c5853
reduce cruft
braver Oct 9, 2017
fa75b0e
Merge branch 'pr/fix-159' of https://github.com/deathaxe/LSP into bra…
braver Oct 9, 2017
f6a79e4
perhaps a p is more semantic? 🤷🏼‍♂️
braver Oct 9, 2017
748db3e
do md2html before passing to show_popup
braver Oct 10, 2017
8368d9a
less code is much betterer
braver Oct 10, 2017
54a07f9
hm, that border caused a 1px offset
braver Oct 11, 2017
3e75de8
uniform popups everywhere
braver Oct 11, 2017
197f771
Merge upstream/master into braver-whitespace
braver Oct 11, 2017
e1ab152
specify which arrow keys
braver Oct 11, 2017
67daa34
use the "system" generic font for descriptions
braver Oct 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2022,14 +2022,22 @@ def show_hover(self, point, contents):
value = item.get("value")
language = item.get("language")
if language:
formatted.append("```{}\n{}\n```".format(language, value))
formatted.append("```{}\n{}\n```\n".format(language, value))
else:
formatted.append(value)
formatted.append("<p class='description'>{}</p>".format(value))
Copy link
Contributor

@deathaxe deathaxe Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line mixes html with markdown and breaks mdpopups' markdown processing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But markdown can have embedded HTML, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should handle this server-side?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwols in what sense exactly? Getting the servers to standardize their output?

Copy link
Contributor Author

@braver braver Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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&nbsp;scaleOrdinal&lt;string&gt;(range?:&nbsp;string[]):&nbsp;ScaleOrdinal&lt;string,&nbsp;string&gt;&nbsp;(+1&nbsp;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>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


mdpopups.show_popup(
self.view,
preserve_whitespace("\n".join(formatted)),
css=".mdpopups .lsp_hover { margin: 4px; } .mdpopups p { margin: 0.1rem; }",
preserve_whitespace("".join(formatted)),
css='''
.mdpopups .lsp_hover .highlight {
border-width: 0;
}
.mdpopups .lsp_hover .description {
margin: 0 0.5rem;
font-family: sans-serif;
}
''',
md=True,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
Expand All @@ -2040,9 +2048,10 @@ def show_hover(self, point, contents):
def preserve_whitespace(contents: str) -> str:
"""Preserve empty lines and whitespace for markdown conversion."""
contents = contents.strip(' \t\r\n')
contents = contents.replace('\t', '&nbsp;' * 4)
contents = contents.replace(' ', '&nbsp;' * 2)
contents = contents.replace('\n\n', '\n&nbsp;\n')
contents = contents.replace('\r\n', '\n')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which language-server returns \r\n?

Copy link
Member

@rwols rwols Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be all of them, according to protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsts in my case at least

contents = contents.replace('\t', '\u00A0' * 4)
contents = contents.replace(' ', '\u00A0' * 2)
contents = contents.replace('\n\n', '\n\u00A0\n')
Copy link
Contributor

@deathaxe deathaxe Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

screen shot 2017-10-10 at 20 00 37

Some screenshots to compare:

screen shot 2017-10-10 at 20 04 27

screen shot 2017-10-10 at 20 11 49

screen shot 2017-10-10 at 20 11 20

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

screen shot 2017-10-10 at 20 38 35

screen shot 2017-10-10 at 20 39 00

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.

screen shot 2017-10-10 at 20 41 18

I also kinda like removing the border around the code, by adding .lsp_hover .highlight { border-width: 0; border-radius: 0; }

screen shot 2017-10-10 at 20 43 50

Copy link
Member

@rwols rwols Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, you probably want to experiment around with the new signature help browser now, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Contributor

@deathaxe deathaxe Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return contents


Expand Down