[PATCH 5/5] Clarify (and fix) dive trip auto-generation
Dirk Hohndel
dirk at hohndel.org
Mon Nov 26 09:42:19 PST 2012
Nice work. This series definitely makes the code much more readable.
You and I talked about a change to the "autogroup" semantic. To make
this a feature of a specific data file and not a global property. Is now
a good time to switch to that model?
/D
Linus Torvalds <torvalds at linux-foundation.org> writes:
> From: Linus Torvalds <torvalds at linux-foundation.org>
> Date: Sun, 25 Nov 2012 20:06:54 -0800
> Subject: [PATCH 5/5] Clarify (and fix) dive trip auto-generation
>
> This makes the dive trip auto-generation a separate pass from the
> showing of the dive trips, which makes things much more understandable.
> It simplifies the code a lot too, because it's much more natural to
> generate the automatic trip data by walking the dives from oldest to
> newest (while the tree model wants to walk the other way).
>
> It gets rid of the most annoying part of using the gtk tree model for
> dive trip management, but some still remains.
>
> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> ---
>
> This is the big one. Not only does it remove almost a hundred lines of
> code, it makes the remaining code much simpler too. And it gets rid of
> more gtk data structures - notably the use of the nasty GList crud, which
> is actually much more complicated than just doing a linked list by hand.
>
> And doing that linked list by hand means that we get the types right, and
> don't need the silly conversions from GList entries to dive trips etc.
>
> GList - just say no.
>
> But more importantly, it re-does the dive trip autogeneration code so that
> it's readable, and independent of filling the gtk tree models.
>
> There is more to be done, but this does do a big chunk of the inpenetrable
> trip handling code.
>
> dive.h | 7 +-
> divelist.c | 288 ++++++++++++++++++++++---------------------------------------
> gtk-gui.c | 22 +----
> 3 files changed, 109 insertions(+), 208 deletions(-)
>
> diff --git a/dive.h b/dive.h
> index 18a48c94edfa..b8614bcb314b 100644
> --- a/dive.h
> +++ b/dive.h
> @@ -280,8 +280,12 @@ typedef struct dive_trip {
> struct dive *dives;
> int nrdives;
> int expanded:1, selected:1;
> + struct dive_trip *next;
> } dive_trip_t;
>
> +/* List of dive trips (sorted by date) */
> +extern dive_trip_t *dive_trip_list;
> +
> struct dive {
> int number;
> tripflag_t tripflag;
> @@ -346,7 +350,6 @@ static inline int rel_mbar_to_depth(int mbar, struct dive *dive)
> * be able to edit a dive without unintended side effects */
> extern struct dive edit_dive;
>
> -extern GList *dive_trip_list;
> extern gboolean autogroup;
> /* random threashold: three days without diving -> new trip
> * this works very well for people who usually dive as part of a trip and don't
> @@ -356,8 +359,6 @@ extern gboolean autogroup;
> #define UNGROUPED_DIVE(_dive) ((_dive)->tripflag == NO_TRIP)
> #define DIVE_IN_TRIP(_dive) ((_dive)->tripflag == IN_TRIP || (_dive)->tripflag == ASSIGNED_TRIP)
> #define DIVE_NEEDS_TRIP(_dive) ((_dive)->tripflag == TF_NONE)
> -#define NEXT_TRIP(_entry) ((_entry) ? g_list_next(_entry) : (dive_trip_list))
> -#define PREV_TRIP(_entry) ((_entry) ? g_list_previous(_entry) : g_list_last(dive_trip_list))
> #define DIVE_TRIP(_trip) ((dive_trip_t *)(_trip)->data)
> #define DIVE_FITS_TRIP(_dive, _dive_trip) ((_dive_trip)->when - TRIP_THRESHOLD <= (_dive)->when)
>
> diff --git a/divelist.c b/divelist.c
> index 8f9b3de40548..af58b480671f 100644
> --- a/divelist.c
> +++ b/divelist.c
> @@ -40,7 +40,7 @@ static struct DiveList dive_list;
> #define TREESTORE(_dl) GTK_TREE_STORE((_dl).treemodel)
> #define LISTSTORE(_dl) GTK_TREE_STORE((_dl).listmodel)
>
> -GList *dive_trip_list;
> +dive_trip_t *dive_trip_list;
> gboolean autogroup = FALSE;
>
> /* this duplicate assignment of "INTRIP" causes the save_xml code
> @@ -935,38 +935,39 @@ void update_dive_list_col_visibility(void)
> #ifdef DEBUG_TRIP
> static void dump_trip_list(void)
> {
> - GList *p = NULL;
> + dive_trip_t *trip;
> int i=0;
> timestamp_t last_time = 0;
> - while ((p = NEXT_TRIP(p))) {
> - dive_trip_t *dive_trip = DIVE_TRIP(p);
> +
> + for (trip = dive_trip_list; trip; trip = trip->next) {
> struct tm tm;
> - utc_mkdate(dive_trip->when, &tm);
> - if (dive_trip->when < last_time)
> + utc_mkdate(trip->when, &tm);
> + if (trip->when < last_time)
> printf("\n\ndive_trip_list OUT OF ORDER!!!\n\n\n");
> printf("%s trip %d to \"%s\" on %04u-%02u-%02u %02u:%02u:%02u (%d dives - %p)\n",
> - dive_trip->tripflag == AUTOGEN_TRIP ? "autogen " : "",
> - ++i, dive_trip->location,
> + trip->tripflag == AUTOGEN_TRIP ? "autogen " : "",
> + ++i, trip->location,
> tm.tm_year + 1900, tm.tm_mon+1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec,
> - dive_trip->nrdives, dive_trip);
> - last_time = dive_trip->when;
> + trip->nrdives, trip);
> + last_time = trip->when;
> }
> printf("-----\n");
> }
> #endif
>
> /* this finds a trip that starts at precisely the time given */
> -static GList *find_trip_by_time(timestamp_t when)
> +static dive_trip_t *find_trip_by_time(timestamp_t when)
> {
> - GList *trip = dive_trip_list;
> - while (trip && DIVE_TRIP(trip)->when < when)
> + dive_trip_t *trip = dive_trip_list;
> +
> + while (trip && trip->when < when)
> trip = trip->next;
> - if (DIVE_TRIP(trip)->when == when) {
> + if (trip->when == when) {
> #ifdef DEBUG_TRIP
> struct tm tm;
> - utc_mkdate(DIVE_TRIP(trip)->when, &tm);
> + utc_mkdate(trip->when, &tm);
> printf("found trip %p @ %04d-%02d-%02d %02d:%02d:%02d\n",
> - DIVE_TRIP(trip),
> + trip,
> tm.tm_year+1900, tm.tm_mon+1, tm.tm_mday,
> tm.tm_hour, tm.tm_min, tm.tm_sec);
> #endif
> @@ -978,36 +979,25 @@ static GList *find_trip_by_time(timestamp_t when)
> return NULL;
> }
>
> -/* this finds a trip that starts at precisely the time given */
> -static GList *find_trip(dive_trip_t *target)
> -{
> - GList *trip = dive_trip_list;
> - while (trip) {
> - if (DIVE_TRIP(trip) == target)
> - return trip;
> - trip = trip->next;
> - }
> - return NULL;
> -}
> -
> /* this finds the last trip that at or before the time given */
> -static GList *find_matching_trip(timestamp_t when)
> +static dive_trip_t *find_matching_trip(timestamp_t when)
> {
> - GList *trip = dive_trip_list;
> - if (!trip || DIVE_TRIP(trip)->when > when) {
> + dive_trip_t *trip = dive_trip_list;
> +
> + if (!trip || trip->when > when) {
> #ifdef DEBUG_TRIP
> printf("no matching trip\n");
> #endif
> return NULL;
> }
> - while (trip->next && DIVE_TRIP(trip->next)->when <= when)
> + while (trip->next && trip->next->when <= when)
> trip = trip->next;
> #ifdef DEBUG_TRIP
> {
> struct tm tm;
> - utc_mkdate(DIVE_TRIP(trip)->when, &tm);
> + utc_mkdate(trip->when, &tm);
> printf("found trip %p @ %04d-%02d-%02d %02d:%02d:%02d\n",
> - DIVE_TRIP(trip),
> + trip,
> tm.tm_year+1900, tm.tm_mon+1, tm.tm_mday,
> tm.tm_hour, tm.tm_min, tm.tm_sec);
> }
> @@ -1021,40 +1011,43 @@ static GList *find_matching_trip(timestamp_t when)
> void insert_trip(dive_trip_t **dive_trip_p)
> {
> dive_trip_t *dive_trip = *dive_trip_p;
> - GList *trip = dive_trip_list;
> - while (trip && DIVE_TRIP(trip)->when < dive_trip->when)
> - trip = trip->next;
> - if (trip && DIVE_TRIP(trip)->when == dive_trip->when) {
> - if (! DIVE_TRIP(trip)->location)
> - DIVE_TRIP(trip)->location = dive_trip->location;
> - *dive_trip_p = DIVE_TRIP(trip);
> + dive_trip_t **p = &dive_trip_list;
> + dive_trip_t *trip;
> +
> + /* Walk the dive trip list looking for the right location.. */
> + while ((trip = *p) != NULL && trip->when < dive_trip->when)
> + p = &trip->next;
> +
> + if (trip && trip->when == dive_trip->when) {
> + if (! trip->location)
> + trip->location = dive_trip->location;
> + *dive_trip_p = trip;
> } else {
> - dive_trip_list = g_list_insert_before(dive_trip_list, trip, *dive_trip_p);
> + dive_trip->next = trip;
> + *p = dive_trip;
> }
> #ifdef DEBUG_TRIP
> dump_trip_list();
> #endif
> }
>
> -static inline void delete_trip_list_entry(GList *trip)
> -{
> - dive_trip_list = g_list_delete_link(dive_trip_list, trip);
> -#ifdef DEBUG_TRIP
> - dump_trip_list();
> -#endif
> -}
> -
> static void delete_trip(dive_trip_t *trip)
> {
> - GList *trip_list = find_trip(trip);
> + dive_trip_t **p, *tmp;
>
> assert(!trip->dives);
> - /*
> - * The trip may not be on the list, if it had the
> - * same time as another trip.
> - */
> - if (trip_list)
> - delete_trip_list_entry(trip_list);
> +
> + /* Remove the trip from the list of trips */
> + p = &dive_trip_list;
> + while ((tmp = *p) != NULL) {
> + if (tmp == trip) {
> + *p = trip->next;
> + break;
> + }
> + p = &tmp->next;
> + }
> +
> + /* .. and free it */
> if (trip->location)
> free(trip->location);
> free(trip);
> @@ -1104,6 +1097,7 @@ void add_dive_to_trip(struct dive *dive, dive_trip_t *trip)
> remove_dive_from_trip(dive);
> trip->nrdives++;
> dive->divetrip = trip;
> + dive->tripflag = ASSIGNED_TRIP;
>
> /* Add it to the trip's list of dives*/
> dive->next = trip->dives;
> @@ -1129,47 +1123,45 @@ static dive_trip_t *create_and_hookup_trip_from_dive(struct dive *dive)
> return dive_trip;
> }
>
> -/* check that a dive should be in a trip starting at 'when'
> - * first the regular check (dive is before the trip start, but within the
> - * threshold)
> - * then for dives that are after the trip start we walk back to the dive
> - * that starts at when and check on the way that there is no ungrouped
> - * dive and no break beyond the 3 day threshold between dives that
> - * haven't already been assigned to this trip */
> -static gboolean dive_can_be_in_trip(int idx, dive_trip_t *dive_trip)
> +/*
> + * Walk the dives from the oldest dive, and see if we can autogroup them
> + */
> +static void autogroup_dives(void)
> {
> - struct dive *dive, *pdive;
> - int i = idx;
> - timestamp_t when = dive_trip->when;
> + int i;
> + struct dive *dive, *lastdive = NULL;
>
> - dive = get_dive(idx);
> - /* if the dive is before the trip start but within the threshold
> - * then just accept it, otherwise reject it */
> - if (dive->when < when) {
> - if (DIVE_FITS_TRIP(dive, dive_trip))
> - return TRUE;
> - else
> - return FALSE;
> - }
> + for_each_dive(i, dive) {
> + dive_trip_t *trip;
>
> - while (--i >= 0) {
> - pdive = get_dive(i);
> - /* an ungrouped dive cannot be in the middle of a trip
> - * also, if there are two consecutive dives that are too far apart
> - * that aren't both already labeled as 'in trip' (which shows that
> - * this was manually done so we shouldn't override that) */
> - if ( UNGROUPED_DIVE(pdive) ||
> - (! (DIVE_IN_TRIP(pdive) && DIVE_IN_TRIP(dive)) &&
> - dive->when - pdive->when > TRIP_THRESHOLD)) {
> - return FALSE;
> + if (dive->divetrip) {
> + lastdive = dive;
> + continue;
> }
> - if (pdive->when == when)
> - /* done - we have reached the first dive in the trip */
> - return TRUE;
> - dive = pdive;
> +
> + if (!DIVE_NEEDS_TRIP(dive)) {
> + lastdive = NULL;
> + continue;
> + }
> +
> + /* Do we have a trip we can combine this into? */
> + if (lastdive && dive->when < lastdive->when + TRIP_THRESHOLD) {
> + dive_trip_t *trip = lastdive->divetrip;
> + add_dive_to_trip(dive, trip);
> + if (dive->location && !trip->location)
> + trip->location = strdup(dive->location);
> + lastdive = dive;
> + continue;
> + }
> +
> + lastdive = dive;
> + trip = create_and_hookup_trip_from_dive(dive);
> + trip->tripflag = AUTOGEN_TRIP;
> }
> - /* we should never get here */
> - return TRUE;
> +
> +#ifdef DEBUG_TRIP
> + dump_trip_list();
> +#endif
> }
>
> static void fill_dive_list(void)
> @@ -1178,11 +1170,10 @@ static void fill_dive_list(void)
> GtkTreeIter iter, parent_iter, *parent_ptr = NULL;
> GtkTreeStore *liststore, *treestore;
> dive_trip_t *last_trip = NULL;
> - GList *trip;
> - dive_trip_t *dive_trip = NULL;
>
> - /* if we have pre-existing trips, start on the last one */
> - trip = g_list_last(dive_trip_list);
> + /* Do we need to create any dive groups automatically? */
> + if (autogroup)
> + autogroup_dives();
>
> treestore = TREESTORE(dive_list);
> liststore = LISTSTORE(dive_list);
> @@ -1190,85 +1181,23 @@ static void fill_dive_list(void)
> i = dive_table.nr;
> while (--i >= 0) {
> struct dive *dive = get_dive(i);
> + dive_trip_t *trip = dive->divetrip;
>
> - /* make sure we display the first date of the trip in previous summary */
> - if (dive_trip && parent_ptr) {
> - gtk_tree_store_set(treestore, parent_ptr,
> - DIVE_DATE, dive_trip->when,
> - DIVE_LOCATION, dive_trip->location,
> - -1);
> - }
> - /* the dive_trip info might have been killed by a previous UNGROUPED dive */
> - if (trip)
> - dive_trip = DIVE_TRIP(trip);
> - /* tripflag defines how dives are handled;
> - * TF_NONE "not handled yet" - create time based group if autogroup == TRUE
> - * NO_TRIP "set as no group" - simply leave at top level
> - * IN_TRIP "use the trip with the largest trip time (when) that is <= this dive"
> - */
> - if (UNGROUPED_DIVE(dive)) {
> - /* first dives that go to the top level */
> - parent_ptr = NULL;
> - dive_trip = NULL;
> - } else if (autogroup && DIVE_NEEDS_TRIP(dive)){
> - /* if we already have existing trips there are up to two trips that this
> - * could potentially be part of.
> - * Let's see if there is a matching one already */
> - GList *matching_trip;
> - matching_trip = find_matching_trip(dive->when);
> - if (matching_trip && dive_can_be_in_trip(i, DIVE_TRIP(matching_trip))) {
> - trip = matching_trip;
> - } else {
> - /* is there a trip we can extend ? */
> - if (! matching_trip && dive_trip) {
> - /* careful - this is before the first trip
> - yet we have a trip we're looking at; make
> - sure that is indeed the first trip */
> - dive_trip = DIVE_TRIP(dive_trip_list);
> - trip = dive_trip_list;
> - }
> - /* maybe we can extend the current trip */
> - if (! (dive_trip && dive_can_be_in_trip(i, dive_trip))) {
> - /* seems like neither of these trips work, so create
> - * a new one; all fields default to 0 and get filled
> - * in further down */
> - parent_ptr = NULL;
> - dive_trip = create_and_hookup_trip_from_dive(dive);
> - dive_trip->tripflag = AUTOGEN_TRIP;
> - trip = find_trip(dive_trip);
> - }
> - }
> - if (trip)
> - dive_trip = DIVE_TRIP(trip);
> - } else if (DIVE_IN_TRIP(dive)) {
> - trip = find_matching_trip(dive->when);
> - if (trip)
> - dive_trip = DIVE_TRIP(trip);
> - } else {
> - /* dive is not in a trip and we aren't autogrouping */
> - dive_trip = NULL;
> - parent_ptr = NULL;
> - }
> - /* update dive as part of dive_trip and
> - * (if necessary) update dive_trip time and location */
> - if (dive_trip) {
> - if(DIVE_NEEDS_TRIP(dive))
> - dive->tripflag = ASSIGNED_TRIP;
> - add_dive_to_trip(dive, dive_trip);
> - if (!dive_trip->location && dive->location)
> - dive_trip->location = strdup(dive->location);
> - if (dive_trip != last_trip) {
> - last_trip = dive_trip;
> + if (trip != last_trip) {
> + last_trip = trip;
> + if (trip) {
> /* create trip entry */
> gtk_tree_store_append(treestore, &parent_iter, NULL);
> parent_ptr = &parent_iter;
> /* a duration of 0 (and negative index) identifies a group */
> gtk_tree_store_set(treestore, parent_ptr,
> DIVE_INDEX, -1,
> - DIVE_DATE, dive_trip->when,
> - DIVE_LOCATION, dive_trip->location,
> + DIVE_DATE, trip->when,
> + DIVE_LOCATION, trip->location,
> DIVE_DURATION, 0,
> -1);
> + } else {
> + parent_ptr = NULL;
> }
> }
>
> @@ -1300,17 +1229,8 @@ static void fill_dive_list(void)
> DIVE_SUIT, dive->suit,
> DIVE_SAC, 0,
> -1);
> -#ifdef DEBUG_TRIP
> - dump_trip_list();
> -#endif
> }
>
> - /* make sure we display the first date of the trip in previous summary */
> - if (parent_ptr && dive_trip)
> - gtk_tree_store_set(treestore, parent_ptr,
> - DIVE_DATE, dive_trip->when,
> - DIVE_LOCATION, dive_trip->location,
> - -1);
> update_dive_list_units();
> if (amount_selected == 0 && gtk_tree_model_get_iter_first(MODEL(dive_list), &iter)) {
> GtkTreeSelection *selection;
> @@ -1441,12 +1361,10 @@ void edit_trip_cb(GtkWidget *menuitem, GtkTreePath *path)
> GtkTreeIter iter;
> timestamp_t when;
> dive_trip_t *dive_trip;
> - GList *trip;
>
> gtk_tree_model_get_iter(MODEL(dive_list), &iter, path);
> gtk_tree_model_get(MODEL(dive_list), &iter, DIVE_DATE, &when, -1);
> - trip = find_trip_by_time(when);
> - dive_trip = DIVE_TRIP(trip);
> + dive_trip = find_trip_by_time(when);
> if (edit_trip(dive_trip))
> gtk_tree_store_set(STORE(dive_list), &iter, DIVE_LOCATION, dive_trip->location, -1);
> }
> @@ -1563,7 +1481,7 @@ static GtkTreeIter *move_dive_between_trips(GtkTreeIter *dive_iter, GtkTreeIter
> gtk_tree_store_remove(STORE(dive_list), dive_iter);
> if (old_trip) {
> gtk_tree_model_get(MODEL(dive_list), old_trip, DIVE_DATE, &old_when, -1);
> - old_divetrip = DIVE_TRIP(find_matching_trip(old_when));
> + old_divetrip = find_matching_trip(old_when);
> update_trip_timestamp(old_trip, old_divetrip);
> }
> if (new_trip) {
> @@ -1902,7 +1820,7 @@ void merge_trips_cb(GtkWidget *menuitem, GtkTreePath *trippath)
> GtkTreePath *prevpath;
> GtkTreeIter thistripiter, prevtripiter, newiter, iter;
> GtkTreeModel *tm = MODEL(dive_list);
> - GList *prevtrip;
> + dive_trip_t *prevtrip;
> timestamp_t when;
>
> /* this only gets called when we are on a trip and there is another trip right before */
> @@ -1918,7 +1836,7 @@ void merge_trips_cb(GtkWidget *menuitem, GtkTreePath *trippath)
> gtk_tree_store_insert_before(STORE(dive_list), &newiter, &prevtripiter, NULL);
> idx = copy_tree_node(&iter, &newiter);
> gtk_tree_store_remove(STORE(dive_list), &iter);
> - add_dive_to_trip(get_dive(idx), DIVE_TRIP(prevtrip));
> + add_dive_to_trip(get_dive(idx), prevtrip);
> }
> gtk_tree_store_remove(STORE(dive_list), &thistripiter);
> mark_divelist_changed(TRUE);
> @@ -1972,7 +1890,7 @@ void remember_tree_state()
> continue;
> path = gtk_tree_model_get_path(TREEMODEL(dive_list), &iter);
> if (gtk_tree_view_row_expanded(GTK_TREE_VIEW(dive_list.tree_view), path))
> - DIVE_TRIP(find_trip_by_time(when))->expanded = TRUE;
> + find_trip_by_time(when)->expanded = TRUE;
> } while (gtk_tree_model_iter_next(TREEMODEL(dive_list), &iter));
> }
>
> @@ -1986,9 +1904,9 @@ static gboolean restore_node_state(GtkTreeModel *model, GtkTreePath *path, GtkTr
>
> gtk_tree_model_get(model, iter, DIVE_INDEX, &idx, DIVE_DATE, &when, -1);
> if (idx < 0) {
> - if (DIVE_TRIP(find_trip_by_time(when))->expanded)
> + if (find_trip_by_time(when)->expanded)
> gtk_tree_view_expand_row(tree_view, path, FALSE);
> - if (DIVE_TRIP(find_trip_by_time(when))->selected)
> + if (find_trip_by_time(when)->selected)
> gtk_tree_selection_select_iter(selection, iter);
> } else {
> dive = get_dive(idx);
> diff --git a/gtk-gui.c b/gtk-gui.c
> index dca239bf2fff..dc9620914402 100644
> --- a/gtk-gui.c
> +++ b/gtk-gui.c
> @@ -262,18 +262,8 @@ static gboolean ask_save_changes()
> return quit;
> }
>
> -static void dive_trip_unref(gpointer data, gpointer user_data)
> -{
> - dive_trip_t *dive_trip = (dive_trip_t *)data;
> - if (dive_trip->location)
> - free(dive_trip->location);
> - free(data);
> -}
> -
> static void file_close(GtkWidget *w, gpointer data)
> {
> - int i;
> -
> if (unsaved_changes())
> if (ask_save_changes() == FALSE)
> return;
> @@ -283,20 +273,12 @@ static void file_close(GtkWidget *w, gpointer data)
> existing_filename = NULL;
>
> /* free the dives and trips */
> - for (i = 0; i < dive_table.nr; i++)
> - free(get_dive(i));
> - dive_table.nr = 0;
> + while (dive_table.nr)
> + delete_single_dive(0);
> dive_table.preexisting = 0;
> mark_divelist_changed(FALSE);
>
> - /* inlined version of g_list_free_full(dive_trip_list, free); */
> - g_list_foreach(dive_trip_list, (GFunc)dive_trip_unref, NULL);
> - g_list_free(dive_trip_list);
> -
> - dive_trip_list = NULL;
> -
> /* clear the selection and the statistics */
> - amount_selected = 0;
> selected_dive = 0;
> process_selected_dives();
> clear_stats_widgets();
> --
> 1.8.0.dirty
>
--
Dirk Hohndel
Intel Open Source Technology Center
More information about the subsurface
mailing list