After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 619768 - Remove public fields from GtkAdjustment structure
Remove public fields from GtkAdjustment structure
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
2.90.x
Other All
: Normal blocker
: 3.0
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 611955 619474 619493
 
 
Reported: 2010-05-26 23:50 UTC by Tadej Borovšak
Modified: 2011-02-02 15:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Sample implementation of GtkAdjustment for GTK+-3.0 (26.35 KB, text/plain)
2010-05-26 23:50 UTC, Tadej Borovšak
  Details
Updated GtkAdjustment implementation (31.62 KB, text/plain)
2010-06-08 19:37 UTC, Tadej Borovšak
  Details
Diff against existing code (33.15 KB, patch)
2010-06-09 10:29 UTC, Tadej Borovšak
none Details | Review
Minimal set of changes that are needed in order to retain current functionality (13.47 KB, patch)
2010-06-09 21:35 UTC, Tadej Borovšak
none Details | Review

Description Tadej Borovšak 2010-05-26 23:50:11 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.
Comment 1 Tadej Borovšak 2010-06-08 19:37:18 UTC
Created attachment 163095 [details]
Updated GtkAdjustment implementation
Comment 2 Michael Natterer 2010-06-09 10:23:27 UTC
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.
Comment 3 Tadej Borovšak 2010-06-09 10:29:52 UTC
Created attachment 163182 [details] [review]
Diff against existing code
Comment 4 Tadej Borovšak 2010-06-09 11:40:35 UTC
> 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!". ;-)
Comment 5 Tadej Borovšak 2010-06-09 13:04:00 UTC
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.
Comment 6 Tadej Borovšak 2010-06-09 21:35:52 UTC
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].
Comment 7 Benjamin Otte (Company) 2010-09-15 15:20:45 UTC
(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?
Comment 8 Matthias Clasen 2010-09-15 15:47:07 UTC
(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
Comment 9 Benjamin Otte (Company) 2010-09-15 17:53:03 UTC
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?
Comment 10 Javier Jardón (IRC: jjardon) 2011-02-02 14:28:31 UTC
I think this can be close now, Benjamin?
Comment 11 Benjamin Otte (Company) 2011-02-02 15:29:29 UTC
Not if I close this first!