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 7 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
39 changes: 31 additions & 8 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2021,16 +2021,38 @@ def show_hover(self, point, contents):
else:
value = item.get("value")
language = item.get("language")

if language:
formatted.append("```{}\n{}\n```".format(language, value))
formatted.append(
mdpopups.md2html(
sublime.active_window().active_view(),
"```{}\n{}\n```\n".format(language, value)
)
)
else:
formatted.append(value)
formatted.append(
mdpopups.md2html(
sublime.active_window().active_view(),
"<div class='description' markdown='1'>{}</div>".format(preserve_whitespace(value))
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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


mdpopups.show_popup(
self.view,
preserve_whitespace("\n".join(formatted)),
css=".mdpopups .lsp_hover { margin: 4px; } .mdpopups p { margin: 0.1rem; }",
md=True,
"".join(formatted),
css='''
.lsp_hover .highlight {
border-width: 0;
}
.lsp_hover div.highlight,
.lsp_hover pre.highlight {
margin-bottom: 0;
}
.lsp_hover .description {
padding: 0.5rem 0.5rem 0 0.5rem;
}
''',
md=False,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
wrapper_class="lsp_hover",
Expand All @@ -2040,9 +2062,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