Re: Making TTY menus more visual

[Eli Zaretskii]

> Date: Thu, 08 Oct 2020 22:17:41 -0700
> From: Jared Finder <jared@finder.org>
> Cc: emacs-devel@gnu.org
> 
> Thanks for the detailed reasoning. I have updated my patches to only 
> conditionally call mouse-position-function, as requested.

Thanks.

> I've also added one additional patch that rebinds [menu-bar mouse-1]
> to get TTY menus by default.

I'm not sure I understand the need.  E.g., the mouse-equipped w32
console doesn't need this, and neither should the GPM mouse.  Can you
explain?  Does xterm-mouse alone need it?  Also, could this affect the
GUI frames in any way?

A few minor comments below, and then we will wait for your legal
paperwork to come through.

> +(defun menu-bar-open-mouse (event)
> +  "Mosue-triggered version of `menu-bar-open'.
      ^^^^^
"Mouse".  And the first line of any doc string should be a summary of
what the function does, and should reference the arguments.

> +This command is to be used when you click the mouse in the menubar."
> +  (interactive "e")
> +  (let* ((x-position (car (posn-x-y (event-start event))))
> +         (menu-bar-item-cons (menu-bar-item-at-x x-position)))
> +    (menu-bar-open nil
> +                   (if menu-bar-item-cons
> +                       (cdr menu-bar-item-cons)
> +                     0))))
> +
> +(defun menu-bar-keymap ()
> +  "Return the current menu-bar keymap.
> +
> +The ordering of the return value respects `menu-bar-final-items'."
> +  (let ((menu-bar '())
> +        (menu-end '()))
> +    (map-keymap
> +     (lambda (key binding)
> +       (let ((pos (seq-position menu-bar-final-items key))
> +             (menu-item (cons key binding)))
> +         (if pos
> +             ;; If KEY is the name of an item that we want to put
> +             ;; last, store it separately with explicit ordering for
> +             ;; sorting.
> +             (push (cons pos menu-item) menu-end)
> +           (push menu-item menu-bar))))
> +     (menu-bar-get-binding [menu-bar]))
> +    `(keymap ,@(nreverse menu-bar)
> +             ,@(mapcar #'cdr (sort menu-end
> +                                   (lambda (a b)
> +                                     (< (car a) (car b))))))))
> +
> +(defun menu-bar-get-binding (keyseq)
> +  "Return the current binding of KEYSEQ, merging prefix definitions.

This should somehow mention the menu bar, otherwise it's too general.

> +However, for the menu bar itself, the value does not take account
> +of `menu-bar-final-items'."           ^^^^^^^^^^^^^^^^^^^^^^^^^^^

"take into account" instead of "take account of", I presume?

> +(defun menu-bar-item-at-x (x-position)
> +  "Return a cons of the form (KEY . X) for the item clicked on.

Can we say that X here is the X coordinate of the click?

> ++++
> +** New variable 'tty-menu-calls-mouse-position-function'.
> +This controls if TTY menu logic calls mouse-position-function. Libraries that
                                                                ^^
In documentation, doc strings, and comments we use US English
convention of leaving 2 spaces between sentences.

> @@ -321,7 +325,8 @@ xterm-mouse-mode
>    (if xterm-mouse-mode
>        ;; Turn it on
>        (progn
> -     (setq mouse-position-function #'xterm-mouse-position-function)
> +        (setq mouse-position-function #'xterm-mouse-position-function
> +              tty-menu-calls-mouse-position-function t)

Can we bind tty-menu-calls-mouse-position-function only around the
code that uses the menus?  If that's not feasible, we should at least
reset it to nil when the mode is switched off.

> +  if (EQ (selected_frame, XCAR(mouse)))
                                 ^
Our style is to leave one space between the function/macro name and
the opening parenthesis that follows.

> +TTY menus support mouse navigation and selection when xterm-mouse-mode
> +is active. When run in a terminal, clicking on the menu bar with the
> +mouse now pops up a TTY menu by default instead of running the command
> +'tmm-menubar'. To restore the old behavior, set the variable
> +'tty-menu-open-use-tmm' to non-nil.

Same issue with 2 spaces between sentences.

Thanks again for working on this.