[PATCH 2/2] Fix axis labelling for mixed devices.

[Chase Douglas]

On 01/17/2012 11:11 PM, Peter Hutterer wrote:
> On Tue, Jan 17, 2012 at 06:34:38PM +0100, Chase Douglas wrote:
>> On 01/17/2012 06:05 AM, Peter Hutterer wrote:
>>> If a device has both relative and absolute axes, we'd initialise the
>>> relative axes but label them with the absolute labels.
>>>
>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>> ---
>>>  src/evdev.c |   25 ++++++++++++++++++-------
>>>  1 files changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/evdev.c b/src/evdev.c
>>> index effac40..1cdba41 100644
>>> --- a/src/evdev.c
>>> +++ b/src/evdev.c
>>> @@ -120,7 +120,7 @@ static BOOL EvdevGrabDevice(InputInfoPtr pInfo, int grab, int ungrab);
>>>  static void EvdevSetCalibration(InputInfoPtr pInfo, int num_calibration, int calibration[4]);
>>>  static int EvdevOpenDevice(InputInfoPtr pInfo);
>>>  
>>> -static void EvdevInitAxesLabels(EvdevPtr pEvdev, int natoms, Atom *atoms);
>>> +static void EvdevInitAxesLabels(EvdevPtr pEvdev, int mode, int natoms, Atom *atoms);
>>>  static void EvdevInitButtonLabels(EvdevPtr pEvdev, int natoms, Atom *atoms);
>>>  static void EvdevInitProperty(DeviceIntPtr dev);
>>>  static int EvdevSetProperty(DeviceIntPtr dev, Atom atom,
>>> @@ -1297,7 +1297,7 @@ EvdevAddAbsValuatorClass(DeviceIntPtr device)
>>>              i++;
>>>      }
>>>  
>>> -    EvdevInitAxesLabels(pEvdev, pEvdev->num_vals + num_mt_axes, atoms);
>>> +    EvdevInitAxesLabels(pEvdev, Absolute, pEvdev->num_vals + num_mt_axes, atoms);
>>>  
>>>      if (!InitValuatorClassDeviceStruct(device, num_axes + num_mt_axes, atoms,
>>>                                         GetMotionHistorySize(), Absolute)) {
>>> @@ -1496,7 +1496,7 @@ EvdevAddRelValuatorClass(DeviceIntPtr device)
>>>          i++;
>>>      }
>>>  
>>> -    EvdevInitAxesLabels(pEvdev, pEvdev->num_vals, atoms);
>>> +    EvdevInitAxesLabels(pEvdev, Relative, pEvdev->num_vals, atoms);
>>>  
>>>      if (!InitValuatorClassDeviceStruct(device, num_axes, atoms,
>>>                                         GetMotionHistorySize(), Relative)) {
>>> @@ -2637,18 +2637,18 @@ static char* btn_labels[][16] = {
>>>      }
>>>  };
>>>  
>>> -static void EvdevInitAxesLabels(EvdevPtr pEvdev, int natoms, Atom *atoms)
>>> +static void EvdevInitAxesLabels(EvdevPtr pEvdev, int mode, int natoms, Atom *atoms)
>>>  {
>>>      Atom atom;
>>>      int axis;
>>>      char **labels;
>>>      int labels_len = 0;
>>>  
>>> -    if (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS)
>>> +    if (mode == Absolute)
>>>      {
>>>          labels     = abs_labels;
>>>          labels_len = ArrayLength(abs_labels);
>>> -    } else if ((pEvdev->flags & EVDEV_RELATIVE_EVENTS))
>>> +    } else if (mode == Relative)
>>>      {
>>>          labels     = rel_labels;
>>>          labels_len = ArrayLength(rel_labels);
>>> @@ -2810,8 +2810,19 @@ EvdevInitProperty(DeviceIntPtr dev)
>>>          /* Axis labelling */
>>>          if ((pEvdev->num_vals > 0) && (prop_axis_label = XIGetKnownProperty(AXIS_LABEL_PROP)))
>>>          {
>>> +            int mode;
>>>              Atom atoms[pEvdev->num_vals];
>>> -            EvdevInitAxesLabels(pEvdev, pEvdev->num_vals, atoms);
>>> +
>>> +            if (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS)
>>> +                mode = Absolute;
>>> +            else if (pEvdev->flags & EVDEV_RELATIVE_EVENTS)
>>> +                mode = Relative;
>>> +            else {
>>> +                xf86IDrvMsg(pInfo, X_ERROR, "BUG: mode is neither absolute nor relative\n");
>>> +                mode = Absolute;
>>> +            }
>>> +
>>> +            EvdevInitAxesLabels(pEvdev, mode, pEvdev->num_vals, atoms);
>>>              XIChangeDeviceProperty(dev, prop_axis_label, XA_ATOM, 32,
>>>                                     PropModeReplace, pEvdev->num_vals, atoms, FALSE);
>>>              XISetDevicePropertyDeletable(dev, prop_axis_label, FALSE);
>>
>> How does this fix mixed mode devices? We're still only initializing the
>> array for either relative or absolute, but not both.
> 
> it doesn't yet, but tbh I don't think that's the only issue we have with
> mixed mode devices. one day I'll get to it, but that day ain't today.

Ok, that's reasonable. So the change really is that we default to
relative labels if both absolute and relative axes are available.

If so, then the commit message is a bit misleading. The subject makes it
sound like we really are fixing things. The message assumes the reader
knows that we want relative axes to be correct over absolute axes. I
think the following would be better:

====

Prefer relative axis labelling over absolute axis labelling

The current code is broken for mixed mode devices. Most of these devices
operate primarily in relative mode, but have some absolute axes
available for secondary functionality. For now, label the relative axes
properly. We can fix the absolute axes later.

====

I'm fine with the code, so with a fixed commit message:

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>