Re: Patch: Bug 50278

[Owen Taylor <otaylor redhat com>]

Mike Kestner <mkestner enteract com> writes:

> Here's a patch for the SpinButton convenience API enhancements.
> 
> Let me know if it's broke-ass or if I can commit. 
> 
> I tried it out with testgtk using some funky step values and it seems to
> pick proper digits values and such. 

I like this patch, and think it is just about ready to commit, with
a few comments:

> +GtkWidget *
> +gtk_spin_button_new_with_range (gfloat value,
> +		     		gfloat min,
> +		     		gfloat max,
> +		     		gfloat step,
> +		     		gfloat page)

I'd like to cut off two of the arguments here to get:

 GtkWidget *
 gtk_spin_button_new_with_range (gfloat min,
	 	     		 gfloat max,
		     		 gfloat step);

The more you can reduce the number of arguments, the better.

I think almost everybody is going to use page = step * 10, and forcing
people to pick a value is just annoying.

And for editable controls (checkbuttons, entries, and so forth),
we typically don't include the initial value in the constructor.

There is an implicit separation:

 - The setup of the widget
 
 - The value of the widget

And since you'll almost certainly need to adjust the value in
more code, there is little advantage to putting the value in
the constructor.

Also, you need a documentation comment here.

> +void 
> +gtk_spin_button_set_increment (GtkSpinButton *spin_button, 
> +			       gfloat         step,
> +			       gfloat	      page)

> +{
> +  g_return_if_fail (spin_button != NULL);

As Tim said, We don't include the spin_button != NULL checks any more
since IS_SPIN_BUTTON does that.

> +  g_return_if_fail (GTK_IS_SPIN_BUTTON (spin_button));
> +
> +  spin_button->adjustment->step_increment = step;
> +  spin_button->adjustment->page_increment = page;
> +}
> +

This either needs to be set_increments() as Tim said, or drop the page
argument and be gtk_spin_button_set_step(). I think the UI part of
moving by pages is so obscure that configuring it as something other
than step * 10 isn't important, and gtk_spin_button_set_step is
better.

> +void 
> +gtk_spin_button_set_range (GtkSpinButton *spin_button, 
> +			   gfloat         min,
> +			   gfloat	  max)
> +{
> +  g_return_if_fail (spin_button != NULL);
> +  g_return_if_fail (GTK_IS_SPIN_BUTTON (spin_button));
> +
> +  spin_button->adjustment->lower = min;
> +  spin_button->adjustment->upper = max;
>  }

It's a little peculiar that new_with_range() takes min/max/step
while set_range takes min/max, but on consideration, probably
for the best.

> +      spinner = gtk_spin_button_new_with_range (0.0, 0.0, 10.0, 0.09, 1.5);
> +      gtk_object_set_user_data (GTK_OBJECT (spinner), val_label);

gtk_object_set_user_data() is highly obsolete. You should always use
a specific key (or actually g_object_set_data(), now)

> +      gtk_signal_connect (GTK_OBJECT (spinner), "value_changed",
> +			  GTK_SIGNAL_FUNC (get_spin_value),
> +			  GINT_TO_POINTER (2));

This is pretty ugly even for testgtk... why don't you add a quick
enum here.

Why don't you send another version of the patch with the doc comment
and the suggested changes; Havoc agrees with the above modifications,
and hopefully nobody else will disagree too strongly ;-)

I'll give that a quick look-over and then you should be able to go
ahead and commit it.

Thanks,
                                        Owen