dive planner fixes [was: Re: source code may be getting a bit intimidating]

[Dirk Hohndel]

Linus Torvalds <torvalds at linux-foundation.org> writes:

> On Mon, Jan 7, 2013 at 12:57 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
>>
>> Ok, I found two bugs in the dynamic update code related to gas
>> changes. Please try again, this should work now (mind you, it doesn't
>> have the "keep the last gas" logic, yet, so once you enter depth and
>> time you get a pointless gaschange shown, once you add the gas it goes
>> away again (assuming you entered the same gas) or becomes valid (if you
>> actually changed gas)).
>
> Ok, we crossed paths.

Certainly. And your patch implements some things I had already started
on, just differently. I'll throw away what I have in my working branch
and apply yours.

I do have a couple of questions, but my mailer doesn't quote the
attachments when replying...

So I'll do it hear with just cut and paste copies of your patch:

      -		for (i = t; i < dp->time; i += 10) {
      -			depth = lastdepth + (i - t) * (dp->depth - lastdepth) / (dp->time - t);
      -			sample = prepare_sample(dc);
      -			sample->time.seconds = i;
      -			sample->depth.mm = depth;
      -			sample->sensor = gasused;
      -			dc->samples++;

I assume you are removing the intermediate samples because with your
change to the plot_info this isn't needed for things to look good...

But this has an interesting side effect. We do the deco calculation
based on the samples here, while profile.c does it based on the
plot_info. While I think that shouldn't matter, I haven't fully
convinced myself that this doesn't cause issues. I guess if this does
cause an issue it mostly points at a bug in the underlying calculations,
as the intermediate samples shouldn't make any differents on the path
along which we want to calculate deco.

Why am I bringing this up? The deco code doesn't do slopes. It does
'time at this level'. So that's why we break this up into 1s steps.
We do this between the plot_info entries and we do it here with the
samples. Given that we do linear interpolation and are doing this at mm
resolution I don't see any obvious reasons why this should be different,
but I still worry if this could cause odd corner cases.

The other comment is on this:

      @@ -542,48 +542,43 @@ void show_planned_dive(void)

       static gboolean gas_focus_out_cb(GtkWidget *entry, GdkEvent *event, gpointer data)
       {
      -	char *gastext;
      +	const char *gastext;
	      int o2, he;
	      int idx = data - NULL;

      -	gastext = strdup(gtk_entry_get_text(GTK_ENTRY(entry)));
      -	if (validate_gas(gastext, &o2, &he)) {
      +	gastext = gtk_entry_get_text(GTK_ENTRY(entry));
      +	o2 = he = 0;
      +	if (validate_gas(gastext, &o2, &he))
		      add_string_list_entry(gastext, gas_model);
      -		add_gas_to_nth_dp(&diveplan, idx, o2, he);
      -		show_planned_dive();
      -	} else {
      -		/* we need to instead change the color of the input field or something */
      -		printf("invalid gas for row %d\n",idx);
      -	}
      -	free(gastext);
      +	add_gas_to_nth_dp(&diveplan, idx, o2, he);
      +	show_planned_dive();
	      return FALSE;
       }

       static void gas_changed_cb(GtkWidget *combo, gpointer data)
       {
      -	char *gastext;
      +	const char *gastext;
	      int o2, he;
	      int idx = data - NULL;

      -	gastext = strdup(gtk_combo_box_get_active_text(GTK_COMBO_BOX(combo)));
      -	if (validate_gas(gastext, &o2, &he)) {
      -		add_gas_to_nth_dp(&diveplan, idx, o2, he);
      -		show_planned_dive();
      -	} else {
      +	gastext = gtk_combo_box_get_active_text(GTK_COMBO_BOX(combo));
      +	o2 = he = 0;
      +	if (!validate_gas(gastext, &o2, &he)) {
		      /* this should never happen as only validated texts should be
		       * in the dropdown */
		      printf("invalid gas for row %d\n",idx);
	      }
      -	free(gastext);
      +	add_gas_to_nth_dp(&diveplan, idx, o2, he);
      +	show_planned_dive();
       }

This calls show_planned_dive even if the gas didn't validate with a
default of AIR. I'm not sure this is the right thing to do - shouldn't
we just leave what's in the diveplan alone? Let's say someone has 20/30
in the diveplan, then edits it to read "20^30" by mistake. Should that
switch to AIR and recalculate and redisplay the dive? Or should this
simply change the color of the cell but keep the old value and not
redraw the profile?

> Here's my patch re-based on top of yours. We found and fixed the same
> gas index bug, so it conflicted, and you added the gas "changed"
> callback, but most of my patch ends up being valid.

Ok, as I said, I'm willing to apply yours and continue from there.

> So with this, leaving the gas empty will actually mean "use same gas
> as last entry".
> With air being the default first entry if you have nothing at all.

I implemented that completely differently but like your version better.

So any feedback on the above? Can I get a signed-off patch with a commit
message? 

Thanks

/D