I think this makes sense. 

Here is a first cut at it that basically takes the epi widget  with few changes
(adding the _new and _new_from_stock functions) and moves it to the gtk coding
style.

It's still rough, but I thought to attach it for two reasons:
- get api feedback etc
- avoid for anyone that wants to work on it the grunt work of reindenting etc


I decided to use "DropdownToolButton" instead of "ArrowToolButton" (got the name
from the evo implementation)

some of the open issues:

- should the priv->menu be created? (ephy implementation creted it, while I
chosed to have a set_menu function... looked like a more familiar api to me -
see the old OptionMenu)

- still need to figure out how to handle separate tooltips

- what happens when it's packed at the end of the toolbar, the window is
resized, and the button should end up in the overflow menu?

- should other internal stuff be implemented? (for instance in
gtktoggletoolbutton I see the menu_proxy stuff... I've no idea what it is)

Created attachment 31132 [details]
eggdropdowntoolbutton.c

Created attachment 31133 [details]
eggdropdowntoolbutton.h

>- should the priv->menu be created? (ephy implementation creted it, while I
>chosed to have a set_menu function... looked like a more familiar api to me -
>see the old OptionMenu)

I'd like to (and I was going to do this for ephy 1.6) make it use GtkUIManager
for the popdown menu, i.e. just have it know which ui manager to use and which
path to get the menu from it.
For use without a UI manager, set_menu looks better to me, too.

>- what happens when it's packed at the end of the toolbar, the window is
>resized, and the button should end up in the overflow menu?

IMHO the popdown menu should become the submenu of the proxy menuitem.

Can you please commit this in libegg, so that I can move ephy 1.5 to use this
from libegg instead of its own copy? Thanks.

I agree with everybody that such a widget makes sense, and I don't even think
it's inconceivable that we can get it in 2.6. Please go ahead and commit it to
libegg.

One thing I thought about is how it would interact with homogeneous items.
Epiphany turns homogeneous-ness off for drop-down buttons (for good reasons),
but that is not quite the desired behavior in my opinion. Ideally, the button
part of the drop down widget would be treated as homogeneous, ie. the part of
the widget that isn't an arrow would be made the same size as other homogeneous
buttons. 

To do this, we could add a new signal to GtkToolItem, 

   void homogeneous_size_request (int *size_request),

that would allow items to report how wide they should be considered for
homogeneous-ness purposes. The default handler for this signal would just return
the size_reques.width, but the DropDownItem would override it to report the
requested width of its button instead of its total width.

I'll try to put together the required autotools stuff tomorrow and commit this
preliminary version in libegg tomorrow.

Note however that my goal with the bugreport and the first cut implementation
was to get the ball rolling, but I do not feel qualified enough to do the
complete implementation.
I know too little about gtk+ internals and in particular about the
toolbar/UImanager parts :(

committed in libegg/dropdowntoolbutton/

I noticed one problem with the code, namely that it's based on the epiphany impl
which is GPL, not LGPL which is needed for inclusion in gtk+. Marco says he
wrote the widget, but probably had copied from galeon. So we need to track all
people involved and see if they will relicense -- or we need to start from
scratch; since it's not a big chunk of code, that might be easier ;) (?)
At marco's suggestion, CC:ing Ricardo.

that's why I cc'ed teuf: that's the one that had the (c) on the epiphany file.

looking in viewcvs it looks like successive changes to the epiphany file are
mostly oneliners so they aren't copirightable[1] (does such a word exist?)...
I've not been able to dig the galeon version though.


[1] according to the evolution copiright assignment debate

I didn't write this widget, I don't know why I own the copyright on that file...
I'm generally happy to relicence code I have written from GPL to LGPL, but since
I'm not the author of that particular piece of code, that's not really useful ;) 

I wrote the current widget for the galeon toolbar, but I think it is posterior
to the epiphany fork (not sure). I wrote also the previous galeon
implementation, or at least that what cvs logs seem to say.

Either way, I'm happy relicensing any of this code thay I may have written under
the LGPL.

thanks Ricardo!

How does the current galeon widget differs? Does the one proposed fit your needs?

btw, can you post a link to the current galeon implementation on viewcvs?

It's here:
http://cvs.gnome.org/viewcvs/galeon/utils/gul-toolbutton.h?rev=1.4&view=markup

Galeon's one is very similar to Epiphany's. If it is posterior to the fork (as I
think) it is very probable that I looked at Epiphany's and not the other way around.

Galeon's widgets add the following:
  - The arrow can be hidden.
  - The menu is also shown when right clicking in the button.
  - You can get a pointer to the internal button (I think we needed this for dnd).

I like the idea of a manual "set_menu", or at least it should be possible to
create the menuitems manually, not only through GtkUIManager.

some updates on what is currently in libegg:

- I fixed some bugs and added a super simple test app
- I added an api to set the tooltip for the arrow button
- I made the menu show up on right click as suggested by Ricardo (since also
mozilla behaves that way)

I didn't add the possibility of hiding the arrow: why is it needed? if you don't
want the arrow to show up you should not be using this widget anyway...

With regard to the _set_menu function and memory management, currently the code
works in the following way: I unref priv->menu (if not null), set priv->menu to
the new menu and then ref it... is it right?

> With regard to the _set_menu function and memory management, currently the code
> works in the following way: I unref priv->menu (if not null), set priv->menu to
> the new menu and then ref it... is it right?

I think it should probably ref/sink the menu. See for exampe
gtk_tool_item_set_proxy_menu_item() for an example of how to do that.

yes, it already does that: chpe fixed it.

Looking a bit more detailed on it, here are some comments:

- The widget needs to have keyboard navigation.

- The arrow button should not take focus on click

- Using the keyboard to pop up the menu should probably be done like in
  gtktoolbar.c

- The menu need to pop up in the correct position when the item is on
  a vertical toolbar

wrt the keyboard and focus issues, may you be a bit more specific? I haven't
looked at gtktoolbar.c yet, but by testing with the simple
testdropdowntoolbutton I can't see any difference with the behavior of the
overflow menu of the toolbar:
- the arrow button doesn't take focus when you click on it
- the arrow button can be focused using arrow keys once the focus is in the toolbar
- the arrow button can be activate pressing the space bar key once it has the focus


with regard to the menu positioning, the menu should probably pop up on the
right when the toolbar is vertical, but should also the arrow change direction
and be packed under the main button using a vbox instead of a hbox? Is that even
possible to change on the fly? How can I query the orienatation of the toolbar?
What happens if the widget hasn't been added to a toolbar yet or if it's moved
from a toolbar to another?

You are right that the arrow can be focused with the arrow keys once the focus
is on the toolbar. Sorry about that.

What I mean about the focus is that it should act like the toolbar overflow menu.
- "pop up" being tied to the activation of the button, not just random
keypresses (which might be themed differently)
- show the button depressed when the menu is up
- select first item when the menu is shown.

WRt. positioning:
  - gtk_tool_item_get_orientation() will return the orientation of the toolbar,
or a default value if the item hasn't been added to a toolbar yet.
  - GtkToolItemClass->toolbar_reconfigured will be called when something changes
about the toolbar, such as its orientation. The implementation for
GtkArrowToolButton will have to chain up to allow GtkToolButton to rebuild itself.

I think the best thing to do is to have a 'construct_contents' method, that
rebuilds the widget completely. And call that whenever something changes (such
as the orientation of the toolbar).

For the menu positioning I think the code in gtktoolbar.c
(gtk_toolbar_arrow_button_clicked, show_menu, menu_position_func) can be adapted
fairly simply.

I fixed the first 3 items (tie the menu to the toggle state and select the first
item when activating with the keyboard).


Unreated question: what is the push_in param of the GtkMenuPositionFunc? should
that be set to TRUE like I saw in gtktoolbar.c? we were ignoring it before... I
cannot see any difference. It would be nice to add docs for that param in since
it is undocumented... maybe I should file a separate bug for that.

fixed menu positioning. btw, the code in toolbar.c doesn't make sure that the
overflow menu is on screen.

Well, the menu code will clamp the menu on screen and then make it scrol, but
the result is pretty weird for the toolbar currently. For the ArrowButton it
looks like the menu will just be clamped on screen which means it will show up
covering the arrow. 

I think the right behavior in both cases is to do what the pull-down menus do:
allocate the menu *small* enough that it fits on the screen, then set the scrolling.

I haven't investigated how to actually do this, except wondering if some public
generic menu positioning function that would take care of all this would perhaps
be a useful addition to gtk+.

That's what "push_in" is about; I forget the exact details, but it's
something like that if position function sets it to to TRUE, then the calling
code will make the size of the menu smaller and add scrolling rather than than
displacing the menu to make it fit onscreen.

Generally, position functions don't need to worry about size, just
position. (GtkOptionMenu might be an example worth looking at.)

Ok, I have removed the clamping and as a matter of fact scrolling is
automagically added to the menu if ends out of the screen.


I've also implemented ssp's suggestion of connecting to the toolbar_reconfigured
signal and to reconstruct the widget contents so that on verical toolbars we
have  the arrow_button below the real button and with a right arrow.

Since I'm not 100% confident of having got everithing right I'm attaching the
last patch here for easier review.

Created attachment 31617 [details] [review]
last patch committed to libegg, for easier review

The construct_contents part of the patch looks fine, but I am not completely
happy with the menu positioning. I think that it should position the menu
similar to how normal dropdown menus (from a menubar) are positioned. Ie.,

  if there is room below the toolbutton, 
      position the menu there
  else if there is room above to toolbutton
      position the menu there
  else 
      position the menu above or below, depending on
      where there is more space. Set push_in to FALSE,
      so that it will get scroll arrows.

The toolbar overflow menu should do the same (it doesn't currently). I don't
think this particular issue should prevent the widget from going in though.

Other issues:

   - egg_dropdown_toolbutton_set_menu() should accept a NULL menu to mean 
     'no menu'. In that case the arrow button should be shown, but 
     insensitized.

   - The menu needs to be a real property 

   - I don't think a right-click on the button should pop up the menu

   - The widget should probably be renamed GtkMenuToolButton

   - egg_dropdown_tool_button_new() should take label and icon parameters

In addition I am a little nervous about the button reparenting going on in
egg_dropdown_toolbutton_init() where the GtkButton ends up being a child of an
hbox. This will break code that does 

   GTK_BIN (dropdown_button)->child

on a toolbutton and expects to be able to cast the result to a GtkButton. There
is probably not much we can do about that though.

>  else 
>      position the menu above or below, depending on
>      where there is more space. Set push_in to FALSE,
>      so that it will get scroll arrows.

as far as I can tell with push_in = TRUE you get the scroll arrows, while with
push_in = false the menu is moved to stai on screen.


>   - egg_dropdown_toolbutton_set_menu() should accept a NULL menu to mean 
>     'no menu'. In that case the arrow button should be shown, but 
>     insensitized.

agreed

>   - The menu needs to be a real property 

Ok, I saw OPtionMenu did the same. Can I ask what is the reason?

>   - I don't think a right-click on the button should pop up the menu

Not a big issue and if you prefer this way I'll remove it, but I like the
right-click behavior. For instance the real button part offers a bigger target
for the pointer, beside you can get to the menu with fewer keyboard presses
using shift+f10. Note that mozilla and firefox show the menu on right-click.

>   - The widget should probably be renamed GtkMenuToolButton

I felt dropdown_menu_tool_button a bit more descriptive, but on the other hand
it is also longer. I'm fine with the rename, anyway we should handle this when
moving to gtk+.

>   - egg_dropdown_tool_button_new() should take label and icon parameters

Umh... I know very little about bindings, but I always thought that having _new
(void) constructor was a requirement. We can probably add other constructors
like new_with_label() but I didn't want add tons of differnt constructors that
are never used in practice.

> In addition I am a little nervous about the button reparenting going on in
> egg_dropdown_toolbutton_init() where the GtkButton ends up being a child of an
> hbox. This will break code that does 
>
>   GTK_BIN (dropdown_button)->child
>
> on a toolbutton and expects to be able to cast the result to a GtkButton.
> There is probably not much we can do about that though.

I understand your concern, but I'm out of better suggestions... maybe we should
live with it and document that such a use is illegal.

Created attachment 31943 [details] [review]
menu as property patch

Here is the patch I just committed to libegg: it implements the "menu as
property" suggestion and the ability to set a NULL menu.

Since I know very little about properties an accurate review is appreciated, to
make sure I didn't forget something.

I noticed a bug in previous patch: g_object_notify was not called when setting
menu to NULL. fixed in cvs.

> as far as I can tell with push_in = TRUE you get the scroll arrows, while with
> push_in = false the menu is moved to stai on screen.

No, it's the other way around: *push_in = TRUE means, "whoever called me should
move the button on-screen". FALSE means "don't move the menu; just it where it
is, but add scroll arrows".

> >   - The menu needs to be a real property 
> Ok, I saw OPtionMenu did the same. Can I ask what is the reason?

So that it can be used by language bindings and GUI builders.

> Not a big issue and if you prefer this way I'll remove it, but I like the
> right-click behavior. For instance the real button part offers a bigger 

My reasons are:
 
 - You can't tell by looking at the widget what is going to happen, and
 - You can't use the right click for a context menu on the toolbar then.

Ideally I would like to have right-click reserved for context menus for
all widgets.

> Umh... I know very little about bindings, but I always thought that having 
> _new (void) constructor was a requirement

It's not. Bindings can use g_object_new() if they want to. See GtkToolButton for
an example.

> I understand your concern, but I'm out of better suggestions... maybe
> we should live with it and document that such a use is illegal.

Well, there are examples of such use already: people wrote this
"EphyArrowToolButton", which makes use of it ;-) We can't break Epiphany and
Galeon; if we could, it would be possible to fix the problem by having
GtkToolButton put the real button into an hbox that the arrow button could 
extend.

> Since I know very little about properties an accurate review is
> appreciated, to make sure I didn't forget something.

The patch looks good, except I think it is possible to simplify
egg_dropdown_tool_button_set_menu() by doing something like

    if (priv->menu != menu)
      {
          if (priv->menu)
             unref;

          priv->menu = menu;

          if (priv->menu)
             {
                ref/sink

                set sensitive;
             }
          else
             {
                set insensitive
             }

          g_object_notify();
     }
          
It is okay to cast a NULL pointer with GTK_MENU().

    if (priv->menu != menu)
      {
          if (priv->menu)
             unref;

          priv->menu = menu;

          if (priv->menu)
             {
                ref/sink

                set sensitive;
             }
          else
             {
                set insensitive
             }

          g_object_notify();
     }


this doesn't work: if the first time you set the menu to NULL, (priv->menu ==
menu) is true and the arrow button is not set insensitive.

ripped out right-click and changed new() to take icon and label.

Ok, I'm going to attach here the version proposed for gtk inclusion.

It contains all of your suggestion except for the menu positioning (which, as
you say, can be fixed later). In partular I still left *push_in = TRUE, because
setting it false and trying to popup the menu at the bottom of the screen gives
me the following warnings:

(testdropdowntoolbutton:8513): Gtk-WARNING **: gtk_widget_size_allocate():
attempt to allocate widget with width 64 and height -10

(testdropdowntoolbutton:8513): Gtk-WARNING **: gtk_widget_size_allocate():
attempt to allocate widget with width 64 and height -10


Some other minor issues that came to my mind, but important before putting this
in gtk are:

- is menu_activated the best name for the signal?

- should we include an api to programmatically popup the menu?

Created attachment 31970 [details]
gtkmenutoolbutton.c

Created attachment 31971 [details]
gtkmenutoolbutton.h

- Is the menu_activated signal needed at all? Couldn't people just connect
  to "show" on the menu itself?

- I am not necessarily opposed to adding a way of showing the menu 
  programmatically, but I'd rather wait until there is a proven
  need for it.

The final thing we need before it can go in is a brief overview documentation in
docs/reference/gtk/tmpl/gtkmenutoolbutton.sgml. That file should be generated
automatically if you build gtk+ with --enable-gtk-doc and gtkmenutoolbutton.[ch]
added to Makefile.am.

When we have that documentation, I think you should go ahead and commit the
widget to gtk+ CVS. There are some new bugs that should filed in connection with
that:

      - "Fix menu positioning for both toolbar and menu-toolbutton"
              Milestone: 2.6.0

      - "Simplify gtk_menu_tool_button_set_menu" 
              Milestone: 2.6.0
              I think the pseudo-code I posted will work as long as the
              initial state of the arrow button is insensitive. (As far as I
              can see that is a bug in the current code as well).

      - "Consider removing menu_activated signal"
              Milestone: 2.6 API freeze

I don't expect to be able to look at this bug for at least a week.

Ok. I alreday have the documentation (I didn't attach it to cut down the
bugzilla spam)

I'll give it a look at setting the arrow button insentive by deafult and
simplify set_menu and then I'll commit and file the bugs mentioned above.

Thanks a lot for your time and review.

IMHO, menu-activated is needed for on-deman menu building. Also maybe a program
wants to decide which one of different menus to show based on some program
state, and therefore needs to set the menu itself, not just show the previous
one? So I think a way to set the menu before it's shown is still necessary.

the gtkmenutoolbuttonwidget is now committed to gtk+ head. Including docs.

I'm closing this bug and will now open two ones as suggested by ssp (the
simplification of set menu is already solved).

I'll also remove the code from libegg and add a note saying that it's in gtk
now, it should cause any problems since the only user is epiphany head.