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

proposal: x/tools/cmd/gopls: provide config option for hover documentation placement #33352

Closed
myitcv opened this issue Jul 29, 2019 · 13 comments
Closed

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jul 29, 2019

What version of Go are you using (go version)?

$ go version
go version devel +c4ca60313e Sun Jul 28 18:09:57 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20190727173135-db2fa46ec33c
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.4-0.20190727173135-db2fa46ec33c

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build108929039=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I believe (from a quick test) that VSCode places hover-popups above-and-to-the-right of the mouse.

In Vim such popups have historically been below-and-to-the-right.

The text returned for a hover is as follows:

Printf formats according to a format specifier and writes to standard output.
It returns the number of bytes written and any write error encountered.

func fmt.Printf(format string, a ...interface{}) (n int, err error)

This ordering works well when the popup is show above-and-to-the-right, because the signature is near the identifier that trigger the hover.

It doesn't work so well for below-and-to-the-right popup placement, because the distance between signature and identifier is at its maximum.

Could I suggest therefore we have a config option to configure this, to place the type before/after the documentation?

Related discussion: govim/govim#421

cc @stamblerre @ianthehat

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 2, 2019

I have an open issue with LSP about textDocument/hover - I mentioned this there. I feel like it's something that could be fixed really easily in the LSP. I'm a bit wary to add extra configurations like that to gopls, but depending on how quickly changes are actually made in LSP, I might be more open to it.

@rsc rsc changed the title x/tools/cmd/gopls: proposal: provide config option for hover documentation placement proposal: x/tools/cmd/gopls: provide config option for hover documentation placement Aug 6, 2019
@gopherbot gopherbot added the Proposal label Aug 6, 2019
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 8, 2019

Thinking about #33550 made me realize that there are some additional issues here.

  1. Clients will probably want to vary the placement of the hover pop-up per-request, meaning that clients will also want to vary the ordering of documentation per-request. This will have to be done as a change to the LSP. Adding a configuration to gopls will only change the general "rule", but it won't offer per-request flexibility.
  2. There are issues with the ordering even in the top-right configuration. If there is a lot of documentation, as with the json.Unmarshal example provided in #33550, the client (in that example, VSCode), may limit the size of the hover window, forcing users to scroll to see the signature. Perhaps a solution would be to switch the ordering when the amount of documentation exceeds a certain length. Still, the size of the hover window could vary between clients, so having a strict rule is not the ideal solution.
@inliquid
Copy link

@inliquid inliquid commented Aug 10, 2019

Sorry I don't understand, why don't just make it in same way, as it was done in all other versions/editors listed in #33550: sourcegraph's go-langserver, bingo, and in the default vscode-go config with godoc or gogetdoc, (btw, including Goland quick documentation feature), signature at the top and the rest is scrollable documentation text?
Right now it makes a problem with viewing docs:
изображение
Why not fix it to normal order, as it should be? For example here is how it looks like in VS Code when turning off gopls and switching back to simple godoc:
изображение

@myitcv
Copy link
Member Author

@myitcv myitcv commented Aug 12, 2019

Clients will probably want to vary the placement of the hover pop-up per-request

Small nit: I'd say "possibly" rather than "probably". Reason being, popup placement (at least in my experience with govim) is generally fixed per function. i.e. hovers go above-and-to-the-right, completion candidates below-and-to-the-right, etc.

Completely agree this is more complex than simply putting the signature at the top or bottom.

Perhaps a solution would be to switch the ordering when the amount of documentation exceeds a certain length

I think we need to determine the structure of what we are returning. There are effectively two parts:

  • the type/signature
  • the associated documentation

Currently the hover API bundles all this content together, which I think is part of the issue. The ultimate UI/UX from my perspective would be to have the type/signature always visible, and the associated documentation scroll as required. This might well be implemented using two popups; one for the type/signature, the other (immediately below/above) a max-height X-lines scrollable popup the same width as the type/signature popup, but I only mention this in passing because I think the structure point needs to be established first, the implementation is just a detail

This might even make my request to have the type/signature "at the bottom" redundant, because the max-height of the scrolling would mean the distance between the hover trigger and the type/signature is limited.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 12, 2019

@inliquid: I think my main issue with this solution is that, in some cases, as @myitcv said, it's nicer to have the signature of the function pop-up right next to the function call in your code (so, at the bottom).

I certainly prefer the solution of having 2 windows, and I really do think that the best fix is to have structured types for hover. Perhaps a reasonable temporary solution is to add a configuration, and meanwhile to work with the LSP folks on resolving microsoft/language-server-protocol#772.

@inliquid
Copy link

@inliquid inliquid commented Aug 12, 2019

Sorry I don't understand "it's nicer to have the signature of the function pop-up right next to the function call in your code (so, at the bottom)". Where is it "nicer"? I was wondering why it's moved to the bottom from the very beginning with a single line doc approach, now it just made UX even worse than it was, because user have to scroll every time text is long enough.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 12, 2019

From @myitcv's comment:

This ordering works well when the popup is show above-and-to-the-right, because the signature is near the identifier that trigger the hover.

It doesn't work so well for below-and-to-the-right popup placement, because the distance between signature and identifier is at its maximum.

It can be helpful to minimize the distance between the identifier and the signature, to make things clearer to the user. I agree that this is not always the case, particularly when the documentation is long.

My current opinion on this issue is that we should:

  1. Work with the LSP folks on providing structured hover results.
  2. In the meantime, offer some configuration to the user. My thinking is that the signature should go on top if the level of documentation is FullDocumentation, and then on the bottom for the other cases.
@myitcv
Copy link
Member Author

@myitcv myitcv commented Aug 12, 2019

Another option: provide a config option that would tell gopls to return a JSON-structured response, including signature, one-liner, synopsis and full documentation. This would effectively allow us to prototype this without any changes to the LSP spec, adding more weight to your proposal @stamblerre in microsoft/language-server-protocol#772 (comment)

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 12, 2019

Change https://golang.org/cl/189943 mentions this issue: internal/lsp: change ordering of hover depending on hoverKind

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 12, 2019

Just mailed the change to handle part 2 of my suggestions, so @inliquid, you will see the updated behavior on master once it is merged.

@myitcv: Thanks for the suggestion - I think that's a good approach to start with.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 12, 2019

Change https://golang.org/cl/189977 mentions this issue: internal/lsp: support an experimental structured hover format

gopherbot pushed a commit to golang/tools that referenced this issue Aug 12, 2019
This change configures the ordering of documentation on hover. If the
user has requested full documentation, the signature appears at the top,
to avoid the user having to scroll. Otherwise, the signature appears at
the bottom, to minimize the distance between the triggering identifier
and the signature.

Updates golang/go#33352

Change-Id: I017baaabd0ee0c31cb13cb6abdda296782929823
Reviewed-on: https://go-review.googlesource.com/c/tools/+/189943
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Aug 12, 2019
Updates golang/go#33352

Change-Id: Ibf18e2529c9ba8c94c66942ea6f2c27f047ed285
Reviewed-on: https://go-review.googlesource.com/c/tools/+/189977
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@rsc
Copy link
Contributor

@rsc rsc commented Aug 20, 2019

It sounds like this discussion has converged to a result. Is this proposal issue still worth keeping open?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 20, 2019

I think we can close this. We added workarounds to gopls to handle different cases for hover, and we can wait on responses from the LSP team on microsoft/language-server-protocol#772.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.