API redesign progress

[Linus Torvalds]

On Sat, Jun 23, 2012 at 1:45 PM, Jef Driesen <jefdriesen at telenet.be> wrote:
>
> Creating the parser in the callback is convenient. In some cases you can't
> create a parser upfront (at the time of calling dc_device_open, or even
> immediately after it) because the actual model number is required for
> parsing the data.

Jef, this is exactly the kind of totally broken thinking that makes
libdivecomputer interfaces so nasty to use.

The user SHOULD NOT CARE about these kinds of internal libdivecomputer
issues. The whole *point* of libdivecomputer should be to hide all
these internal implementation issues, and as long as you continue to
think that your internal details should matter for the interface, the
interface will be crap.

Why the hell should I care that you need some model number for the
parser? I do not even *KNOW* the model number, for chissake! The only
thing that knows (and cares) is libdivecomputer itself.

So according to your absolutely insane logic, I cannot create the
parser at device open time, because I don't know if that particular
device needs the model number. But that means that I should magically
decide to create the dive parser after I get the DC_EVENT_DEVINFO
callback. Can you not see that THAT MAKES NO SENSE! From an interface
standpoint, that is absolutely insane. I don't even *care* about the
DC_EVENT_DEVINFO - the fact that I actually catch it and print out the
model number on the progress bar is simply because that (to me) is an
indicator that libdivecomputer is working at all.

You fixed the interfaces to the top-level open code, and to the
top-level parser create code. Please just fix this too.

The fix is damn easy: get rid of the insane and useless
"dc_parser_new()" and "dc_parser_destroy()" interfaces entirely.
Create the parser inside of libdivecomputer (whenever it makes sense)
and don't expose that totally idiotic and pointless interface to the
user at all. Seriously. There is absolutely *ZERO* upside to ask me to
create a parser, since I don't care, and I cannot possibly do it at a
sane time anyway, because for the user, no sane time exists at all.

Just create the parser internally, and hide that information in the
"dc_device_t" structure that is totally internal to libdivecomputer,
and is just an opaque object to the users.

Please. Stop these insane interfaces that only complicate the use of
libdivecomputer, and don't help anybody.


> You also have to take into account the use-case where you want to parse data
> without a device connection available.

No you don't. You can expose damn well any interface you want for that
case, but that's a totally separate case. That in *no* way means that
when we have the device available, the user should somehow magically
know when to create the parser. There is zero point.

> BTW, for this situation I want to introduce some variant of the new
> dc_parser_new function that doesn't take a device handle. I have been
> thinking of something like this:
>
> dc_status_t
> dc_parser_new_for_data (dc_parser_t **parser,
>                        const dc_event_devinfo_t *devinfo,
>                        const dc_event_clock_t *clock);

Congratulations on making the interface EVEN MORE BROKEN.

Don't do this. Seriously. Just f*cking don't. Any sane user of the
libdivecomputer stuff will never ever care. If I opened a device
through libdivecomputer, you already have ALL that information. It's
totally pointless to give it to be as a callback, and then expect me
to just give it back to you. Can't you see how STUPID that is?

Make that interface what you do *internally*. Use it for the pointless
case where somebody cares. Don't force it for the normal case.

Even if I were to want to parse a pre-downloaded dump from a file, I
WOULD NEVER EVER WANT TO USE THAT INTERFACE. I would want to use the
"open dive computer" interface, and just give you the filename. And
then libdivecomputer can again internally do the above.

So feel free to do those kinds of changes for your internal flow, but
if you really think that it's a good idea to expose that kind of
internal detail to the user, I don't know what to say. Except to say
that you're wrong. It's a really *bad* idea to make these kinds of
interfaces. Don't make things worse, make them better.

>>  - The event handling is still horrible. There does not seem to be any
>> sane generic way to just get the event type and meaning.
>
> Which events are you referring to? The device events (e.g. DC_EVENT_*), or
> the parser events (e.g. DC_SAMPLE_EVENT)? Can you explain in some more
> detail?

The DC_SAMPLE_EVENT interface is just crazy.

Why do I have to have this kind of code in there:


        static const char *events[] = {
                "none", "deco", "rbt", "ascent", "ceiling",
"workload", "transmitter",
                "violation", "bookmark", "surface", "safety stop", "gaschange",
                "safety stop (voluntary)", "safety stop (mandatory)",
"deepstop",
                "ceiling (safety stop)", "unknown", "divetime", "maxdepth",
                "OLF", "PO2", "airtime", "rgbm", "heading", "tissue
level warning"
        };
        const int nr_events = sizeof(events) / sizeof(const char *);
        ...
        type = value.event.type;
        name = "invalid event number";
        if (type < nr_events)
                name = events[type];

where that set of strings is something I had to just mindlessly copy
from your "example.c".

The 'enum parser_sample_event_t' numbers are totally meaningless to
me. They don't tell me anything. Same goes for 'enum
parser_sample_flags_t'. And what does the even 'value' actually do?
None of this makes any sense, and more importantly, as far as I can
tell, I *cannot* make sense of them.

Yes, the 'flags' things seems to possibly make sense as a "this begins
the event" and "this ends it". That's not documented, but at least
it's *somehow* sensible. But if that is all it does, why is it some
bitfield thing? What else can it possibly be? What should I do as a
user to protect myself from you changing the flags?

And the 'value' field? What does it do? It makes no sense to me, and I
can't tell what it would do. Does it have any meaning? Is it in
degrees for the heading event? Is it minutes for the airtime event?
What is it?

                     Linus