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

Enhancement: Keep whitespace displayed in popups #112

Merged
merged 2 commits into from
Sep 19, 2017
Merged

Enhancement: Keep whitespace displayed in popups #112

merged 2 commits into from
Sep 19, 2017

Conversation

deathaxe
Copy link
Contributor

mdpopups' markdown engine squashes down all multiple spaces to one.

This PR prepares the content to preserve them all and make popup content look a bit better.

mdpopups' markdown engine squashes down all multiple spaces to one.
This commit prepares the content to preserve them all and make popup content look a bit better.
@tomv564
Copy link
Contributor

tomv564 commented Sep 18, 2017

Could you share an example (e.g. before/after screenshot?)

Especially as there is also issue #105 and PR #111 ongoing, I'd be unfortunate if 3 people were working on improving popups in different ways.

@deathaxe
Copy link
Contributor Author

It is related to #105 yes, even though I havn't been not following that issue. Just realized it myself.

before

screenshot_before

now

screenshot

@deathaxe
Copy link
Contributor Author

deathaxe commented Sep 19, 2017

If you ask me, signature/docstring popups require their own rendering module/package to be able to handle different languages and formats and create a common user experience. This is just a very short and quick hotfix.

  • The signature itself should be highlighted.
  • Some keywords like the Arguments: etc. should be highlighted (see: Jedi - Autocompletion package) same with other formats like doxygen, ...

@rwols
Copy link
Member

rwols commented Sep 19, 2017

Yeah, I sort of agree with @deathaxe in that signature popups may vary too much between servers/languages.

It could be an idea to make LSP itself a PC package that can be added as a dependency, and then provide language-specific overrides for various servers/languages. This is how it's handled in VSCode and Atom, for example.

@deathaxe
Copy link
Contributor Author

This approach would split the server/protocol specific communication infrastructure which is some kind of backend away from presenting the results and handle user interactions which would be the frontend.

No matter how it is solved, but I think some infrastructure which splits apart the the protocol and communication stuff away from language-specific handling is required. But I like the idea to have one package, which is at least capable to provide a minimum set of functions. If you ask someone to create a frontend for each language, you'll probably not find one.

Have a look onto the new Python Debugger package. It should better be named as Sublime Debugger as it does the same which LSP does. It provides an frontend/backend infrastructure to attach nearly each debugger to ST. I wonder if one day someone will start using it to create new debugger backends. I am in doupt with that (unfortunately).

Copy link
Contributor

@tomv564 tomv564 left a comment

Choose a reason for hiding this comment

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

Solid improvement, thanks!

main.py Outdated
@@ -1721,6 +1721,14 @@ def show_hover(self, point, contents):
max_width=800)


def format_popup_content(contents: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid confusion about what "formatting" really means, could you rename this to something like preserve_whitespace or encode_whitespace (or something else to your liking?)

@tomv564 tomv564 merged commit bbd5130 into sublimelsp:master Sep 19, 2017
@tomv564
Copy link
Contributor

tomv564 commented Sep 19, 2017

🤘

@deathaxe deathaxe deleted the pr/popup-whitespace branch September 20, 2017 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants