tumashu / ivy-posframe

ivy-posframe is a ivy extension, which let ivy use posframe to show its candidate menu, ivy-posframe is a **GNU ELPA** package.
415 stars 26 forks source link

Regression : not usable with tty and gui emacsen under same daemon #69

Closed gagbo closed 5 years ago

gagbo commented 5 years ago

Hello,

An earlier PR in the project got the tty and gui emacsen to work under the same daemon. We think we found a small regression in the recent change to use a macro to define the advices. As discussed on this doom-emacs issue, the macro looks like it lacks a fallback when display-graphic-p is nil, so when running a tty emacs, ivy-posframe doesn't work at all.

I don't know at all how macros work (as you can read in the linked issue above :) ), so I'm not entirely sure how it would be fixable. I guess using if instead of when is a start, but then the false case needs to be handled.

Sorry if this seems too vague, I can give more details if needed.

Cheers

conao3 commented 5 years ago

I don't use tty Emacs and GUI Emacs at the same time, so I couldn't understand the necessary situation during the recent discussion. For me, the situation setting is strange in the first place.

It may seem the same work to you, but please tell us the detailed reproduction steps and environment. I can't start working right away because I'm busy now, but I can start as soon as I have time, and @tumashu also can do it when the problem becomes clear.

tumashu commented 5 years ago

@gagbo Please test this macro define. @conao3

(defmacro ivy-posframe--defun-advice (name arglist &optional docstring &rest body)
  "Define NAME as a `ivy-posframe' advice function.  see `defun'.
The definition is (lambda ARGLIST [DOCSTRING] BODY...).
See also the function `interactive'.
DECL is a declaration, optional, of the form (declare DECLS...) where
DECLS is a list of elements of the form (PROP . VALUES).  These are
interpreted according to `defun-declarations-alist'.
The return value is undefined.

\(fn NAME ARGLIST &optional DOCSTRING DECL &rest BODY)"
  (declare (doc-string 3) (indent 2))
  (let ((decls (cond
                ((eq (car-safe docstring) 'declare)
                 (prog1 (cdr docstring) (setq docstring nil)))
                ((and (stringp docstring)
                      (eq (car-safe (car body)) 'declare))
                 (prog1 (cdr (car body)) (setq body (cdr body)))))))
    `(defun ,name ,arglist
       ,(when (stringp docstring) docstring)
       (declare ,@decls)
       (if (display-graphic-p)
           (progn ,@body)
         (funcall ,(car arglist)
                  ,@(cdr (remove '&rest (remove '&optional arglist))))))))
gagbo commented 5 years ago

It may seem the same work to you, but please tell us the detailed reproduction steps and environment.

I'm on macOS (emacs-plus brew with --HEAD flag, from 2 weeks ago), but I think you'll be able to reproduce with any environment, since I was able to reproduce it in Linux/emacs 26 (as packaged in Fedora 30 repos) as well.

  1. Start a daemonized emacs server
  2. Start a tty emacsen connected to this server. Both 1. and 2. can be done with ALTERNATE_EDITOR="" emacsclient --tty
  3. Invoke any function which should create an ivy posframe (like M-x)

@gagbo Please test this macro define.

Will do

gagbo commented 5 years ago

With the new macro :

  1. Everything still works fine in GUI
  2. M-x (and other) fail in tty with Wrong number of arguments (2 . 2), 1
  3. Trying to get a backtrace with toggle-debug-on-error just crashes emacs instead of showing me the backtrace so I'll probably test this more on linux later to see if I can get the backtrace
tumashu commented 5 years ago

@gagbo edit (funcall to (apply, and try again

tumashu commented 5 years ago

@gagbo @conao3 Should be fixed, please try again, I have removed the macro ivy-posframe--defun-advice, for it is complicated to deal with this problem.

gagbo commented 5 years ago

Changing the funcall to apply did seem to work fine though

tumashu commented 5 years ago

@gagbo try this change: https://github.com/tumashu/ivy-posframe/commit/d9ceee94171767b4aba6c55ebe93e51ccbe0fa8a

gagbo commented 5 years ago

I also get random crashes (sometimes it works fine - e.g. posframe in gui, and classic ivy in a tty besides - but sometimes I just get a crash), but this time I can't be sure it's not my setup, I should be able to test this more tonight.

gagbo commented 5 years ago

Can confirm it works for mw now