Good morning Dirk.
2015-04-02 18:18 GMT+02:00 Dirk Hohndel <[email protected]>:
>
> > + 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.
_______________________________________________
subsurface mailing list
[email protected]
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
