Use posframe library for lsp-ui-doc #459
Conversation
Can you elaborate on that? |
|
The posframe doesn't get resized correctly with the webkit xwidget in it - it just stays as a one character wide and tall block. I haven't yet been able to determine why this is. I'd like to make this work properly before we merge, however. |
|
Just pushed some changes that uses posframe to manage webkit buffers. After a bit of discussion in the Gitter room, I've also made posframe a required dependency. Interested to hear the general community's thoughts in this. |
|
Pushed a set of changes resolving the review and the point placement. |
Sorixelle
changed the title
|
@kiennq I pushed a commit that uses advice instead of adding/removing hooks. I didn't run into the issue you described in my testing, so let me know if this fixes the issue. |
|
I've tested it. Seems to work well. |
|
Please don't abandon support for terminal users. |
From memory, terminal Emacs doesn't support creating child frames at all, so this PR isn't going to break anything that wasn't already not working in the terminal. |
|
@Sorixelle Using lsp-ui from current master in a terminal I currently use lsp-ui to show doc for functions. I just checked out this PR and instead I get the message about requiring graphical mode, so this PR would definitely be a regression for me. |
| "Set the buffer with STRING." | ||
| (defvar lsp-ui-doc--render-string nil | ||
| "The string to render in the documentation popup.") | ||
| (defvar lsp-ui-doc--render-symbol nil |
There was a problem hiding this comment.
Why you need this two global variables? The original version already pass symbol and string, why need to change function signature?
There was a problem hiding this comment.
I needed to call lsp-ui-doc--render-buffer from the xwidget-event callback lambda to solve the problem where webkit frames would be empty the first time they popped up.
Lines 651 to 658 in 61651ba
I can't just pass the parameters down to it, because it's only called when the webkit widget is first created, and since it's a lambda, it'll just capture the first values it sees, and won't update on subsequent hovers.
There was a problem hiding this comment.
I needed to call
lsp-ui-doc--render-bufferfrom thexwidget-eventcallback lambda to solve the problem where webkit frames would be empty the first time they popped up
Interesting. I wonder how just call lsp-ui-doc--render-buffer will resolve that problem?
From what I can see, the global vars are updated in lsp-ui-doc--display, which in turn will also call lsp-ui-doc--render-buffer.
There was a problem hiding this comment.
The thing is, lsp-ui-doc--render-buffer inserts the text directly into the webkit widget via a javascript call, so it needs to be called after both the webkit widget has been inserted, and after lsp-ui-doc.html has finished loading. That last one in particular isn't guaranteed by the time lsp-ui-doc--display has finished executing, but that 'load-changed event is fired specifically when the page has finished loading, so it's the best thing for us to use, unless there's something else I'm missing.
|
Are there more open issues preventing merging this PR? @Sorixelle can you check the CI? Seems like it is using something that is already deleted. |
|
Nothing that I can think of at the moment. Not sure what's going on with the CI - it's pointing to the wrong line number, and failing on a warning. I just moved the minor mode declaration for |
|
I'll merge this and take a look at the test the next days(the tests are now failing for a different reason). @Sorixelle Thanks! |
|
FYI I have reverted this in my local usage, it causes the hover window to capture focus, making editing impossible. |
|
This isn't an issue I ran into in my testing. If you've got time, open an issue with some extra detail so we can look into it. |
|
Any benefit from this integration? I don't see any value here, and it introduces many bugs 😭 |
| (mouse-wheel-frame . nil) | ||
| (no-other-frame . t) | ||
| (no-accept-focus . nil) |
There was a problem hiding this comment.
Why was it added explicitly?
@alanz you can set it to t to fix focus captures.
Added: though it might not work...
There was a problem hiding this comment.
In our original child frame implementation, we removed the no-accept-focus so we can focus into child frame to scroll the document. There is little no issue with that approach though, i.e: mouse is not suddenly set focus to other frame.
However, posframe add no-accept-focus to t explicitly, so we need to add back no-accept-focus . nil here
|
Just asking, but what's the value of your |
|
Please revert this PR before it's ready. |
If it's a question to me then I set it to |
Webkit buffers don't work in posframe windows, so the old manual framemanagement is kept around for webkit support.
lsp-ui-doc-borderwas removed, since the internal border is now usedto pad the frame.
lsp-ui-doc-modenow signals an error if Emacs is older than version 26or is running in the terminal, as posframe does not work under those
conditions.
Closes #331, fixes #334.