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

posfram--set-frame-position: also set position when frame size changed #65

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

kiennq
Copy link
Contributor

@kiennq kiennq commented Jun 28, 2020

With poshandler set to posframe-poshandler-frame-bottom-right-corner, the posframe will not be displayed properly once a bigger frame has been displayed.
To repo this issue, try this snippet

(cl-labels ((show (content time)
              (run-at-time time nil
                           (lambda (content)
                             (with-current-buffer (get-buffer-create "*a*")
                               (erase-buffer)
                               (insert content))
                             (posframe-show (get-buffer-create "*a*")
                                            :poshandler #'posframe-poshandler-frame-bottom-right-corner
                                            :internal-border-width 1
                                            :internal-border-color "white"))
                           content)))
  (show "aaaaa" 1)
  (show "aa" 2))

And how it looks like
74db63eb-9ab9-4ac9-af00-029f2ba52525

The problem is 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.

We should also call set-frame-position when the frame displayed size is changed.

How it looks like after the fix
c77dfd64-0acc-45c1-aa00-fb12f64a53da

@kiennq
Copy link
Contributor Author

kiennq commented Jun 29, 2020

@tumashu

@tumashu
Copy link
Owner

tumashu commented Jun 29, 2020

do you have signed gpl paper? posframe is gnu-elpa package, need it

@kiennq
Copy link
Contributor Author

kiennq commented Jun 29, 2020

do you have signed gpl paper? posframe is gnu-elpa package, need it

Yes, I've signed it.

@tumashu
Copy link
Owner

tumashu commented Jun 29, 2020

(defvar-local posframe--last-posframe-size nil
  "Record the last size of posframe's frame.")

(defvar-local posframe--last-posframe-displayed-size nil
  "Record the last displayed size of posframe's frame.")

The two variable seem to do same job?

@kiennq
Copy link
Contributor Author

kiennq commented Jun 29, 2020

(defvar-local posframe--last-posframe-size nil
  "Record the last size of posframe's frame.")

(defvar-local posframe--last-posframe-displayed-size nil
  "Record the last displayed size of posframe's frame.")

The two variable seem to do same job?

No, the posframe--last-posframe-size is recording max-height, min-height stuffs that are passed from caller to fit-frame-to-buffer. It's not guaranteed to be the same with actual posframe size.
On other hand, posframe--last-posframe-displayed-size is recording frame size after fit-frame-to-buffer. It's located in posframe--set-frame-position so it can be used to check if the frame size has been changed and re-trigger set-frame-position.

@tumashu
Copy link
Owner

tumashu commented Jun 29, 2020

maybe we can use posframe--last-posframe-size-1 and posframe--last-posframe-size-2, and show the different in docstring.

@kiennq
Copy link
Contributor Author

kiennq commented Jun 29, 2020

maybe we can use posframe--last-posframe-size-1 and posframe--last-posframe-size-2, and show the different in docstring.

Hmm, shouldn't the name itself should be explainable? It's no problem for me to change the name like that, but it make code much harder to look.
On other hand, with current naming of displayed-size, it's clearly that this is posframe displayed size, and other thing is something else.

@tumashu tumashu merged commit 922e4d2 into tumashu:master Jun 29, 2020
@tumashu
Copy link
Owner

tumashu commented Jun 29, 2020

merged, thanks!

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.

2 participants