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

Ivy-posframe now minor-mode #43

Merged
merged 38 commits into from Jun 3, 2019
Merged

Conversation

conao3
Copy link
Collaborator

@conao3 conao3 commented Jun 2, 2019

Yesterday I made another package related to posframe. The package uses minor mode and can be safely turned on and off.

I used that experience to redefine ivy-posframe as a minor mode using the same mechanism.
I changed a lot of code, so it would be helpful if you could check if it works in your environment.

As noted in the README, this variable is used for configuration.

(setq ivy-posframe-configure-alist
      '((ivy-display-functions-alist . ((swiper          . nil)
                                        (complete-symbol . ivy-posframe-display-at-point)
                                        (counsel-M-x     . ivy-posframe-display-at-window-bottom-left)
                                        (t               . ivy-posframe-display)))))

And if you have changed the variable for ivy, delete that line.

diff --git a/.dotfiles/.emacs.d/init.el b/.dotfiles/.emacs.d/init.el
index ee400d0..7ab77bf 100644
--- a/.dotfiles/.emacs.d/init.el
+++ b/.dotfiles/.emacs.d/init.el
@@ -855,9 +855,9 @@
         :doc "Using posframe to show Ivy"
         :when (version<= "26.1" emacs-version)
         :when window-system
         :ensure t
-        :custom ((ivy-height              40)
-                 (ivy-display-function    . #'ivy-posframe-display-at-frame-center)
-                 (ivy-posframe-parameters . '((left-fringe . 10))))
+        :custom ((ivy-posframe-parameters . '((left-fringe . 10))))

Then turn on ivy-posframe-mode, test it, and turn off it.

@tumashu
Copy link
Owner

tumashu commented Jun 2, 2019

I do not think it is a good idea, two reason:

  1. it make ivy-posframe's code very complicated, hard to understand
  2. this minor-mode is not real minor-mode in my opinion, for minor-mode should not setq any ivy's custom variable

@tumashu
Copy link
Owner

tumashu commented Jun 2, 2019

If we want to a safe minor-mode, a simply way is that create ivy-posframe-minibuffer-map and advice ivy-read to override ivy-minibuffer-map

@conao3
Copy link
Collaborator Author

conao3 commented Jun 2, 2019

Solved define-key issue, but there are still places where I set the value of ivy-display-functions-alist.
I'd like to add some advice to ivy-alist-setting, but It doesn't know how to change the return value because the function is passed value and It doesn't know which symbol was passed.

@conao3
Copy link
Collaborator Author

conao3 commented Jun 2, 2019

Squashed tmp commit.

Solved overwrite variable issue. But this approach to advice is vulnerable to changes in ivy, where our job is to add just one more function that references ivy-display-functions-alist, for example.

The commit history of this PR is recorded along with the reason for all the changes, but it is a little too much, so please tell me if you want to squash it.

@tumashu
Copy link
Owner

tumashu commented Jun 2, 2019

this piece is not look good, what about use macro, i do not suggest use eval in this package

defvar ivy-posframe-display-function-alist
  '((window-center      . window-center)
    (frame-center       . frame-center)
    (window-bottom-left . window-bottom-left-corner)
    (frame-bottom-left  . frame-bottom-left-corner)
    (point              . point-bottom-left-corner)))

(eval
 `(progn
    ,@(mapcar
       (lambda (elm)
         `(defun ,(intern (format "ivy-posframe-display-at-%s" (car elm))) (str)
            ,(format "Display STR via `posframe' at %s." (car elm))
            (ivy-posframe--display str #',(intern (format "posframe-poshandler-%s" (cdr elm))))))
       ivy-posframe-display-function-alist)))

@tumashu
Copy link
Owner

tumashu commented Jun 2, 2019

by the way, point-bottom-left-corner is not function, while variable is named function alist

@conao3
Copy link
Collaborator Author

conao3 commented Jun 2, 2019

point-bottom-left-corner is part of (posframe-poshandler-)point-bottom-left-corner, it is function, right?

@tumashu
Copy link
Owner

tumashu commented Jun 2, 2019

suggest use posframe-poshandler-point-bottom-left-corner instead, part function name is not valid function name

@tumashu
Copy link
Owner

tumashu commented Jun 2, 2019

please deal this eval

 (if ivy-posframe-mode
        (eval
         `(progn
            ,@(mapcar (lambda (elm) `(advice-add ',(car elm) :around #',(cdr elm))) advices)))
      (eval
       `(progn
          ,@(mapcar (lambda (elm) `(advice-remove ',(car elm) #',(cdr elm))) advices))))))

@tumashu
Copy link
Owner

tumashu commented Jun 2, 2019

add ivy-posframe-enable as alias of ivy-posframe-mode

@conao3
Copy link
Collaborator Author

conao3 commented Jun 3, 2019

Fixed.

@tumashu tumashu merged commit 9872f35 into tumashu:master Jun 3, 2019
@tumashu
Copy link
Owner

tumashu commented Jun 3, 2019

ok, good job!

@conao3 conao3 deleted the ivy-posframe-mode branch June 3, 2019 03:47
@tumashu
Copy link
Owner

tumashu commented Jun 3, 2019

@conao3 I have simplify code a lot, so suggest pull newest code before hack, by the way , add your info to ivy-posframe.el's head

@conao3
Copy link
Collaborator Author

conao3 commented Jun 3, 2019

OK. In what role do I add?
Add the following line after you?

Maintainer: Naoya Yamashita <conao3@gmail.com>

@tumashu
Copy link
Owner

tumashu commented Jun 3, 2019

ok

@conao3
Copy link
Collaborator Author

conao3 commented Jun 3, 2019

Did it. Thanks!

@conao3 conao3 mentioned this pull request Jun 30, 2019
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

2 participants