[PATCH 2/2] Fixing up the code as Dirk suggested
Miika Turkia
miika.turkia at gmail.com
Mon Sep 10 21:59:34 PDT 2012
On Tue, Sep 11, 2012 at 7:31 AM, Dirk Hohndel <dirk at hohndel.org> wrote:
>
> Miika,
>
> I noticed that you were still missing a few of my recommendations
> (you keep passing '0' as a parameter that's a gboolean). I fixed all
> these up and streamlined the code a little, and then I noticed that the
> statistics displayed were actually garbled - it displayed values like
> "57ft" for SAC rates... definitely not reasonable :-)
oops, that should have been clear even with the simple test dives I
used for testing.
> I didn't want to send you back to redo one more time, so I fixed that
> part of the code as well and will be pushing all of this out to Linus
> tonight.
>
> It seems pretty obvious that you didn't test the second patch very well
> (what was shown was quite obviously bogus). Please take a little more
> care before submitting patches in the future.
Will do. My possibilities for testing this one were quite limited as
subsurface crashes on me when I import new divelogs from XML files. At
least after the last patch yesterday I was able to add dives manually.
> Once Linus pushes this out, please check that none of my fixes broke the
> actual intended functionality of your code. :-)
Sure, I'll check it out.
> One thing I noticed is that the current code doesn't refresh the display
> when the chosen units are changed in the preferences. That's clearly not
> a show stopper (the user could simply close the statistics window and
> reopen it) but it may be something to look into for the future.
I'll take a look into it.
miika
> Miika Turkia <miika.turkia at gmail.com> writes:
>
>> Using already existing macro for splitting seconds into minutes:seconds.
>> Moving repetitive code to a function (couldn't think of the suggested
>> clever macro, but this should pretty much do the trick).
>> Now the statistics are updated every time the process_all_dives function
>> is called. It might make sense to actually verify the structures need to
>> be re-allocated, but such optimization is currently not implemented.
>>
>> Signed-off-by: Miika Turkia <miika.turkia at gmail.com>
>> ---
>> dive.h | 2 +
>> save-xml.c | 2 -
>> statistics.c | 132 +++++++++++++++++++++++++++++-----------------------------
>> 3 files changed, 67 insertions(+), 69 deletions(-)
>>
>> diff --git a/dive.h b/dive.h
>> index 6826132..e45d962 100644
>> --- a/dive.h
>> +++ b/dive.h
>> @@ -464,4 +464,6 @@ extern const char *star_strings[];
>>
>> #define AIR_PERMILLE 209
>>
>> +#define FRACTION(n,x) ((unsigned)(n)/(x)),((unsigned)(n)%(x))
>> +
>> #endif /* DIVE_H */
>> diff --git a/save-xml.c b/save-xml.c
>> index 3bea2ad..7ce0afb 100644
>> --- a/save-xml.c
>> +++ b/save-xml.c
>> @@ -7,8 +7,6 @@
>>
>> #include "dive.h"
>>
>> -#define FRACTION(n,x) ((unsigned)(n)/(x)),((unsigned)(n)%(x))
>> -
>> static void show_milli(FILE *f, const char *pre, int value, const char *unit, const char *post)
>> {
>> int i;
>> diff --git a/statistics.c b/statistics.c
>> index 86128ae..c5af8ab 100644
>> --- a/statistics.c
>> +++ b/statistics.c
>> @@ -13,7 +13,6 @@
>> #include <stdlib.h>
>> #include <stdarg.h>
>> #include <time.h>
>> -#include <stdbool.h>
>> #include <gdk/gdkkeysyms.h>
>>
>> #include "dive.h"
>> @@ -80,7 +79,6 @@ static stats_t stats_selection;
>> static stats_t *stats_monthly = NULL;
>> static stats_t *stats_yearly = NULL;
>>
>> -bool init_yearly = 1;
>> GtkTreeIter yearly_iter;
>>
>> enum {
>> @@ -213,19 +211,28 @@ static void add_cell_to_tree(GtkWidget *tree, char *value, int index, gboolean r
>> static char * get_minutes(int seconds)
>> {
>> static char buf[80];
>> - int minutes = seconds / 60;
>> - int sec = seconds % 60;
>> - snprintf(buf, sizeof(buf), "%d:%.2d", minutes, sec);
>> + snprintf(buf, sizeof(buf), "%d:%.2d", FRACTION(seconds, 60));
>> return buf;
>> }
>>
>> -void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter *parent)
>> +void add_cell(GtkWidget *tree, GtkTreeIter *parent, unsigned int val, int cell)
>> {
>> double value;
>> int decimals;
>> const char *unit;
>> char value_str[40];
>>
>> + value = get_depth_units(val, &decimals, &unit);
>> + snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
>> + add_cell_to_tree(tree, value_str, cell, 0, parent);
>> +}
>> +
>> +void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter *parent)
>> +{
>> + double value;
>> + const char *unit;
>> + char value_str[40];
>> +
>> /* Year or month */
>> snprintf(value_str, sizeof(value_str), "%d", stats_interval.period);
>> add_cell_to_tree(tree, value_str, 0, 1, parent);
>> @@ -241,29 +248,17 @@ void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter
>> /* Longest duration */
>> add_cell_to_tree(tree, get_minutes(stats_interval.longest_time.seconds), 5, 0, parent);
>> /* Average depth */
>> - value = get_depth_units(stats_interval.avg_depth.mm, &decimals, &unit);
>> - snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
>> - add_cell_to_tree(tree, value_str, 6, 0, parent);
>> + add_cell(tree, parent, stats_interval.avg_depth.mm, 6);
>> /* Smallest maximum depth */
>> - value = get_depth_units(stats_interval.min_depth.mm, &decimals, &unit);
>> - snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
>> - add_cell_to_tree(tree, value_str, 7, 0, parent);
>> + add_cell(tree, parent, stats_interval.min_depth.mm, 7);
>> /* Deepest maximum depth */
>> - value = get_depth_units(stats_interval.max_depth.mm, &decimals, &unit);
>> - snprintf(value_str, sizeof(value_str), "%.*f %s", decimals, value, unit);
>> - add_cell_to_tree(tree, value_str, 8, 0, parent);
>> + add_cell(tree, parent, stats_interval.max_depth.mm, 8);
>> /* Average air consumption */
>> - value = get_volume_units(stats_interval.avg_sac.mliter, &decimals, &unit);
>> - snprintf(value_str, sizeof(value_str), "%.*f %s/min\t", decimals, value, unit);
>> - add_cell_to_tree(tree, value_str, 9, 0, parent);
>> + add_cell(tree, parent, stats_interval.avg_sac.mliter, 9);
>> /* Smallest average air consumption */
>> - value = get_volume_units(stats_interval.min_sac.mliter, &decimals, &unit);
>> - snprintf(value_str, sizeof(value_str), "%.*f %s/min\t", decimals, value, unit);
>> - add_cell_to_tree(tree, value_str, 10, 0, parent);
>> + add_cell(tree, parent, stats_interval.min_sac.mliter, 10);
>> /* Biggest air consumption */
>> - value = get_volume_units(stats_interval.max_sac.mliter, &decimals, &unit);
>> - snprintf(value_str, sizeof(value_str), "%.*f %s/min\t", decimals, value, unit);
>> - add_cell_to_tree(tree, value_str, 11, 0, parent);
>> + add_cell(tree, parent, stats_interval.max_sac.mliter, 11);
>> /* Average water temperature */
>> value = get_temp_units(stats_interval.min_temp, &unit);
>> if (stats_interval.combined_temp && stats_interval.combined_count) {
>> @@ -283,7 +278,8 @@ void process_interval_stats(GtkWidget *tree, stats_t stats_interval, GtkTreeIter
>>
>> static void key_press_event(GtkWidget *window, GdkEventKey *event, gpointer data)
>> {
>> - if (event->string != NULL && event->keyval == GDK_Escape)
>> + if ((event->string != NULL && event->keyval == GDK_Escape) ||
>> + (event->string != NULL && event->keyval == GDK_w && event->state & GDK_CONTROL_MASK))
>> gtk_widget_destroy(window);
>> }
>>
>> @@ -347,6 +343,23 @@ static void process_all_dives(struct dive *dive, struct dive **prev_dive)
>> stats.min_depth.mm = dive_table.dives[0]->maxdepth.mm;
>> stats.selection_size = dive_table.nr;
>> }
>> +
>> + /* allocate sufficient space to hold the worst
>> + * case (one dive per year or all dives during
>> + * one month) for yearly and monthly statistics*/
>> +
>> + if (stats_yearly != NULL) {
>> + free(stats_yearly);
>> + free(stats_monthly);
>> + }
>> + size = sizeof(stats_t) * (dive_table.nr + 1);
>> + stats_yearly = malloc(size);
>> + stats_monthly = malloc(size);
>> + if (!stats_yearly || !stats_monthly)
>> + return;
>> + memset(stats_yearly, 0, size);
>> + memset(stats_monthly, 0, size);
>> +
>> /* this relies on the fact that the dives in the dive_table
>> * are in chronological order */
>> for (idx = 0; idx < dive_table.nr; idx++) {
>> @@ -357,52 +370,37 @@ static void process_all_dives(struct dive *dive, struct dive **prev_dive)
>> *prev_dive = dive_table.dives[idx-1];
>> }
>> process_dive(dp, &stats);
>> - if (init_yearly) {
>> - /* allocate sufficient space to hold the worst
>> - * case (one dive per year or all dives during
>> - * one month) */
>> - if (stats_yearly == NULL) {
>> - size = sizeof(stats_t) * (dive_table.nr + 1);
>> - stats_yearly = malloc(size);
>> - stats_monthly = malloc(size);
>> - if (!stats_yearly || !stats_monthly)
>> - return;
>> - memset(stats_yearly, 0, size);
>> - memset(stats_monthly, 0, size);
>> - }
>>
>> - /* yearly statistics */
>> - tm = gmtime(&dp->when);
>> - if (current_year == 0)
>> - current_year = tm->tm_year + 1900;
>> -
>> - if (current_year != tm->tm_year + 1900) {
>> - current_year = tm->tm_year + 1900;
>> - process_dive(dp, &(stats_yearly[++year_iter]));
>> - } else
>> - process_dive(dp, &(stats_yearly[year_iter]));
>> -
>> - stats_yearly[year_iter].selection_size++;
>> - stats_yearly[year_iter].period = current_year;
>> -
>> - /* monthly statistics */
>> - if (current_month == 0) {
>> + /* yearly statistics */
>> + tm = gmtime(&dp->when);
>> + if (current_year == 0)
>> + current_year = tm->tm_year + 1900;
>> +
>> + if (current_year != tm->tm_year + 1900) {
>> + current_year = tm->tm_year + 1900;
>> + process_dive(dp, &(stats_yearly[++year_iter]));
>> + } else
>> + process_dive(dp, &(stats_yearly[year_iter]));
>> +
>> + stats_yearly[year_iter].selection_size++;
>> + stats_yearly[year_iter].period = current_year;
>> +
>> + /* monthly statistics */
>> + if (current_month == 0) {
>> + current_month = tm->tm_mon + 1;
>> + } else {
>> + if (current_month != tm->tm_mon + 1)
>> current_month = tm->tm_mon + 1;
>> - } else {
>> - if (current_month != tm->tm_mon + 1)
>> - current_month = tm->tm_mon + 1;
>> - if (prev_month != current_month || prev_year != current_year)
>> - month_iter++;
>> - }
>> -
>> - process_dive(dp, &(stats_monthly[month_iter]));
>> - stats_monthly[month_iter].selection_size++;
>> - stats_monthly[month_iter].period = current_month;
>> - prev_month = current_month;
>> - prev_year = current_year;
>> + if (prev_month != current_month || prev_year != current_year)
>> + month_iter++;
>> }
>> +
>> + process_dive(dp, &(stats_monthly[month_iter]));
>> + stats_monthly[month_iter].selection_size++;
>> + stats_monthly[month_iter].period = current_month;
>> + prev_month = current_month;
>> + prev_year = current_year;
>> }
>> - init_yearly = 0;
>> }
>>
>> /* make sure we skip the selected summary entries */
>> --
>> 1.7.9.5
>>
>
> --
> Dirk Hohndel
> Intel Open Source Technology Center
More information about the subsurface
mailing list