scalameta / metals-sublime

Sublime Text package for Metals, a language server for Scala
https://packagecontrol.io/packages/LSP-metals
Apache License 2.0
16 stars 10 forks source link

Handle metals/publishDecorations (ST4 only) #36

Closed ayoub-benali closed 3 years ago

ayoub-benali commented 3 years ago

image

@ckipp01 @tgodzik

As you can see in the example above infered types work fine. But the implicit parameter (x) is off. It should be on the right i.e. f("hello")(x). Any idea why metals sends the wrong LSP region ?

This feature is limited to ST4 only: Comment

TODO:

ckipp01 commented 3 years ago

Any idea why metals sends the wrong LSP region ?

🤔 so I haven't seen them misplaced before either in the hovers or when decorations. I'm not fully sure without seeing the exact positions. One thing you may want to do if you have it is to make a test file and try to maybe match it up against running the same file in VS Code. Or at least that way with a file and the exact ranges we can try to see what is going wrong. When you looked at the LSP trace did the range seem incorrect?

ayoub-benali commented 3 years ago

This is the range I saw for the bug at line 11

{'start': {'line': 10, 'character': 2}, 'end': {'line': 10, 'character': 12}}

As you can see the character value for the start is wrong. IMO should 9 or 10.

I guess Metals calculates ranges differently for implicit arguments that is why the infered types ranges are correct. Maybe the behavior is geared towards VScode ?

tgodzik commented 3 years ago

The decorations in VS code show up after the end of the range, so that might be the difference. Start is not important there.

Ranges are actually take directly from semanticdb.

If it's impossible to change in Sublime we could fix it in Metals by setting start to the end.

ayoub-benali commented 3 years ago

The decorations in VS code show up after the end of the range, so that might be the difference.

That is probably it and there isn't much choice regarding Sublime API: https://www.sublimetext.com/docs/api_reference.html#sublime.Phantom:ver-dev

These are the layout options available :

I can only use "LAYOUT_INLINE"

ayoub-benali commented 3 years ago

The decorations in VS code show up after the end of the range, so that might be the difference. Start is not important there.

I did that and it fixed the problem :)

image

Since decoration is specific to VScode I don't mind having a special case in the client side.

Is it safer to actually use assign the end as start and make end be end + contentText length ?

tgodzik commented 3 years ago

Is it safer to actually use assign the end as start and make end be end + contentText length ?

Not sure, I think that depends on the editor. But if it works I would leave it as is.

ayoub-benali commented 3 years ago

@ckipp01 I have one more issue while testing another decoration use case; worksheets : I created a worksheet file in my project: image

But the client doesn't receive any decorations. Bellow is the LSP exchange:

::  -> metals textDocument/didChange: {'textDocument': {'uri': 'file:///home/ayoub/workspace/test-project/src/main/scala/example/hello.sc', 'version': 2}, 'contentChanges': [{'text': 'val x = 1.to(10).toList\n'}]}
::  -> metals textDocument/didSave: {'text': 'val x = 1.to(10).toList\n', 'textDocument': {'uri': 'file:///home/ayoub/workspace/test-project/src/main/scala/example/hello.sc'}}
:: <-  metals textDocument/publishDiagnostics: {'uri': 'file:///home/ayoub/workspace/test-project/src/main/scala/example/hello.sc', 'diagnostics': []}

You can see that the worksheet isn't evaluated :/

Is some action missing from the client ?

ckipp01 commented 3 years ago

@ckipp01 I have one more issue while testing another decoration use case; worksheets : I created a worksheet file in my project: image

But the client doesn't receive any decorations. Bellow is the LSP exchange:

::  -> metals textDocument/didChange: {'textDocument': {'uri': 'file:///home/ayoub/workspace/test-project/src/main/scala/example/hello.sc', 'version': 2}, 'contentChanges': [{'text': 'val x = 1.to(10).toList\n'}]}
::  -> metals textDocument/didSave: {'text': 'val x = 1.to(10).toList\n', 'textDocument': {'uri': 'file:///home/ayoub/workspace/test-project/src/main/scala/example/hello.sc'}}
:: <-  metals textDocument/publishDiagnostics: {'uri': 'file:///home/ayoub/workspace/test-project/src/main/scala/example/hello.sc', 'diagnostics': []}

You can see that the worksheet isn't evaluated :/

Is some action missing from the client ?

That's because it thinks you're in a Scala script, not a worksheet. Worksheets have to end with .worksheet.sc. Try adding in the .worksheet part and then you should see them.

ayoub-benali commented 3 years ago

My bad :facepalm:

image

1 - Are there any other use cases for decorations I should test ? 2 - how critical is the hoverMessage part ? because its implementation with the sublime API isn't trivial :/

ckipp01 commented 3 years ago

2 - how critical is the hoverMessage part ? because its implementation with the sublime API isn't trivial :/

In some cases, quite important. Mainly because if there is any println in the expression or the output is too long, they won't see it and have no way to see it if they can't hover on it.

rwols commented 3 years ago

In some cases, quite important. Mainly because if there is any println in the expression or the output is too long, they won't see it and have no way to see it if they can't hover on it.

Maybe we can stick it in a clickable annotation, which then shows a popup?

EDIT: Or maybe as a phantom with sublime.LAYOUT_BLOCK.

ayoub-benali commented 3 years ago

In some cases, quite important. Mainly because if there is any println in the expression or the output is too long, they won't see it and have no way to see it if they can't hover on it.

Maybe we can stick it in a clickable annotation, which then shows a popup?

EDIT: Or maybe as a phantom with sublime.LAYOUT_BLOCK.

Didn't manage to get a ViewEventListener working yet but I wonder if the on_hover would work because I don't know if it will detect the phantom hover. Otherwise we have to use a clickable annotation.

sublime.LAYOUT_BLOCK. isn't possible because we have to display phantoms inline

rwols commented 3 years ago

You can forget about handling this in ST3 as the api doesn’t allow it.

ayoub-benali commented 3 years ago

@ckipp01 @tgodzik adding a clickable button 'more` showing a popup is the best I could come up for now to handle the "hoverContent"

Peek 2021-02-13 17-32

@rwols I reused css().popups from LSP. Do you see any problem with that ?

EDIT: do you think it makes sense to show the button if the popup content is the same as the decoration. IMO it shouldn't be shown when there isn't extra data.