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

posframe-integration: dont create posframe on every request #465

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kiennq
Copy link
Member

@kiennq kiennq commented Jun 28, 2020

Per this #464 (comment)

@yyoncho
Copy link
Member

yyoncho commented Jun 28, 2020

cc @Sorixelle

@Sorixelle
Copy link
Contributor

Gave this PR a test on my setup, it looks like the windows aren't appearing in the correct place; almost a similar issue to what we were trying to solve with posframe in the first place.

Example

@kiennq
Copy link
Member Author

kiennq commented Jun 28, 2020

What's your configuration for lsp-ui-doc?
The change is simple, and from the logic it just stop calling lsp-ui-doc--delete-frame so the posframe-show works as expected and doesn't create new frame so often.
If there's problem, I doubt it would be problem on posframe itself (and previous child frame implementation), so just switch to posframe will not make the problem going away.

@Sorixelle
Copy link
Contributor

Sorixelle commented Jun 28, 2020

Actually, I think this is a problem on posframe. It seems like, from https://github.com/tumashu/posframe/blob/master/posframe.el#L703-L711, that the posframe's position is only updated when the position is different from last time (we're always passing the same poshandler so it doesn't change), and when the parent frame's size changes (which is unlikely to happen).

When you create a new frame, the variables that store the previous values are zeroed out (https://github.com/tumashu/posframe/blob/master/posframe.el#L306-L308), so the frame gets positioned properly.

@kiennq
Copy link
Member Author

kiennq commented Jun 28, 2020

@Sorixelle The issue is indeed inside posframe implementation, specifically with posframe--set-frame-position.
I guess you're setting the lsp-ui-doc-position to bottom, which will trigger the poshandler of posframe-poshandler-frame/window-bottom-right-corner.
posframe-show will call posframe--set-frame-position and since the position is the same (relative from right edge and bottom), it will not trigger set-frame-position again.
Well, the child frame size does changed, so without calling set-frame-position, the position will be wrong.

See this PR tumashu/posframe#65

@kiennq
Copy link
Member Author

kiennq commented Jun 28, 2020

@Sorixelle, I've created PR to fix problem on posframe repo, can you test it with your setup?
Note that problem is not happening with default lsp-ui setup that show popup at point

@Sorixelle
Copy link
Contributor

Sorixelle commented Jun 28, 2020

Positioning is better, but there also seems to be an issue where the window in the frame isn't correctly sized all the time. In this recording, once I focus the frame, I hold down l (I use evil) to move through the line. Moving the point right doesn't jump to the next line once it gets to the end, so it seems like the content of the buffer is all one line; just the window isn't fit to the frame and is too small to display it all.

Example

EDIT: Just checked in case, this issue is happening for me as well when lsp-ui-doc-position is set to 'at-point.

@kiennq
Copy link
Member Author

kiennq commented Jun 28, 2020

@Sorixelle What's your setup again? Can you have minimal repro for this?
Did this issue not repro without this change?
I doubt it may related to some of your special minor modes that automatically breaks line visually but doesn't inform Emacs so the fit-frame-to-buffer does not work.
Actually it seems that the size of frame is calculated based on all of your 3 lines as a single line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants