Re: [PATCH v1 4/6] vt/vt: Add SRG mouse reporting protocol

[Jiri Slaby <jirislaby@kernel.org>]

On 30. 06. 20, 9:11, Tammo Block wrote:
> The SRG protocol indicates a button release by appending a "m" to the
> report. In this case the button number is not 3 ("release state") but
> the number of the button that was released. As release event are only
> reported for the first three buttons, we need to store the number on
> click events because it is not sent to us from userspace.
> 
> We also need to check for the case where no button state change occurred
> at all (bit 6 set), in this case a value of 3 is OK even in SRG.
> 
> Bitmasks for your convenience:
> 195 - All bits related to any button number
> 227 - All bits related to button number and "state not changed" bit
> 252 - All, except low button numbers (left, middle, right button)

Ick, define macros for those (likely in 2/6 where you define the vc_data
members holding these bits) and use them below.

> Signed-off-by: Tammo Block <tammo.block@gmail.com>
> ---
>  drivers/tty/vt/vt.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index e2324d8e4e74..36520f7f0315 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -183,6 +183,9 @@ core_param(consoleblank, blankinterval, int, 0444);
>  static DECLARE_WORK(console_work, console_callback);
>  static DECLARE_WORK(con_driver_unregister_work, con_driver_unregister_callback);
>  
> +/* remember mouse state */
> +unsigned char last_button_pressed = 3;

So this can be static, right? Can this be in mouse_report or is it used
elsewhere? If it has to be global, a mouse_ prefix would be good.

>  /*
>   * fg_console is the current virtual console,
>   * last_console is the last used one,
> @@ -1840,11 +1843,21 @@ static inline void respond_ID(struct tty_struct *tty)
>  
>  void mouse_report(struct tty_struct *tty, int butt, int mrx, int mry)
>  {
> -	char buf[8];
> +	char buf[20];
> +	bool rel;
>  	int len;
>  
> -	len = sprintf(buf, "\033[M%c%c%c", (char)(' ' + butt),
> -			(char)('!' + mrx), (char)('!' + mry));
> +	if (vc_cons[fg_console].d->vc_protocol_mouse == 1) {	/* SRG*/

So 1 is SRG, 2 is URXVT, 0 is X10, right? We need constants for those,
or maybe enum and switch+case here? No need for the comments here then.

> +		/* For SRG release events, we send the last clicked button < 3 */
> +		rel = ((butt & 195) == 3);

No need for the outer parentheses.

> +		if ((butt & 195) < 3)
> +			last_button_pressed = butt & 3;
> +		if ((butt & 227) == 3)
> +			butt = (butt & 252) | last_button_pressed;
> +		len = sprintf(buf, "\033[<%d;%d;%d%c", butt, mrx + 1, mry + 1, rel ? 'm' : 'M');
> +	} else							/* X10 */
> +		len = sprintf(buf, "\033[M%c%c%c", (char)(' ' + butt),
> +				(char)('!' + mrx), (char)('!' + mry));
>  	respond_string(buf, len, tty->port);
>  }

thanks,
-- 
js
suse labs