Re: [RFC 1/6] Input: Rename extra mouse buttons defines to match their actual usage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

On Wed, Apr 03, 2019 at 05:37:11PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 03-04-19 16:12, Benjamin Tissoires wrote:
> > On Mon, Apr 1, 2019 at 1:11 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > > 
> > > Hi,
> > > 
> > > On 01-04-19 12:34, Peter Hutterer wrote:
> > > > On Mon, Apr 01, 2019 at 09:16:22AM +0200, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 01-04-19 05:05, Peter Hutterer wrote:
> > > > > > On Sat, Mar 30, 2019 at 02:37:30PM +0100, Hans de Goede wrote:
> > > > > > > Almost all modern mice use HID. For buttons 1 - 3 the order of left /
> > > > > > > right / middle specified in the HUT matches with the BTN_LEFT, BTN_RIGHT
> > > > > > > and BTN_MIDDLE order of the evdev code defines since the HUT does not
> > > > > > > specify any meaning for button usages >= 4 the HID input driver simply
> > > > > > > maps buttons to BTN_LEFT + (number - 1).
> > > > > > > 
> > > > > > > In practice on most mice which have more then 3 buttons, usage 4 is
> > > > > > > used for the back(ward) button and usage 5 for the forward button.
> > > > > > > 
> > > > > > > This means that these buttons generate an input_event code of 0x113 resp.
> > > > > > > 0x114, which looking at the old defines maps to BTN_SIDE and BTN_EXTRA.
> > > > > > > 
> > > > > > > Under X these are mapped to buttons 8 resp. 9; and under wayland the
> > > > > > > input_event code is passed through unmodified. Apps, e.g. Firefox both
> > > > > > > when running as Wayland client under GNOME3 and when running under Xorg,
> > > > > > > correctly interpret these as back and forward.
> > > > > > > 
> > > > > > > So we've back and forward buttons generating BTN_SIDE and BTN_EXTRA,
> > > > > > > which the apps then interpret as BTN_BACK and BTN_FORWARD, rather then as
> > > > > > > BTN_SIDE and BTN_EXTRA (which have no clear meaning).
> > > > > > > 
> > > > > > > I find this all very confusing, this commit tries to remove the confusion
> > > > > > > by deprecating the old defines and adding new defines which assign labels
> > > > > > > to the 0x113 - 0x116 input_event codes which match how they are actually
> > > > > > > used today.
> > > > > > > 
> > > > > > > Note there are no functional changes here, after this userspace will
> > > > > > > see the exact same input_event codes as before, this purely about
> > > > > > > assigning a human-readable label to these codes which matches with how
> > > > > > > they are actually being used.
> > > > > > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > > > > > ---
> > > > > > >     Documentation/input/event-codes.rst    |  2 +-
> > > > > > >     drivers/hid/hid-debug.c                |  6 +++---
> > > > > > >     drivers/input/mousedev.c               |  8 ++++----
> > > > > > >     include/uapi/linux/input-event-codes.h | 13 ++++++++++++-
> > > > > > >     4 files changed, 20 insertions(+), 9 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
> > > > > > > index b24b5343f5eb..db52b96e7a83 100644
> > > > > > > --- a/Documentation/input/event-codes.rst
> > > > > > > +++ b/Documentation/input/event-codes.rst
> > > > > > > @@ -422,7 +422,7 @@ should be used to report when the tool is in contact with the tablet.
> > > > > > >     BTN_{STYLUS,STYLUS2} should be used to report buttons on the tool itself. Any
> > > > > > >     button may be used for buttons on the tablet except BTN_{MOUSE,LEFT}.
> > > > > > >     BTN_{0,1,2,etc} are good generic codes for unlabeled buttons. Do not use
> > > > > > > -meaningful buttons, like BTN_FORWARD, unless the button is labeled for that
> > > > > > > +meaningful buttons, like BTN_FORWRD, unless the button is labeled for that
> > > > > > 
> > > > > > I haven't thought of a better name yet, but using BTN_FORWRD is bound to
> > > > > > introduce bugs, especially when BTN_FORWARD still resolves. I don't think
> > > > > > that name is a good idea but I don't have a better suggestion yet.
> > > > > 
> > > > > Good point about the typos my plan for the kernel is to add #ifndef __KERNEL__
> > > > > around the compat defines once all users are moved over.
> > > > 
> > > > good, but not enough. the kernel is easiest here because you could literally
> > > > just add a script to a test suite to make sure it doesn't use BTN_FORWARD.
> > > > It's all the userspace that'll introduce typos.
> > > > 
> > > > > > Thought of the day: if HID doesn't have any meaning, is it really worth
> > > > > > changing these names based on what you hope a client interprets it as?
> > > > > > would a more generic naming scheme that admits this is just button 4 be
> > > > > > better? BTN_HID_4 or something?
> > > > > 
> > > > > The HUT spec may not say anything about this (note I did not check the
> > > > > gazillion errata I really wish they would just fold those into an
> > > > > updated version), but Microsoft writes the following about this and it
> > > > > is reasonable to expect mice vendors to follow this (and in practice they do):
> > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/keyboard-and-mouse-hid-client-drivers
> > > > > 
> > > > > "Buttons 4 & 5 on are mapped to WM_APPCOMMAND messages and correspond to App_Back and App_Forward."
> > > > 
> > > > BTN_APP_FORWARD and BTN_APP_BACK then? seems obvious enough and is a lot
> > > > easier to visually identify than a missing A in FORWRD.
> > > 
> > > Yes I was thinking the same about 5 minutes after sending the mail you
> > > replied to, so I think we agree on this, will fix for the the first non
> > > RFC version.
> > 
> > For the record, I also agree with BTN_APP_FORWARD and BTN_APP_BACK.
> > 
> > We made some archeology this morning with Peter.
> > So now is story time, gather around children:
> > BTN_SIDE and BTN_EXTRA were added in 2.3.36 with patch-2.3.36.bz2 that
> > can be found at
> > https://mirrors.edge.kernel.org/pub/linux/kernel/v2.3/.
> > 
> > It's actually the big switch from Vojtech from a custom USB mouse boot
> > protocol to an actual HID bus. It's also the creation of the evdev
> > protocol.
> > 
> > And those BTN_* were added to suport mice with extra buttons. I guess
> > it was pretty uncommon for such mice in Jan 2000 and the buttons were
> > not commonly used like today as back and forward.
> > 
> > Note that it's actually easy to go back to 2.4.0 by using:
> > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/?ofs=63400
> > 
> > So, my fears that the buttons would have a meaning is relieved, and I
> > would Ack such a series (I haven't fully read it so it's an ACK on the
> > principle).
> 
> Ok, so before I send a v1 of this patch-set, Dmitry do you have any remarks?

I played with my Evoluent Vertical mouse and I found that it reports
BTN_EXTRA for button under scroll wheel, BTN_SIDE for the button on the
side of the mouse and BTN_FORWARD for the 2nd button on the side. This
means that not all mice have these buttons in forward/back positions...

Thanks.

-- 
Dmitry



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux