[PATCH libevdev 3/5] Add libevdev_event_type/code_from_name() resolvers

[Peter Hutterer]

On Mon, Oct 07, 2013 at 01:22:32PM +0200, David Herrmann wrote:
> Hi
> 
> On Mon, Oct 7, 2013 at 3:17 AM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> > On Fri, Oct 04, 2013 at 08:37:33PM +0200, David Herrmann wrote:
> >> Two new helpers are added:
> >> (1) libevdev_event_type_from_name() takes a string describing an EV_*
> >> event type and returns the given event-type constant.
> >> (2) libevdev_event_code_from_name() takes a string describing an event
> >> code and returns the given event-code constant.
> >>
> >> Both functions share common behavior:
> >>  - Return -1 if the string is not found.
> >>  - Accept a string-length argument (if <0 zero-terminated string is
> >>    assumed). This is highly useful for lexers. If you mmap() a file it is
> >>    usually read-only and a lexer cannot assume an identifier is
> >>    zero-terminated. So without an explicit size-argument, a lexer would
> >>    have to copy the string before passing it to the resolver.
> >>  - Accept a flags argument. Two flags exist: One flag to make the resolver
> >>    case-sensitive (case-insensitive is the default). The other flag allows
> >
> > what's the use-case of having case-sensitive matching? these defines are always
> > uppercase and I doubt that'll change soon. the only reason for
> > case-sensitive matching I can see is if the caller doesn't quite know if
> > something is an event code or not and want's to decide based on the case
> > (e.g. "trigger" in a text isn't BTN_TRIGGER). and that's more likely to go
> > wrong than right anyway ("a" vs "A").
> 
> Yepp, that's the use-case I had in mind. Think of a
> configuration-option that allows to configure a button-mapping. To
> avoid the redundant BTN_ prefix, I support the prefix-less mode. But
> to allow users to also support additional options like
> "btn-xy-map=off" I want to avoid matching this against some imaginary
> BTN_OFF. Yes the application should catch this before passing it to
> libevdev, but what if it is supposed to be btn-xy-map=disabled and the
> user just wrote the wrong option..
> 
> On the other hand, all my code does is to do the case-insensitive
> search and then do the strcmp() afterwards. We could actually leave
> that to the caller.. I'm fine with both.

let's leave this out. it looks to me you're trying to solve a use-case that
must be handled by the application anyway and if the libevdev behaviour
is too flexible it'll be hard to debug and could potentially give the
call bad results. I suspect any meaningful caller will be a lot more
complicated than just feeding parsed text files, so the more predictable the
API, the better.
 
> >>    the caller to force the resolver to fail if the code prefix is omitted.
> >>    By default, the resolver allows omitting prefixes like "EV_", "KEY_"
> >>    and so on to simplify configuration files where this is obvious,
> >>    anyway. However, this might be ambigious or misleading in some
> >>    use-cases so this flag allows to disable the prefix-guessing.
> >>
> >> libevdev_event_code_from_name() has one additional argument which
> >> specifies the type/group of the code. If it is -1, the resolver tries to
> >> deduce it from the string prefix. Otherwise, only the given group is
> >> searched.
> >
> > I'm not a big fan of the API just yet, tbh. I'd set the DEFAULT flags enum
> > to set a specific value so that 0 is an invalid value.
> 
> You mean like this:
> 
> enum libevdev_name_flag {
>        /** Require the resolver to compare names case sensitive */
>        LIBEVDEV_NAME_FLAG_CASE_SENSITIVE           = 1,
>        /** Allow skipping the prefix */
>        LIBEVDEV_NAME_FLAG_GUESS_PREFIX           = 2,
>        /** Default behavior for name resolution (no flags passed). */
>        LIBEVDEV_NAME_FLAG_DEFAULT                  =
> LIBEVDEV_NAME_FLAG_GUESS_PREFIX,
> };
> 
> Note that "0" is not invalid. If you want "case-insensitive" but
> require a prefix, you would pass 0 now.

hmm. not if you define _all_ possible options. just thinking aloud here, what about:

enum libevdev_name_flag {
       LIBEVDEV_NAME_FLAG_DEFAULT                  = 1,
       /** Require the resolver to compare names case sensitive */
       LIBEVDEV_NAME_FLAG_CASE_SENSITIVE           = 2,
       /** Allow skipping the prefix */
       LIBEVDEV_NAME_FLAG_GUESS_PREFIX           = 4,


       /* default options for readability */
       LIBEVDEV_NAME_FLAG_CASE_INSENSITIVE = LIBEVDEV_NAME_FLAG_DEFAULT,
       LIBEVDEV_NAME_FLAG_WITH_PREFIX = LIBEVDEV_NAME_FLAG_DEFAULT,
};

still not quite happy with it, but I don't like allowing 0 where we should
have something readable.

[...]

> >> Another special behavior is BTN_* parsing. The BTN_* constants may overlap
> >> with KEY_* constants and thus, if the prefix is omitted, the parser cannot
> >> know which to return. We explicitly prefer KEY_* codes in that case so
> >> users have to add the prefix explicitly if they want the BTN_* code of a
> >> conflicting EV_KEY code.
> >
> > I don't think this behaviouri is a good idea, it's likely to produce the
> > wrong result when people don't check the exact behaviour and a lookup
> > failure for BTN_A is a lot easier to debug than a wrong code because
> > we returned KEY_A instead.
> >
> > better not to allow it and require users to use BTN_ as prefix for button
> > lookups.
> 
> So only lookup button names if the BTN_ prefix is passed? Hmmm, makes
> the whole prefix-less thing mostly useless. But yeah, I get your
> point. Will change it.

well, tbh I'm not a big fan of the prefix part anyway because I suspect that
it's easier to have libevdev be simple and stupid and just provide a
straight string to code mapping ("BTN_TRIGGER" → BTN_TRIGGER). and let the
caller worry about prefixes. I struggle to find a use-case where you'd have
a string that you need the code for but you don't know what type it is.

and iirc libevdev_event_type_from_name doesn't give you the type either, so
even if you have TRIGGER all you get is the code but you still don't know
what the type is, right?

again, some more detail on the use-case you have in mind would help to
figure out how much we actually need to do here.
 
[...]

> >> + */
> >> +int libevdev_event_code_from_name(const char *name, ssize_t len, int type,
> >> +                               enum libevdev_resolver_flag flags);
> >
> > all the *event_code* functions have the arguments in order type, code,
> > should be the same here.
> 
> So you're saying like this:
> 
> int libevdev_event_code_from_name(int type, const char *name, ssize_t len,
>                                enum libevdev_resolver_flag flags);

yep, that'll do.

Cheers,
   Peter