Move bugs which are tracking planned 2.4 features to the proper target
milestones.

An implementation by Iain Holmes can be found in
gnome-panel/gnome-panel/disclosure-widget.[ch]. 

Looks like it even supports automatic RTL flipping. Great!

But it is GPLed. I guess we'd have to get it relicensed as LGPL for
inclusion in GTK, right ?

Looking at the widget some more, we probably want to allow arbitrary
widgets instead of only labels in the button, and for labels, we want
to allow mnemonics. 
I wonder if the toggling between two different label texts for
show/hidden state is really worthwhile (we don't support this for
toggle buttons either)

A little experiment teached me that arbitrary widgets and mnemonics
work  fine; we may just need a few convenience constructors.

Iain agreed to put his code unter LGPL; I adjusted it to the GTK
coding style and naming conventions and added two more constructors.

Created attachment 14063 [details]
gtkdisclosure.h

Created attachment 14064 [details]
gtkdisclosure.c

Created attachment 14065 [details] [review]
Makefile.am patch

Created attachment 14066 [details] [review]
doc patch

Created attachment 14067 [details] [review]
testgtk patch

Please see comments on RTL environments for this widget on bug 98215
and 98213. The good patch is the commited in 98213

Thanks for the hint, but the version I adapted (from gnome-media)
seems to have that patch already. There are no obvious visual problems
in RTL mode.

I have problems in RTL with gnome-media 2.1.5 and cvs HEAD doesn't has
the patch applied. The file gnome-panel/disclosure-widget.c of
gnome-panel cvs module has the patch applied

Maybe you can see problems if you change your font size or resize the
track editor window.

I much prefer the idea of this being a GtkBin subclass rather than a
checkbutton which you attach a container to - it just seems to make a
lot more sense.

I've written a widget which does this and called it GtkExpander. It
works quite well and I think the API is more obvious, but the
implementation is quite a good deal more complex than Iain's one ...
so I'm not totally convinced its a better idea.

I'll attach it.

Created attachment 14563 [details]
gtkexpander.h

Created attachment 14564 [details]
gtkexpander.c

Iain: interested to here what you think ...

It does seem a lot more complicated
but the API is better (mine sucked)

Personally I don't care if the official one is the one I wrote or not
I didn't write it to go into GTK, so go for it.

Mark, from looking at the headers, one thing Iains implementation does which yours 
doesn't is allowing arbitrary widgets instead of the label.
Although it should be 
fairly trivial to add 

gtk_expander_set_label (GtkExpander *expander, 
GtkWidget *label);

Just a comment for future interface thoughts wrt to this widget...
Ideally we would actually resize the window dynamically as this widget
opens. So it might be worth making sure there are signals sent out
before the "opening" animation starts, and perhaps some basic thinking
into how you could get the contents of the disclosure triangle to
slide out. Star trek future I know....

Oh, I forgot - when I was doing GtkExpander I wrote down the layout
similar to doc/widget_geometry.txt. I'll attach it.

Created attachment 14946 [details]
layout

Created attachment 15376 [details] [review]
Updated header

Created attachment 15377 [details]
Updated implementation

Created attachment 15552 [details]
further updated implementation

"updated header/implementation":

The changes discussed in:

http://mail.gnome.org/archives/gtk-devel-list/2003-April/msg00005.html

"further updated implementation":

* Fixes for several sillies pointed out by Ray Strode
* Widget now allocates the maximum available size rather than just the
size requested by the child. Suggested by Ray.
* A patch from Ray to fix up focus handling in gtk_expander_focus()

Implementation comments:

* The widget should be a NO_WINDOW widget that uses an
  INPUT_ONLY window to catch events. THe way it is
  currently, it won't theme nicely. GtkButton is probably
  a good template for how to do this.

  You should then be able to use only enter_notify/leave_notify
  and forget maybe_prelight(), and probably eliminate
  is_in_expander_panel() entirely.

* Returning a GdkRectangle as in get_expander_bounds()
  is something we generally avoid in GTK+. I'd pass
  in a GdkRectangle * instead.

* It looks to me like gtk_expander_paint() doesn't handle
  insensitivity right. Why the code 

  if (state != GTK_STATE_PRELIGHT)
    state = GTK_STATE_NORMAL

  ?

* the code needs to catch grab shadowing and being set
  insensitive, or it will stick in the button_down
  state. See gtk_spin_button_grab_notify(),
  gtk_spin_button_state_changed(). 

* The code needs to use gtk_widget_set_child_visible()
  rather than directly showing/hiding the child ...
  gtk_widget_show/hide() is for applications.

* Generally, removing timeouts in destroy isn't a good
  idea, since things can happen between destroy and
  finalize that might result in them getting added
  back. There are lots of possibilities, but what I 
  might do is simply remove the timeout in unrealize
  and not add it unless GTK_WIDGET_REALIZED(expander)

* What the appearance should be for !interior_focus
  is a little obscure to me, and I'm not even entirely
  sure it's worth trying to do anything. But I'd guess
  that it should be only around arrow+label, not around
  the whole widget.

* Why is 2 * priv->spacing added to the height in size_request()?
  [and correspondingly used in size_allocate()]  I guess
  the anology is with expander_spacing, but I think it's
  not really right ... the spacing should be the space
  between label and child, and the border width is the
  external padding.

* In size_allocate(), I think it's better if, when insufficient
  space is available, the child is squashed rather than the
  label.

* gtk_expander_set_label_widget() needs to queue_notify
  on "label" (see patch in bug 108305 for the fix to
  GtkFrame)

* There is some question as whether gtk_expander_forall()
  with include_internals = TRUE should include automatically
  generated label widgets. See bug 105541. I'd leave it
  as you have it for now, I guess.

* gtk_expander_focus() needs to take gtk_widget_get_direction()
  into account when focusing LEFT/RIGHT between the label
  and the widget.

* gtk_expander_focus() should handle label widgets with
  multiple focus sites ... so it needs
  to call gtk_widget_child_focus() on it.

* the label_can_focus check is unecessary and harmful - 
  just gtk_widget_child_focus() and if that returns
  false, go on to the next widget.

* gtk_widget_child_focus() doesn't handle return of
  FALSE from focus_child_in() correctly in all places.

  [ an enumeration for the 3 different focus sites,
    and some helper functions 
    focus_current_site(), focus_in_site (widget, site)
    next_site (widget, site, direction) or something
    might help a lot in cleaning up this function ]

 * calling gtk_widget_set_state(widget) for prelight
   is going to prelight the *child* widget when
   expanded along with the label, which is doubtless not 
   intended. See gtknotebook for how it sets the states of the
   tabs. 

Minor patch details:

 * In size_allocate(), there are some MAX (dimension, 0),
   but the minimum size of an X window is 1, so using that
   is clearer (even though GDK clamps itself)

* The GTK_EXPANDER_GET_PRIVATE(o) definition should be 
  in the C file, and I think we decided that we'd 
  go ahead and for new widgets add a 'priv' field 
  the the instance structure and define
  GTK_EXPANDER_GET_PRIVATE(e) (GTK_EXPANDER(e)->priv)

* gtk_widget_expander_set/get_use_underline/set_expanded/
  get_expanded/set_spacing/get_spacing/new/new_with_mnemonic
  are missing doc comments.

* "Whether or not the expander is expanded" strikes me
  as a bit of a lame comment for "Expanded". 

  "Whether the expander has been opened to reveal the
   child widget", perhaps. (Standard GTK+ phrasing omits
   the "or not')

* For struct definitions, the opening { should be on it's
  own line.

* I'd omit the need_resize stuff in set_label_widget() ...
  queue_resize() needs to be fast for a lot of other reasons
  in GTK+, so better to keep code size small.

OK, I think that's enough comments for now :-). Lots of details,
but it broad outline it looks very much right.

Why shouldn't this be two widgets?
One a GtkCheckButton subclass, like GtkDisclosure.
The other a GtkFrame subclass that uses the GtkDisclosure as
the label widget, turns off the shadow, and make whatever
spacing adjustments are needed.

Please see gnomechat for an example of a disclosure triangle
that doesn't simply reveal something below it.

Just so there's no duplication of effort, I am actually working on
Owen's comments ...

Thats great. I'll find something else to do...

Created attachment 17829 [details]
header

Created attachment 17830 [details]
impl

Created attachment 17831 [details]
layout

Owen: okay I think I've done everything you suggested - every comment
was spot on :-)

I'm still not entirely with the focus code, but I think what I'll do
is to go ahead and commit sometime in the next few days and let you
fix any outstanding problems you see. Scream if you want me to hold
back ...

Okay, I've gone ahead and committed.