I'm very uncomfortable with such a drastic change this late in the game. What we have may be broken, but it seems to work for a lot of people and has received some amount of testing...
Can you re-submit this after 4.2? I'm sure a thorough analysis and a bunch of testing could sway me to include this, but frankly I won't have time for that in the next couple of days ;-( /D On Mon, Jul 14, 2014 at 10:11:47AM +0200, Salvador Cuñat wrote: > We were actually searching dives which match the dowloaded position > fixes. So we're also trying to take into account if the fix is automatic > or no based on a limited amount of predefined strings (bad idea, as the > user can change in companion app settings the predefined string). > This way, in actual implementation, if program concludes that a fix has > been manually got or, simply, the user is unlucky enough to have all the > position fixes out of the dive time, find_dive_n_near() function will pair > fix and dive in an ordered way (1st fix -> 1st dive; 2nd fix -> 2nd dive ...) > which is probably erroneous, except for manual position fixes. > BTW actual implementation is unable to pair the same gps fix with more than > one dive (which is most likely to happen). > > The patch changes the logic: > > - Search positions for defined dives (instead of dives for defined positions) > without care if position has manually or automatically been set. > - It makes two assumptions: > a.- If the position fix has been taken during the dive time, is correct. > b.- If not during diving time, the correct one is the nearest fix > before the dive begins (also the usual case if manually fixed from the > smartphone just before jump into the water). But will work too if there > is only one fix *in range* after the dive (another usual case). > - Finally, as copy_gps_location() in dive.h is used only here, let it take > care of naming the dive if user hasn't named it yet. > > Reported-by: Marc Merlin <[email protected]> > Signed-off-by: Salvador Cuñat <[email protected]> > --- > dive.h | 4 ++ > qt-ui/subsurfacewebservices.cpp | 93 > ++++++++++++++++----------------------- > 2 files changed, 43 insertions(+), 54 deletions(-) > > diff --git a/dive.h b/dive.h > index 09a4dda..bb22a3f 100644 > --- a/dive.h > +++ b/dive.h > @@ -322,7 +322,11 @@ static inline void copy_gps_location(struct dive *from, > struct dive *to) > if (from && to) { > to->latitude.udeg = from->latitude.udeg; > to->longitude.udeg = from->longitude.udeg; > + if (!to->location) { > + to->location = strdup(from->location); > + } > } > + > } > > static inline int get_surface_pressure_in_mbar(const struct dive *dive, bool > non_null) > diff --git a/qt-ui/subsurfacewebservices.cpp b/qt-ui/subsurfacewebservices.cpp > index 27a56ec..fe1a5cb 100644 > --- a/qt-ui/subsurfacewebservices.cpp > +++ b/qt-ui/subsurfacewebservices.cpp > @@ -31,69 +31,54 @@ > struct dive_table gps_location_table; > static bool merge_locations_into_dives(void); > > -static bool is_automatic_fix(struct dive *gpsfix) > -{ > - if (gpsfix && gpsfix->location && > - (!strcmp(gpsfix->location, "automatic fix") || > - !strcmp(gpsfix->location, "Auto-created dive"))) > - return true; > - return false; > -} > - > #define SAME_GROUP 6 * 3600 // six hours > //TODO: C Code. static functions are not good if we plan to have a test for > them. > static bool merge_locations_into_dives(void) > { > - int i, nr = 0, changed = 0; > - struct dive *gpsfix, *last_named_fix = NULL, *dive; > - > - sort_table(&gps_location_table); > - > - for_each_gps_location (i, gpsfix) { > - if (is_automatic_fix(gpsfix) && (dive = > find_dive_including(gpsfix->when))) { > - if (dive && !dive_has_gps_location(dive)) { > -#if DEBUG_WEBSERVICE > - struct tm tm; > - utc_mkdate(gpsfix->when, &tm); > - printf("found dive named %s @ %04d-%02d-%02d > %02d:%02d:%02d\n", > - gpsfix->location, > - tm.tm_year + 1900, tm.tm_mon + 1, > tm.tm_mday, > - tm.tm_hour, tm.tm_min, tm.tm_sec); > -#endif > - changed++; > - copy_gps_location(gpsfix, dive); > - } > - } else { > - if (last_named_fix && > dive_within_time_range(last_named_fix, gpsfix->when, SAME_GROUP)) { > - nr++; > - } else { > - nr = 1; > - last_named_fix = gpsfix; > - } > - dive = find_dive_n_near(gpsfix->when, nr, SAME_GROUP); > - if (dive) { > - if (!dive_has_gps_location(dive)) { > - copy_gps_location(gpsfix, dive); > - changed++; > - } > - if (!dive->location) { > - dive->location = > strdup(gpsfix->location); > - changed++; > + int i, j; > + struct dive *gpsfix, *nextgpsfix, *dive; > + > + sort_table(&dive_table); > + > + for_each_dive (i, dive) { > + if (!dive_has_gps_location(dive)) { > + sort_table(&gps_location_table); > + for_each_gps_location (j, gpsfix) { > + if (dive_within_time_range (dive, gpsfix->when, > SAME_GROUP)) { > + /* > + * If position is fixed during dive. > This is the good one. > + * Asign and end for_each_gps loop > + */ > + if ((dive->when <= gpsfix->when && > gpsfix->when <= dive->when + dive->duration.seconds)) { > + if > (!dive_has_gps_location(dive)) { > + > copy_gps_location(gpsfix,dive); > + break; > + } > + } else { > + /* > + * If it is not, check if there > are more position fixes. > + * If none this is the last (or > the only one) and so the good one. > + * If there is at least one but > later than end of dive, the actual fix > + * is OK, asign it and end > for_each_gps loop. > + * If next fix is closer or > into the dive, it will be evaluated in next > + * loop iteration > + */ > + if (nextgpsfix = > get_gps_location(j+1,&gps_location_table)) { > + if ((dive->when + > dive->duration.seconds - gpsfix->when) < (nextgpsfix->when - gpsfix->when)) { > + > copy_gps_location(gpsfix,dive); > + break; > + } > + } else { > + > copy_gps_location(gpsfix,dive); > + break; > + } > + } > } > - } else { > - struct tm tm; > - utc_mkdate(gpsfix->when, &tm); > -#if DEBUG_WEBSERVICE > - printf("didn't find dive matching gps fix named > %s @ %04d-%02d-%02d %02d:%02d:%02d\n", > - gpsfix->location, > - tm.tm_year + 1900, tm.tm_mon + 1, > tm.tm_mday, > - tm.tm_hour, tm.tm_min, tm.tm_sec); > -#endif > } > } > } > - return changed > 0; > } > + > //TODO: C-code. > static void clear_table(struct dive_table *table) > { > -- > 1.7.10.4 > _______________________________________________ subsurface mailing list [email protected] http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
