GNOME Bugzilla – Bug 619768
Remove public fields from GtkAdjustment structure
Last modified: 2011-02-02 15:29:29 UTC
Created attachment 162057 [details] Sample implementation of GtkAdjustment for GTK+-3.0 During last GTK+ meeting, there was a discussion about how to properly remove public fields from GtkAdjustment structure. Quite large amount of GTK+ code is accessing those fields directly and emitting proper signals manually after the modification, which makes removal of those fields rather delicate. During the meeting, I proposed two solutions to this problem: - make accessors replacement for direct field setting and leave signal emission to the owner - make accessors wrappers for g_object_set() and emit signals together with ::notify signal First solution is quite simple to implement and updating existing GTK+ code would be a breeze. But there are some ugly sides to this approach: - we would need to add some rather bizarre functions to public API in order to keep at least some familiarity with existing API (for example, gtk_adjustment_set_value() emits a ::value-changed signal when value changes, and if we want to have direct access to adjustment fields, function like gtk_adjustment_set_value_direct() would need to be added, which would essentially do exactly the same thins as _set_value(), but without automatic signal emission) - we loose any encapsulation of functionality that GObject brings, because now user is completely responsible for keeping GtkAdjustment in sane state, emitting signals ... Second solution would take more work to implement and changes in GTK+ sources will be more extensive, but we would be able to maintain almost 100% backwards compatible and functionality is nicely encapsulated inside the object. Downsides of this approach are: - it is less efficient, because all property changes need to go through g_object_set() - control over signal emission is a bit harder, because it can only be influenced indirectly (through g_object_(freeze|thaw)_notify()) One additional benefit of the second solution would be the ability to force GtkAdjustment into sane state after each change. For example, if adjustment is configured like this (only relevant fields are shown): lower: 0 upper: 100 page_size: 20 value: 70 If we change page_size to 40, we get this: lower: 0 upper: 100 page_size: 40 value: 70 Using first method, there is no way GtkAdjustment can catch this and so owner is responsible for fixing this (probably setting value to 100 - 40 = 60) and emitting ::value-changed signal. Second solution would be able to catch such situation and fix it automatically, sparing quite a few lines of code. I attached a sample implementation of second proposal. It's a complete gtkadjustment.c file since diff wouldn't be much smaller (if at all). Header file holds nothing special (standard GObject boilerplate code) and is not attached here. Any comments, ideas, suggestions or constructive criticism are welcome.
Created attachment 163095 [details] Updated GtkAdjustment implementation
Please upload a patch instead. Patch aside, I really don't see why GtkAdjustment should be in any way a special case. Why can't people just use the newly added API that exists since 2.14? Yes it's a bit of work to port, but I really don't see why we should add hackish API that allows to set members without any notification or signal emission. I always wonder why it takes me about two or three hours to port GIMP to a new GTK+ version, while other people spend that time on IRC whining about the "huge" amount of work ahead of them. I'm sorry if I sound harsh here, but if people are not willing to maintain their code and do constant refactoring to prevent bit rot, then their software should probably die.
Created attachment 163182 [details] [review] Diff against existing code
> Patch aside, I really don't see why GtkAdjustment should be in any > way a special case. Why can't people just use the newly added API > that exists since 2.14? > > Yes it's a bit of work to port, but I really don't see why we should > add hackish API that allows to set members without any notification > or signal emission. Truth to be told, I was not thinking about applications much when I was writing this (screw them;). My sole goal was to keep GTK+ itself working after removal of GSEALed fields. One of the most problematic classes is GtkRange with it's various update policies. Values are changed when the user interacts with GtkRange derivate, but GtkAdjustment::value-changed signal is only emitted later when policy is set to either GTK_UPDATE_DELAYED or GTK_UPDATE_DISCONTINUOUS. > I always wonder why it takes me about two or three hours to port > GIMP to a new GTK+ version, while other people spend that time > on IRC whining about the "huge" amount of work ahead of them. One is doing a lot of work to get application working with GTK+-3, but it's completely different thing not being able to replicate some functionality. Worse still, even GTK+ itself is not able to work properly with accessors available right now. BTW, even with my proposed changes, there will be no shortage of work to be done before GTK+ compiles again. Only searching for "gtk_adjustment_(value_)?changed" in GTK+ sources returned 10 files; I don't even want to thing about how many times adjustment fields were accessed directly inside the code. > I'm sorry if I sound harsh here, but if people are not willing > to maintain their code and do constant refactoring to prevent > bit rot, then their software should probably die. With my comments in place, this could as well be rephrased as: "If people don't care about GTK+, it must die!". ;-)
In order to make my changes a bit clearer, I'll explain what changes did I do to GtkAdjustment and why. 1. ::value-changed signal is emitted from dispatch_properties_changed() Reason for this change is to make it possible to delay ::value-changed emission view g_object_freeze_notify(). Without this change, some of the old functionality cannot be "recoded". 2. :value property is now set inside _set_property() function This is done in order to make 1) possible. 3. Two additional rules were added: - upper >= lower (added) - page_size <= upper - lower (added) - lower <= value <= upper - page_size (already present) These rules describe all sane states that adjustment can be in. 4. Rules are enforced. By enforcing upper rules, applications can skip some test in their own code that were doing this previously (one typical example where checking for valid was needed is setting :upper property, since this operation could cause :value to become invalid). 5. Some convenience wrappers were added for more efficient changing of :value Hopefully this makes sense.
Created attachment 163235 [details] [review] Minimal set of changes that are needed in order to retain current functionality Seems like people would really prefer to have "dumb" version of GtkAdjustment that would do just enough to replicate current functionality. I'm providing a patch with a minimal set of changes needed to accomplish this. No checking is done, only exception being :value property, whose value is clamped to interval [lower, upper - page_size].
(In reply to comment #6) > Seems like people would really prefer to have "dumb" version of GtkAdjustment > that would do just enough to replicate current functionality. I'm providing a > patch with a minimal set of changes needed to accomplish this. > It turns out that GtkRange breaks (aka renders badly) in GTK3 if those constraints aren't enforced, so I'm looking at enforcing these rules for GTK3. What made you think people don't want these rules enforced?
(In reply to comment #7) > (In reply to comment #6) > > Seems like people would really prefer to have "dumb" version of GtkAdjustment > > that would do just enough to replicate current functionality. I'm providing a > > patch with a minimal set of changes needed to accomplish this. > > > It turns out that GtkRange breaks (aka renders badly) in GTK3 if those > constraints aren't enforced, so I'm looking at enforcing these rules for GTK3. > > What made you think people don't want these rules enforced? the 'intermediate state' problem In the past, people have been free to do the equivalent of set_lower (adj, 100) set_upper (adj, 200) set_value (adj, 150) ... go to use adjustment if we enforce constraints all the time, setup becomes more cumbersome
Well, there is gtk_adjustment_configure() do to such a setup. The problem is that if we don't enforce constraints, everybody using the adjustments needs to enforce the constraints manually. So who do we want to do the work of fixing their code? Developers setting the values or developers getting the values?
I think this can be close now, Benjamin?
Not if I close this first!