[PATCH v3 3/3] xfree86: Introduce InputClass configuration

[Peter Hutterer]

On Mon, Dec 14, 2009 at 09:09:02PM -0800, Dan Nicholson wrote:
> Currently Xorg uses hal's fdi files to decide what configuration options
> are applied to automatically added input devices. This is sub-optimal
> since it requires users to use a new and different configuration store
> than xorg.conf.
> 
> The InputClass section attempts to provide a system similar to hal where
> configuration can be applied to all devices with certain attributes. For
> now, devices can be matched to:
> 
> * A substring of the product name via a MatchProduct entry
> * A substring of the vendir name via a MatchVendor entry
> * A pathname pattern of the device file via a MatchDevicePath entry
> * A device type via boolean entries for MatchIsKeyboard, MatchIsPointer,
>   MatchIsJoystick, MatchIsTablet, MatchIsTouchpad and MatchIsTouchscreen
> 
> See the INPUTCLASS section in xorg.conf(5) for more details.
> 
> Signed-off-by: Dan Nicholson <dbn.lists at gmail.com>

I like it. I like it a lot, and I'd love to see this in 1.8.
I've got a few comments on the patch, the basic principle is working but
there are still some issues with the details of the approach (for extra fun
I tested with an xorg.conf.d).

> +
> +/*
> + * Merge in any InputClass configurations. Each InputClass section can
> + * add to the original device configuration as well as any previous
> + * InputClass sections.
> + */
> +static int
> +MergeInputClasses(IDevPtr idev, InputAttributes *attrs)
> +{
> +    XF86ConfInputClassPtr cl;
> +    XF86OptionPtr classopts;
> +
> +    for (cl = xf86configptr->conf_inputclass_lst; cl; cl = cl->list.next) {
> +        if (!InputClassMatches(cl, attrs))
> +            continue;
> +
> +        xf86Msg(X_CONFIG, "%s: Applying InputClass \"%s\"\n",
> +                idev->identifier, cl->identifier);
> +        if (cl->driver && !idev->driver) {

i'm not sure about this. with the current HAL code there's a a bit of a
dependency chain with the patch set as it is here:
the hal code only calls NIDR for devices with the x11_driver set. this
driver value is copied into idev->driver before MergeInputClasses is called.
which means that MIC is only ever called for devices with the driver already
set, making the "Driver" option in the input classes useless.

looking at the man page where it says 'the "inputdriver" module
from  the  final  Driver  entry will be enabled' I think you had the right
intentions, just the wrong condition here.
IMO, xorg.conf should always override HAL anyway to guarantee a smooth
transition. (note to possible commenters: xorg.conf devices take a different
path and aren't affected by this at all)

anyway, your driver selection also contrasts with "Each class can only
supplement settings from a previous class, so it is best to arrange the
sections  with the most generic matches last." seems like the options are
taken from the first, the driver from the last?
i think the replacing approach may be better instead of the supplementing
approach, it allows for very basic catchall rules that are then modified and
refined, similar to what we have now (evdev for all, synaptics overwrites
for some). 

also, it's hard to run out of numbers for filenames so one can easily stack
to the end of a chain but it's harder to stack to the front of a chain.
i.e. adding a 9999-special-option.conf is easier than a <need smaller
zero>-special-option.conf.


> +            idev->driver = xstrdup(cl->driver);
> +            if (!idev->driver) {
> +                xf86Msg(X_ERROR, "Could not allocate memory while merging "
> +                        "InputClass configuration");

nitpick: I prefer "Failed to do something" over "Could not something", it
seems more expressive and grep-able.

> +                return BadAlloc;
> +            }
> +        }
> +
> +        classopts = xf86optionListDup(cl->option_lst);
> +        if (idev->commonOptions)
> +            idev->commonOptions = xf86optionListMerge(classopts,
> +                                                      idev->commonOptions);

this should be the other way round, otherwise the class options are replaced
with the commonOptions which makes it suprisingly hard to set simple things
like the keyboard layout through the class matching (it's already set in 
HAL). This goes hand in hand with the above comment.

> +        else
> +            idev->commonOptions = classopts;
> +    }
> +
> +    return Success;
> +}
> +
>  /**
>   * Create a new input device, activate and enable it.
>   *
> diff --git a/hw/xfree86/parser/Configint.h b/hw/xfree86/parser/Configint.h
> index 03509b3..80b5f00 100644
> --- a/hw/xfree86/parser/Configint.h
> +++ b/hw/xfree86/parser/Configint.h
> @@ -203,6 +203,8 @@ else\
>  "This section must have only one of either %s line."
>  #define UNDEFINED_INPUTDRIVER_MSG \
>  "InputDevice section \"%s\" must have a Driver line."

why is this a condition? some drivers share the same options so it'd make
sense to be able to share settings across drivers. unless it's really hard
to do so, I'd make the driver line optional too.

> +#define UNDEFINED_INPUTCLASSDRIVER_MSG \
> +"InputClass section \"%s\" must have a Driver line."
>  #define INVALID_GAMMA_MSG \
>  "gamma correction value(s) expected\n either one value or three r/g/b values."
>  #define GROUP_MSG \
> diff --git a/hw/xfree86/parser/InputClass.c b/hw/xfree86/parser/InputClass.c
> new file mode 100644
> index 0000000..1c98160
> --- /dev/null
> +++ b/hw/xfree86/parser/InputClass.c
> @@ -0,0 +1,232 @@
> +/*
> + * Copyright (c) 2009 Dan Nicholson
> + *
> + * Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/* View/edit this file with tab stops set to 4 */

isn't shiftwidth 4 in this file? tabstop doesn't matter since you don't use
tabs. (One reason to maybe take this line out ;)

> +
> +#ifdef HAVE_XORG_CONFIG_H
> +#include <xorg-config.h>
> +#endif
> +
> +#include "xf86Parser.h"
> +#include "xf86tokens.h"
> +#include "Configint.h"
> +
> +extern LexRec val;
> +
> +static
> +xf86ConfigSymTabRec InputClassTab[] =
> +{
> +    {ENDSECTION, "endsection"},
> +    {IDENTIFIER, "identifier"},
> +    {OPTION, "option"},
> +    {DRIVER, "driver"},
> +    {MATCH_PRODUCT, "matchproduct"},
> +    {MATCH_VENDOR, "matchvendor"},
> +    {MATCH_DEVICE_PATH, "matchdevicepath"},
> +    {MATCH_IS_KEYBOARD, "matchiskeyboard"},
> +    {MATCH_IS_POINTER, "matchispointer"},
> +    {MATCH_IS_JOYSTICK, "matchisjoystick"},
> +    {MATCH_IS_TABLET, "matchistablet"},
> +    {MATCH_IS_TOUCHPAD, "matchistouchpad"},
> +    {MATCH_IS_TOUCHSCREEN, "matchistouchscreen"},
> +    {-1, ""},
> +};
> +
> +#define CLEANUP xf86freeInputClassList

doesn't seem to be used anywhere

Cheers,
  Peter