bug#29552: 27.0.50; [PATCH]: improved xterm mouse support

[Eli Zaretskii <eliz@gnu.org>]

> From: Olaf Rogalsky <olaf.rogalsky@t-online.de>
> Date: Mon, 04 Dec 2017 01:08:01 +0100
> 
> as a follow up to bug#29104, I try to improve on the following two
> limitations of xterm-mouse-mode.
> 
> 1. Clicks on the menubar only work partialy: they open up the
>    left-most menu-bar item. After that one has to use the keyboard
>    controls in order to navigate through the menus.
> 
> 2. There is no highlighting of text with mouse-face property, when the
>    mouse pointer is moved over those texts.
> 
> With this patch selection of menus with the mouse and highlighting of
> text with mouse-face properties work as expected. For a demonstration
> of the new features, please follow this URL:
> https://youtu.be/1pp85OshsoA.

Thanks for working on this, Olaf.

> 1. menu-bar.el @@ -2460,6 +2460,31
> 
>    The new function `tty-menu-bar-open-with-mouse' takes a mouse event
>    parameter and opens the menu-bar menu at the corresponding
>    postion. The similar function `menu-bar-open' has no event
>    parameter as argument and is therefore not suitable for mouse
>    clicks.

This has an unfortunate consequence of adding yet another way of
opening a menu-bar menu.  Is there any reasonable way of doing the
same like GPM, the w32 console, and the non-toolkit builds do it,
i.e. generate a MOUSE_CLICK event and get it handled as we do in
keyboard.c around line 5620?  I'd prefer not to add yet another subtle
handling of these events if that can be avoided.

> 8. term.c @@ -793,8 +793,6
>    term.c @@ -851,7 +849,6
>    term.c @@ -2371,9 +2368,7
>    term.c @@ -2387,7 +2382,7
>    term.c @@ -2421,6 +2416,7
>    xdisp.c @@ -29568,9 +29568,7
> 
>    Since this patch makes use of `tty_write_glyphs_with_face' and
>    `tty_draw_row_with_mouse_face', which are otherwise only used by
>    GPM, I make them generally available by taking them out of
>    '#ifdef HAVE_GPM'.

The w32 and the MSDOS terminals have their own versions of these
functions, so defining them unconditionally will cause compilation
errors on those platforms, because they also compile term.c.

Also, this hunk:

> diff --git a/src/xdisp.c b/src/xdisp.c
> index 016e7044ca..96fba761a6 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -29568,9 +29568,7 @@ draw_row_with_mouse_face (struct window *w, int start_x, struct glyph_row *row,
>        return;
>      }
>  #endif
> -#if defined (HAVE_GPM) || defined (MSDOS) || defined (WINDOWSNT)
>    tty_draw_row_with_mouse_face (w, row, start_hpos, end_hpos, draw);
> -#endif
>  }

will now be in effect for NS, for example.  Do all the platforms we
support have everything that tty_draw_row_with_mouse_face uses?

And I see frame.c calls term_mouse_moveto, but you left it conditioned
on GPM.  Why?

> +DEFUN ("note-mouse-highlight", Fnote_mouse_highlight, Snote_mouse_highlight,
> +       3, 3, 0,
> +       doc: /* Take proper action when the mouse has moved to position
> +X, Y on frame F with regards to highlighting portions of display that
> +have mouse-face properties.  Also de-highlight portions of display
> +where the mouse was before, set the mouse pointer shape as appropriate
> +for the mouse coordinates, and activate help echo (tooltips).  X and Y
> +can be negative or out of range.  */)

What is perhaps appropriate for a code comment is not necessarily
appropriate for a Lisp-level doc string, so this needs to be reworked.
For starters, the first sentence should fit on a single screen line.
Next, the frame parameter's name is FRAME, not F.  Finally, some of
what the text says describes the internal working of the function and
shouldn't appear in the doc string.

I would also prefer this function to be internal, so let's call it
something like tty--note-mouse-highlight-internal, and let's say in
the doc string that it's for internal use only.

> +  CHECK_FRAME(frame);
> +  CHECK_NUMBER(x);
> +  CHECK_NUMBER(y);

Please leave one blank between the name of the function/macro and the
opening parenthesis.

> +  /* silently do nothing, if frame is not on a terminal */

Comments should be full sentences: started with a capital letter, and
ending with a period.  Also, please leave 2 blanks between the last
period and the closing "*/" of the comment.

> +  if (XFRAME(frame)->output_method != output_termcap &&
> +      XFRAME(frame)->output_method != output_msdos_raw)

Please use FRAME_TERMCAP_P and FRAME_MSDOS_P here.

> +  note_mouse_highlight(XFRAME(frame), XINT(x), XINT(y));
> +  fflush_unlocked(FRAME_TTY(XFRAME(frame))->output);

Please compute XFRAME (frame) only once and then reuse the value.

Last, but not least: the user-visible functionality introduced by this
should be mentioned in NEWS.

Thanks again for working on these issues.