Re: Making TTY menus more visual

[Eli Zaretskii]

> Date: Sun, 04 Oct 2020 22:36:21 -0700
> From: Jared Finder <jared@finder.org>
> Cc: emacs-devel@gnu.org
> 
> > This is okay as a general idea, but it would be more clean to make
> > Fmouse_position call a new C function (extracted from the first part
> > of Fmouse_position's code) that just computes the mouse coordinates.
> > Then Fmouse_position's would call mouse-position-function after the
> > new C function returns.  Then in term.c we could call just that new C
> > function.  This is because it is inappropriate for mouse_get_xy to
> > call mouse-position-function when a menu is being processed.
> 
> That won't work. Making mouse_get_xy call mouse-position-function is the 
> purpose of this change. mouse-position-function is how xterm-mouse 
> injects its calculated mouse positions to the TTY menus. From searching 
> the Emacs codebase for usage of mouse-position-function, it appears that 
> xterm-mouse is the only client. Of course external libraries could use 
> it too, but I assume they would have the same goal.

Okay, but mouse-position-function can be used for any number of
purposes.  That it's currently used only by xterm-mouse in the Emacs
sources doesn't mean it isn't used elsewhere.  The functions in that
variable are not necessarily meant to be called in the context of TTY
menus; that would be an incompatible change.  So I still think we
shouldn't call mouse-position-function in general in the TTY menu
case, only when xterm-mouse is active.  Please change the code to call
mouse-position-function only in that single case.

Perhaps a slightly more general way of doing that would be to
introduce another variable that a package must set for
mouse-position-function to be called in the context of TTY menus;
xterm-mouse will then bind that variable (or the TTY menu code in
menu-bar.el could do that for xterm-mouse).  This way, we don't
hard-code xterm-mouse in the C sources.

> New patches attached. BTW, is it helpful to have this split up into two 
> patches (three if you count my bug fix)? I did it this way to make 
> reviewing and accepting easier, but if you would rather have one bigger 
> patch, I'm fine doing that too.

It doesn't matter much.  I personally prefer a single patch for each
changeset that makes sense on its own right.

A few minor comments below.

> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index 781a8c5a93..1d7321f0e3 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -54,7 +54,7 @@
>  ;;; Code:
>  
>  (eval-when-compile (require 'cl-lib))
> -(declare-function tmm-menubar-keymap "tmm.el")
> +(declare-function menu-bar-keymap "menu-bar.el")

Why did you need this part?  menu-bar.el is preloaded, so the function
menu-bar-keymap should be known to Emacs.

> +(defun menu-bar-open-mouse (event)
> +  "Mosue-triggered version of `menu-bar-open'.
> +This command is to be used when you click the mouse in the menubar."
> +  (interactive "e")
> +  (require 'tmm)       ; Possibly have tmm depend on menu-bar instead?
     ^^^^^^^^^^^^^^
I'd like to avoid this.  menu-bar.el is preloaded, so the above will
make tmm.el preloaded, for no good reason from my POV.

Can you explain why you needed to require tmm?  (Doing this the other
way around is definitely okay, since menu-bar.el is preloaded.)

> +(defun menu-bar-get-keybind (keyseq)
                       ^^^^^^^
"keybinding".  Or just "binding".

> diff --git a/lisp/tmm.el b/lisp/tmm.el
> index 074ee7593f..b73af9cda3 100644
> --- a/lisp/tmm.el
> +++ b/lisp/tmm.el
> @@ -28,6 +28,8 @@
>  ;;; Code:
>  
>  (require 'electric)
> +(declare-function menu-bar-keymap "menu-bar.el")
> +(declare-function menu-bar-item-at-x "menu-bar.el")

Again, these two shouldn't be needed, since menu-bar.el is preloaded.

Last, but not least: this needs a NEWS entry, announcing TTY menus
support by xterm-mouse.

Thanks.