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

[Tammo Block <tammo.block@gmail.com>]

Hi Jiri,

thanks for your patience ... ;-)

Am Do., 2. Juli 2020 um 10:48 Uhr schrieb Jiri Slaby <jirislaby@kernel.org>:
>
> On 01. 07. 20, 17:13, 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 (RELEASEEVENT) but
> > the number of the button that was released. As release event are only
> > reported for the first three buttons (LOWBUTTONMASK), 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.
> >
> > Signed-off-by: Tammo Block <tammo.block@gmail.com>
> > ---
> >  drivers/tty/vt/vt.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 9abf2829b1d3..9aae3eac7989 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -1838,15 +1838,34 @@ static inline void respond_ID(struct tty_struct *tty)
> >       respond_string(vt102_id, strlen(vt102_id), tty->port);
> >  }
> >
> > +#define ANYBUTTONMASK        0xc3
> > +#define LOWBUTTONMASK        0x03
>
> Insert _ before MASK.
>
> > +#define RELEASEEVENT 0x03
> > +
> >  enum { Mouse_X10 = 0, Mouse_SRG, Mouse_URXVT};
>
> = 0 in unnecessary. And put one per line. And all capitals as
> CodingStyle says.
>

OK, I was just copying the style of other enums in vt.c.
But the code seems to be older than the coding style guide ... ;-)

> You should name the enum somehow and use it as a type for
> vc_protocol_mouse (you'd need a forward declaration of the enum in the
> header).
>

That would make vc_protocol_mouse an enum and vc_report_mouse a define.
As vc_report_mouse is visible from userspace those defines live in tiocl.h.

Wouldn't it be more consistent to make both of them the same type and also use
defines for vc_report_mouse? And if so: Where to place them?

They are only needed inside vt.c, so they could live in any of:
1.) Inside vt.c
2.) in vt.h
3.) in tiocl.h, although userspace does not need them.

> >  void mouse_report(struct tty_struct *tty, int butt, int mrx, int mry)
> >  {
> > -     char buf[8];
> > +     static char last_btn = RELEASEEVENT;
> > +     char buf[20];
> > +     bool rel;
> >       int len;
> >
> > -     len = sprintf(buf, "\033[M%c%c%c", (char)(' ' + butt),
> > -                     (char)('!' + mrx), (char)('!' + mry));
> > +     switch (vc_cons[fg_console].d->vc_protocol_mouse) {
> > +             case Mouse_SRG:
>
> This is not how we indent switch-case.
>
> > +                     rel = (butt & ANYBUTTONMASK) == RELEASEEVENT;
> > +                     if ((butt & ANYBUTTONMASK) < RELEASEEVENT)
> > +                             last_btn = butt & LOWBUTTONMASK;
> > +                     if ((butt & TIOCL_SELBUTTONMASK) == RELEASEEVENT)
> > +                             butt = (butt & ~LOWBUTTONMASK) | last_btn;
> > +                     len = sprintf(buf, "\033[<%d;%d;%d%c", butt,
> > +                                     mrx + 1, mry + 1, rel ? 'm' : 'M');
> > +                     break;
> > +             default:
> > +                     len = sprintf(buf, "\033[M%c%c%c", (char)(' ' + butt),
> > +                                     (char)('!' + mrx), (char)('!' + mry));
> > +                     break;
> > +     }
> >       respond_string(buf, len, tty->port);
> >  }
>
> thanks,
> --
> js
> suse labs

Thanks again,
Tammo