OSTCTools import support

Salvador Cuñat salvador.cunat at gmail.com
Fri Apr 3 00:18:30 PDT 2015


Good morning Dirk.

2015-04-02 18:18 GMT+02:00 Dirk Hohndel <dirk at hohndel.org>:

>
> > +     dc_datetime_t datetime = { 0 };
>
> so you renamed dt to datetime - nothing wrong with that, but it makes the
> patch more verbose and harder to see what's new, what's just renaming...
>
> > -     rc = dc_parser_get_datetime(parser, &dt);
> > +     rc = dc_parser_get_datetime(parser, &datetime);
>
> like this.
>
>
It wasn't my finest hour, sure.  I'll put it back to dt.


> >       if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED) {
> >               dev_info(devdata, translate("gettextFromC", "Error parsing
> the datetime"));
> > -             goto error_exit;
> > +             fprintf(stderr, "Error parsing the datetime");
> > +             return rc;
>
> fprintf to stderr is reasonable for debugging or for fatal errors,
> otherwise it's not ideal. Are these errors that should be reported to the
> user? Or really just debugging messages?
>

Just debugging messages, the returned code is managed and shown to the user
in the error bar on mainwindow.
I'll get rid of all of them.


> > -             dive->dc.duration.seconds = divetime;
> > +             dive->duration.seconds = dive->dc.duration.seconds =
> divetime;
>
> Why? fixup_dive() should be called on any imported dive at some point and
> that calls fixup_duration() which handles this consistently.
>

Sorry, this is a remanent from the work in smartrak files, which, for some
reason are not correctly managed.


>
> > @@ -537,7 +519,7 @@ static int dive_cb(const unsigned char *data,
> unsigned int size,
> >       for (idx = 0; idx < 100; idx++) {
> >               dc_field_string_t str = { NULL };
> >               rc = dc_parser_get_field(parser, DC_FIELD_STRING, idx,
> &str);
> > -             if (rc != DC_STATUS_SUCCESS)
> > +             if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED)
>
> Can you explain this change?
>

Ufff.  This is a bold failure on my side.  Too much copy paste here.
Didn't even realized it, as OSTC fully supports DC_FIELD_STRING.

>
> > @@ -558,21 +541,67 @@ static int dive_cb(const unsigned char *data,
> unsigned int size,
> >                       dive->dc.divemode = FREEDIVE;
> >                       break;
> >               case DC_DIVEMODE_GAUGE:
> > -             case DC_DIVEMODE_OC: /* Open circuit */
> > +             case DC_DIVEMODE_OC:
> >                       dive->dc.divemode = OC;
> >                       break;
> > -             case DC_DIVEMODE_CC:  /* Closed circuit */
> > +             case DC_DIVEMODE_CC:
>
> And why would you remove those comments?
>
> Absolutely unintended, the general idea was just splitting the initial
callback func in two with the minimal changes.


> >  #endif
> >
> > -     rc = parse_gasmixes(devdata, dive, parser, ngases, data);
> > -     if (rc != DC_STATUS_SUCCESS) {
> > +     rc = parse_gasmixes(devdata, dive, parser, ngases, NULL);
> > +     if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED) {
>
> Maybe I got lost reading the patch - but why are you passing NULL instead
> of data?
>

The data pointer is passed to the func but not used. It would  even  be
safely removed. Just didn't it as I wasn't sure if it was intended to be
used in the future.  I'll add a patch to remove it.


>
> >               dev_info(devdata, translate("gettextFromC", "Error parsing
> the gas mix"));
> > +             fprintf(stderr, "Error parsing the gas mix\n");
>
> Umm - so there is the correct error reporting to the user, and you add an
> fprintf to stderr. Not good.
>
>
> Here's what I'd like to see. Either don't do the datetime rename or if you
> do it, have it in its own patch with nothing else mixed in.
> Then, if you want to change all the error returns to show these errors to
> the user, do that in one patch and nothing else.
> As a third patch you can do the split out into a separate function,
> explaining any logic changes that you are doing.
>

OK.  I'll rework the patches removing the debug messages , the datetime
change and all other issues.

Thanks Dirk.

More comments in the other mail.

Regards.

Salva.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150403/80e88e77/attachment.html>


More information about the subsurface mailing list