Needs an API proposal

Created attachment 13960 [details] [review]
API Proposal

Created attachment 14791 [details] [review]
Propsoed implementation

But what's the point? There are a bunch of other bugs in
gtkrc.c currently ... it's in an half-finished state.

(The commit to gtkrc.c was accidental ... I told Pablo
not to back it out because I thought I was going to
finish it, but never did.)

If you want to submit a patch to configure.in for CVS
that makes it bail out with:

 "This branch of CVS no longer is maintained, and
  contains bugs. Use GTK+-1.2.10 instead"

That might be useful.

Owen,

Is your comment intended for a different bug?

Yes, sorry.

owen: comments on the patch?

Updating status_whiteboard field to reflect A11Y team's assessment 
of accessibility impact.

Is gtk_scale_get_text needed? Seems to duplicate 
get_layout().
  
Of the top of my head, copying:

PangoLayout *gtk_label_get_layout         (GtkLabel *label);
void         gtk_label_get_layout_offsets (GtkLabel *label,
                                           gint     *x,
                                           gint     *y);

would make sense, except that there is a fundamental
difference that gtk_label_get_layout() returns a layout
owned by the label, so the get_layout() name is 
inappropriate.

The most straightforward way to resolve this, which would
also solve some of the implementation ugliness that is
there now (creating a temporary layout to get the offsets)
would be to simply cache a layout (in instance-private-data)
for the h/vscale. It's probably best to do this in gtkscale.c 
and have

 _gtk_scale_clear_layout()
 _gtk_scale_get_layout()

where get_layout() creates if necessary, and clear_layout()
is called when in similar circumstances to gtk_label_clear_layout().

  

  

Created attachment 18507 [details] [review]
Updated patch

Apologies for spam... marking as GNOMEVER2.3 so it appears on the official GNOME
bug list :)

I guess the patch should use instant-private data instead of qdata.

In general, the patch looks very good. Various detailed comments:

* As Matthias said, it should use instance-private-data instead
  of qdata. See, e.g., GtkWindow for how this works.

* There's a 'disdplay' typo in gtk_scale_get_layout() docs.

* gtk_scale_get_layout() docs need to document that it returns
  %NULL if !scale->draw_value.

* gtk_scale_get_layout() segfaults if !scale->draw_value

* gtk_scale_get_layout_offsets() needs a 
  g_return_if_fail (GTK_IS_SCALE (scale))

* I think gtk_scale_get_layout_offsets() is going to be
  considerably simpler if you always use local_x/local_y
  and then copy out if x != NULL, y != NULL.

* gtk_scale_get_layout_offsets docs should say that the
  return values are undefined if the scale isn't 
  displaying the value. (It's fine to set them to zero
  in that case, but I wouldn't describe that in the docs)

* The if (G_IS_OBJECT (layout)) in _gtk_scale_clear_layout
  seems wrong to me. Should just be if (layout)

* The header file has gtk_scale_get_layout_offset
  not layout_offsets()


* gtk_hscale_get_layout_offsets() shouldn't have a g_return_if_fail(),
  we don't do that for virtual functions.

* The gtk_hscale_get_layout_offsets() 
  / gtk_hscale_do_get_layout_offset()
  split makes things harder to read to me. I'd just do
   
  if (!layout)
    {
       *x = 0;
       *y = 0;
       return;
    }

Committed with the changes outlined by Owen.