redhat-developer / lsp4ij

LSP Client for IntelliJ
Eclipse Public License 2.0
48 stars 9 forks source link

feat: Support syntax coloration for code block in LSP hover #360

Closed angelozerr closed 2 weeks ago

angelozerr commented 2 weeks ago

Support syntax coloration for code block in LSP hover

Fixes #297

image

Do you think it is correct CppCXY?

I copied / pasted some piece of code from QuickDocHighlightingHelper and it seems it is working with Language. For TextMate Language is not enough (because Language is equals to "textmate). I have implemented TextMate support too.

image

image

RustHover

image

fbricon commented 2 weeks ago

the code block content is duplicated in your screenshot, or you can compare the following

Screenshot 2024-06-17 at 00 29 50

with the JetBrains version:

angelozerr commented 2 weeks ago

the code block content is duplicated in your screenshot, or you can compare the following

Good catch, my translate of kotlin to Java code was bad, it should be fixed now (I have updated screenshot).

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 55.30303% with 118 lines in your changes missing coverage. Please review.

Project coverage is 24.44%. Comparing base (d4d186d) to head (5c4fef8). Report is 2 commits behind head on main.

:exclamation: Current head 5c4fef8 differs from pull request most recent head 9a8da99

Please upload reports for the commit 9a8da99 to get more accurate results.

Files Patch % Lines
...features/documentation/LSPDocumentationHelper.java 40.00% 29 Missing and 10 partials :warning:
...documentation/LightQuickDocHighlightingHelper.java 41.46% 18 Missing and 6 partials :warning:
...cumentation/SyntaxColorationCodeBlockRenderer.java 70.49% 11 Missing and 7 partials :warning:
...tures/documentation/TextMateHighlighterHelper.java 61.11% 9 Missing and 5 partials :warning:
...4ij/features/completion/LSPCompletionProposal.java 0.00% 12 Missing :warning:
...edhat/devtools/lsp4ij/LanguageServersRegistry.java 80.00% 1 Missing and 2 partials :warning:
...dhat/devtools/lsp4ij/commands/CommandExecutor.java 0.00% 3 Missing :warning:
...m/redhat/devtools/lsp4ij/ServerMessageHandler.java 0.00% 2 Missing :warning:
...ols/lsp4ij/launching/ui/ShowInstructionDialog.java 0.00% 2 Missing :warning:
...hat/devtools/lsp4ij/client/LanguageClientImpl.java 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #360 +/- ## ========================================== + Coverage 22.99% 24.44% +1.45% ========================================== Files 267 270 +3 Lines 9369 9410 +41 Branches 1728 1734 +6 ========================================== + Hits 2154 2300 +146 + Misses 6809 6685 -124 - Partials 406 425 +19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

InSyncWithFoo commented 2 weeks ago

Thanks. Glad to know that I'll be able to customize hover support using something more than just a boolean. For comparison, here's how the native API renders the same signature:

Notable differences:

fbricon commented 2 weeks ago

Testing this PR's build on Pycharm, with based-pyright enabled:

Screenshot 2024-06-17 at 10 55 25

VS stock Pycharm:

Screenshot 2024-06-17 at 10 55 01

There's a chance the (function) prefix based-pyright adds is disrupting the built-in textmate grammar, causing the style discrepancy with stock pycharm.

As for the missing monospace font, is it a big deal?

InSyncWithFoo commented 2 weeks ago

As for the missing monospace font, is it a big deal?

It is. A Python function may have many parameters, with type hints for each.

Take json.dump() as example:

def dump(
    obj: Any,
    fp: SupportsWrite[str],
    *,
    skipkeys: bool = False,
    ensure_ascii: bool = True,
    check_circular: bool = True,
    allow_nan: bool = True,
    cls: type[JSONEncoder] | None = None,
    indent: None | int | str = None,
    separators: tuple[str, str] | None = None,
    default: Callable[[Any], Any] | None = None,
    sort_keys: bool = False,
    **kwds: Any,
) -> None: ...

Not crazy enough? Check out rich.console.Console.__init__():

def __init__(
    self,
    *,
    color_system: Optional[
        Literal["auto", "standard", "256", "truecolor", "windows"]
    ] = "auto",
    force_terminal: Optional[bool] = None,
    force_jupyter: Optional[bool] = None,
    force_interactive: Optional[bool] = None,
    soft_wrap: bool = False,
    theme: Optional[Theme] = None,
    stderr: bool = False,
    file: Optional[IO[str]] = None,
    quiet: bool = False,
    width: Optional[int] = None,
    height: Optional[int] = None,
    style: Optional[StyleType] = None,
    no_color: Optional[bool] = None,
    tab_size: int = 8,
    record: bool = False,
    markup: bool = True,
    emoji: bool = True,
    emoji_variant: Optional[EmojiVariant] = None,
    highlight: bool = True,
    log_time: bool = True,
    log_path: bool = True,
    log_time_format: Union[str, FormatTimeCallable] = "[%X]",
    highlighter: Optional["HighlighterType"] = ReprHighlighter(),
    legacy_windows: Optional[bool] = None,
    safe_box: bool = True,
    get_datetime: Optional[Callable[[], datetime]] = None,
    get_time: Optional[Callable[[], float]] = None,
    _environ: Optional[Mapping[str, str]] = None,
):

For the main part, these signatures are code, and (unless you are into that kind of thing) code are meant to be displayed in monospace font.

fbricon commented 2 weeks ago

it's still readable

Screenshot 2024-06-17 at 11 34 44
InSyncWithFoo commented 2 weeks ago

I, for one, largely prefer it this way, and I think dyslexic people would agree:

Readability notwithstanding, here's how the block is sent via LSP (pseudo-JSON):

{
  "result": {
    "contents": {
      "kind": "markdown",
      "value": """
        ```python
        (function) def dumps(
            obj: Any,
            *,
            skipkeys: bool = False,
            ...
        ) -> str
    ---
    Serialize `obj` to a JSON formatted `str`.

    If `skipkeys` is true then `dict` keys that are not basic types [...]
  """
},
"range": {/* ... */}

}



That message should, obviously, be rendered as one <em>code</em> block followed by a horizontal ruler and multiple paragraphs:

> ```python
> (function) def dumps(
>     obj: Any,
>     *,
>     skipkeys: bool = False,
>     ...
> ) -> str
> ```
> ---
> Serialize `obj` to a JSON formatted `str`.
> 
> If `skipkeys` is true then `dict` keys that are not basic types [...]
fbricon commented 2 weeks ago

There you go:

Screenshot 2024-06-17 at 12 01 39

just needed to wrap the code rendering with <pre>

angelozerr commented 2 weeks ago

@fbricon if you want to test quickly this PR, you can create a ts file with documentation like:

/**

 xml: <foo></foo>
 java: String s = new String("abed");
  > const s = "";
def dump(
    obj: Any,
    fp: SupportsWrite[str],
    *,
    skipkeys: bool = False,
    ensure_ascii: bool = True,
    check_circular: bool = True,
    allow_nan: bool = True,
    cls: type[JSONEncoder] | None = None,
    indent: None | int | str = None,
    separators: tuple[str, str] | None = None,
    default: Callable[[Any], Any] | None = None,
    sort_keys: bool = False,
    **kwds: Any,
) -> None: ...

**/

function foo() { }

foo();

and hover the foo:

image

fbricon commented 2 weeks ago

Thanks @angelozerr!