emacs-lsp / lsp-ui

UI integrations for lsp-mode
https://emacs-lsp.github.io/lsp-ui
GNU General Public License v3.0
1.04k stars 139 forks source link

Fix posframe usage for lsp-ui-doc #464

Open brotzeit opened 4 years ago

brotzeit commented 4 years ago

it causes the hover window to capture focus, making editing impossible.

The posframe integration on lsp-ui is slow, due to new frame is being created every time.

Sorixelle commented 4 years ago

In my testing, I didn't run into either of these issues while working on #459. If anyone is experiencing these issues, I'd like to get some more details on the environment this is occuring in so I can try and reproduce these.

dmakarov commented 4 years ago

This happens to me on macOS 10.15.5: GNU Emacs 26.3 (build 1, x86_64-apple-darwin18.2.0, NS appkit-1671.20 Version 10.14.3 (Build 18D109)) of 2019-09-02. I don't have any customizations for lsp-ui or posframe group.

ezgif com-video-to-gif

Here I move pointer to a method name, lsp-ui-doc pops up and it has focus. I press q to switch focus back to the original frame.

yyoncho commented 4 years ago

@alanz are you also using macos?

Sorixelle commented 4 years ago

Did some testing, and managed to get to the bottom of the focus issues. It seems to be an issue with Cocoa builds of Emacs. This issue doesn't exist in the emacs-mac port, so the solution would be to use this. It seems like this is an Emacs bug, and completely out of our control. More info in this issue in posframe.

yyoncho commented 4 years ago

It seems like this is an Emacs bug, and completely out of our control

It was working in the previous implementation, right?

Sorixelle commented 4 years ago

Just reverted the commit, it seems like it did work on Cocoa builds. I'm still unconvinced this is a problem on our side however, as I don't see any reason our code would cause the new frame to get focus.

dmakarov commented 4 years ago

This may be a macOS cocoa issue, but the previous non-posframe implementation did work.

Sorixelle commented 4 years ago

I've come up with a solution that works for Emacs 26 on macOS. However, this solution prevents using the mouse to focus the frame in Linux, and doesn't fix the issue in Emacs 27 or 28 on macOS (no clue why). Also of note is that in Emacs 28 in macOS, it seems to completely ignore positioning and just display in the top-left corner of the screen. If we're ok with losing the ability to focus with the mouse outside of macOS, I'll make a PR with this fix.

The solution is to not advise posframe--redirect-frame-focus, and just let it setup the focus redirect. lsp-ui-doc-focus-frame still works, but the mouse can still focus the frame in Emacs 26 for some reason.

tl;dr - Emacs on macOS is really, really weird.

EDIT: The Emacs 27/28 issues might have something to do with the obsoletion of focus-in-hook for after-focus-change-function. That should probably be reported to posframe.

yyoncho commented 4 years ago

IMO not getting the focus is better than getting the focus all the time. Ideally, we should not introduce regression compared to the previous behaviour of the popup.

alanz commented 4 years ago

@alanz are you also using macos?

Yes, the problem shows up on my work machine, macos. But not on my home machine, Debian Testing.

kiennq commented 4 years ago

@Sorixelle About this

The posframe integration on lsp-ui is slow, due to new frame is being created every time.

A simple profiling will reveal that

- #<compiled 0x347b065>                                                                         266  88%
 - lsp--parser-on-message                                                                       266  88%
  - #<compiled 0x4297c65>                                                                       266  88%
   - #<compiled 0x4297c49>                                                                      266  88%
    - #<compiled 0x4297c15>                                                                     266  88%
     - apply                                                                                    266  88%
      - #<compiled 0x4294071>                                                                   266  88%
       - lsp-ui-doc--callback                                                                   266  88%
        - lsp-ui-doc--display                                                                   266  88%
         - lsp-ui-doc--make-frame                                                               265  88%
          - posframe-show                                                                       260  86%
           - posframe--create-posframe                                                          255  85%
            - make-frame                                                                        255  85%
             - frame-creation-function                                                          255  85%
              - apply                                                                           255  85%
               - #<compiled 0x1072a75>                                                          255  85%
                + x-create-frame-with-faces                                                     255  85%
             posframe--set-frame-position                                                         2   0%
             posframe--mouse-banish                                                               1   0%
          + lsp-ui-doc--delete-frame                                                              5   1%
+ redisplay_internal (C function)                                                                25   8%
+ timer-event-handler                                                                             5   1%
+ ...                                                                                             2   0%
+ eldoc-pre-command-refresh-echo-area                                                             1   0%

It calls make-frame on every lsp-ui-doc--display request, which is slow on some system. Even the default implementation of posframe try to avoid that by associating an posframe--frame with every posframe buffer so it will not be created again.

In case of lsp-ui, why this happens is because of this check inside lsp-ui-doc--display.

      (when (or (not lsp-ui-doc-use-webkit)
                (not (lsp-ui-doc--get-frame)))
        (lsp-ui-doc--set-frame (lsp-ui-doc--make-frame)))

So just simply not set lsp-ui-doc-use-webkit will make this check passed and calling lsp-ui-doc--make-frame. Inside lsp-ui-doc--make-frame, it intentionally delete the posframe with lsp-ui-doc--delete-frame and causes new frame to be created everytime. I believe this check is mistyping and should change from or to and, or simply change to use unless and remove not instead of when.

However, just changing that will not work since the posframe position will not be updated. In the original code, the frame position is updated in every call to lsp-ui-doc--display with lsp-ui-doc--move-frame, we need to update that with posframe too (call posframe-show with new position, I guess).

Alternatively, change the check to check for lsp-ui-doc-use-webkit only and remove the delete frame from lsp-ui-doc--make-frame may work too. We will still calling stuff of setting up frame parameters every time though.

omidmnz commented 4 years ago

I have this problem on NixOS, so the issue is definitely not OSX-specific. But I'm using xmonad, so it might be a contributing factor.

seagle0128 commented 4 years ago

Same issue here after I upgraded today. It's pleasure to provide details if need.

danielmartin commented 4 years ago

IMO, if this issue is causing significant problems to some users, we should revert the PR. We can always re-land it later when it's more stable.

omidmnz commented 4 years ago

@danielmartin I think that's the correct thing to do now, since package.el does not officially support downgrades.

Sorixelle commented 4 years ago

Yeah, I'd be in favour of reverting it for now. Didn't expect it to be such a buggy mess when it was working with no issues at all on my system 😅

kurnevsky commented 4 years ago

I also have this problem on linux with xmonad :)

kiennq commented 4 years ago

Thanks @kurnevsky for answering. Can anyone who has problem with mouse being focused check the value of focus-follow-mouse? I think that's the problem causing mouse is being automatically focus into posframe

omidmnz commented 4 years ago
focus-follows-mouse is a variable defined in ‘C source code’.
Its value is nil

I'm not sure if this helps, but I am using flycheck-posframe without this problem.

P.S.: My xmonad config has focus-follows-mouse disabled too.

kurnevsky commented 4 years ago

Another bug I noticed - when I switch buffer when posframe is about to be shown - it's shown in another window with an error

Error processing message (wrong-type-argument frame-live-p #<dead frame  0x6649010>).
d1egoaz commented 4 years ago

I've come up with a solution that works for Emacs 26 on macOS. However, this solution prevents using the mouse to focus the frame in Linux, and doesn't fix the issue in Emacs 27 or 28 on macOS (no clue why). Also of note is that in Emacs 28 in macOS, it seems to completely ignore positioning and just display in the top-left corner of the screen. If we're ok with losing the ability to focus with the mouse outside of macOS, I'll make a PR with this fix.

The solution is to not advise posframe--redirect-frame-focus, and just let it setup the focus redirect. lsp-ui-doc-focus-frame still works, but the mouse can still focus the frame in Emacs 26 for some reason.

tl;dr - Emacs on macOS is really, really weird.

EDIT: The Emacs 27/28 issues might have something to do with the obsoletion of focus-in-hook for after-focus-change-function. That should probably be reported to posframe.

is there any hack to do in emacs 27/28 while something is fixed in posframe ?

d1egoaz commented 4 years ago

BTW, this works for me: https://github.com/emacs-lsp/lsp-ui/issues/414#issuecomment-601216646

but it adds some screen screen flickering at the top

yyoncho commented 4 years ago

I've come up with a solution that works for Emacs 26 on macOS. However, this solution prevents using the mouse to focus the frame in Linux, and doesn't fix the issue in Emacs 27 or 28 on macOS (no clue why). Also of note is that in Emacs 28 in macOS, it seems to completely ignore positioning and just display in the top-left corner of the screen. If we're ok with losing the ability to focus with the mouse outside of macOS, I'll make a PR with this fix.

The solution is to not advise posframe--redirect-frame-focus, and just let it setup the focus redirect. lsp-ui-doc-focus-frame still works, but the mouse can still focus the frame in Emacs 26 for some reason.

tl;dr - Emacs on macOS is really, really weird.

EDIT: The Emacs 27/28 issues might have something to do with the obsoletion of focus-in-hook for after-focus-change-function. That should probably be reported to posframe.

@Sorixelle - can you post your findings?

yyoncho commented 4 years ago

@Sorixelle - can you post your findings?

cc @ericdallo who will take a look.

joncol commented 4 years ago

Another xmonad user on Linux here. I ran into the same problem with the lsp-ui-doc-frame (sp?) stealing focus. It only seemed to happen if my mouse cursor was in a position that was overlaying the frame.

I tried both of the following in my setup, and then restarting Emacs. Either one worked to solve this problem (the doc frame doesn't get the focus now).

(setq focus-follows-mouse 'auto-raise)

or:

(setq focus-follows-mouse t)

Version info: GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.21, cairo version 1.17.3) of 2020-07-26

adimit commented 4 years ago

Same problem here on Fedora 32 with both XMonad and Gnome 3, so it doesn't seem to be XMonad specific. @joncol's fix didn't work for me.

Additionally, the focus does not get stolen on a fresh Emacs. I need to have set the focus to the doc popup once first, say, by clicking my mouse, or scrolling. This is also the case when opening new frames: when I first open a new frame (even if it's the same buffer) the doc popup does not steal my focus.

But once I've clicked into the doc popup even once, I cannot undo it, it seems, and every successive doc popup will steal the focus if it pops up under my mouse pointer. I cannot call lsp-ui-doc-hide to unset the focus stealing behaviour.

If I were to guess, I'd say there's some persistent state somewhere that gets set as soon as the doc frame is focused for the first time.

So I guess that's another workaround: open a new frame, close the current one.

Ah, but I did find another workaround: if I manually call lsp-ui-doc-unfocus-frame the focus stealing behaviour also ceases. Until I (accidentally) click into the doc popup again!

yyoncho commented 4 years ago

So I guess that's another workaround: open a new frame, close the current one.

This is not an option because creating a new frame is a slow operation. But we could call lsp-ui-doc-unfocus-frame before showing the popup.

adimit commented 4 years ago

Yeah, I meant it as a user-level workaround. Methinks calling lsp-ui-doc-unfocus-frame is the right way to go. I don't quite understand what select-frame-set-input-focus does that just clicking in the right frame doesn't, but that seems to be doing the trick (for me.)

adimit commented 3 years ago

An added bit of data: it seems to happen to me with all posframes, not just LSP ui doc. This includes company-posframe. I think it's just more noticable with the latter, as it's bigger in dimension.

So it's possibly a posframe issue, or a posframe/xmonad issue.

andrelaszlo commented 3 years ago

@joncol's solution works (tack!) until I give the frame focus (Arch/Xmonad) but I found that running lower-frame solves the issue until I click the frame again :slightly_smiling_face:

jcs090218 commented 3 years ago

I think we are using function lsp-ui-doc--move-frame to move the frame instead of creating a new one? Can this issue be closed? Thanks!

brotzeit commented 3 years ago

We are still using the old implementation, because there were some issues with the posframe code. I'd rather keep this open to track it ?