GNOME Bugzilla – Bug 116246
gtk_spin_button_configure()
Last modified: 2014-08-30 04:50:01 UTC
void gtk_spin_button_configure (GtkSpinButton *spin_button, GtkAdjustment *adjustment, gdouble climb_rate, guint digits) last line of the functions code is: gtk_adjustment_value_changed (adjustment); This sends the "value_changed" signal for the adjustment, yet the adjustments value has not been changed by this function.
As a general rule, any change notification from GTK+ is notification of a potential change. You should never assume that something actually *did* change just because you received a notification. The emission of "value_changed" here definitely does have some purpose; just removing it would potentially break applications: - If you have a new adjustment, then you have a new value, and we need to get gtk_spin_button_value_changed() to get things synced up. - Even if you don't have a new adjustment, you still need to get gtk_spin_button_value_changed() if the number of digits changed. See http://mail.gnome.org/archives/gtk-devel-list/2003-June/msg00027.html for some relevant general principles. I think the easiest thing here would be to fix up the code structure of GtkSpinButton to conform to GTK+-2.x norms: - Make gtk_spin_button_configure() just a compatibility wrapper that does: g_object_set (G_OBJECT (spin_button), "climb-rate", climb_rate, "digits", digits, adjustment ? "adjustment" : NULL, adjustment, NULL); - Make gtk_spin_button_new() pass properties to g_object_new() rather than using gtk_spin_button_configure() - Add a gtk_spin_button_constructor() to do any necessary handling at the end of creation. (See the handling gtk_layout_constructor(). It may be that by making "adjustment" a G_PARAM_CONSTRUCT property this would be unnecessary.) - Fix set_adjustment to properly cause updating of the value. I know that sounds like a lot of reorganaization for a simple extra signal emission, but any change here has the potential to break some app (what is an extra signal emission to you may be something that another app is depending on), so if we are going to change things, we should try to get it right. Putting on 'future', though it could be moved back to 2.4.0 if a patch showed up soon.
Created attachment 81001 [details] [review] Patch implementing Owen's plan Tried to implement Owen's suggestions to fix the issue. Can anyone review? :)
Comment on attachment 81001 [details] [review] Patch implementing Owen's plan xan: You don't need to g_object_notify() when implementing set_property() ... that is done automatically <owen> xan: + adjustment ? "adjustment" : NULL, adjustment, <owen> you know that adjustment is non-null there (in the second usage) since you create it yourself <owen> http://mail.gnome.org/archives/gtk-devel-list/2003-June/msg00027.html <owen> * If you replace the adjustment of a range widget with a new one, <owen> all properties of the new adjustment are preserved and applied to the <owen> range widget. <owen> So, that's basically what happens now; I'd probably replace the manual call to gtk_widget_queue_resize() with a call to adjustment_changed_cb, and move the call to gkt_spin_button_valude_changed and g_object_notify() inside the if (spin_button->adjustment != adjustment) check. From the same mail: <owen> xan: Also, from the mail * Setting an adjustment of a range, a scrollable widget, or <owen> a scrolled window to NULL replaces the current adjustment with a newly <owen> created one with identical values to the current adjustment. it would be good to implement that instead of just setting the adjustmnet to null as currently. (Don't break the usage of set_adjustmnet() in finalize however! you might have to have the public function a wrapper around something private) Mostly the patch looks
.... good
Created attachment 84266 [details] [review] Hopefully this addresses the points raised.
Actually, the new patch is broken. On NULL set_adjustment will set an adjustment with all parameters set to 0.0, but the the values need to be the same as the old one. Do we really need to create a *new* one on NULL, why can't we keep the old one? If for some reason we need to create a new one, I think it would make sense to add a gtk_adjustment_copy function to GtkAdjustment...
I think this was addressed with recent changes