Created attachment 63765 [details]
GtkRecentAction API

GtkRecentAction header file

Created attachment 63766 [details]
GtkRecentAction implementation

Created attachment 63767 [details] [review]
GtkRecentAction glue

build glue for GtkRecentAction, plus apidoc, tests and gtk-demo update

Does this API allow the icon to be overridden by the application? This would give the application a chance to display a thumbnail for recently used image files.

Sorry that I am just asking this question instead of digging thru the code you attached, But I hope it will be simpler for you to answer.

(comment #4)

> Does this API allow the icon to be overridden by the application?

nope. the icon is handled by the GtkRecentInfo structure, which retrieves it from the icon theme using the icon for the mime type of the document, as saved by the application that has registered it; I had code that hooked up the recent files code with the fd.o thumbnailing support inside libegg, but I didn't commit it as it required a bunch of other stuff to get inside glib and gtk (see libegg/md5 and libegg/pixbufthumbnailer).

also, the GtkAction API does not allow anything else than stock icons unless you create your own proxy widgets.

you could create a custom recent chooser implementation using that code, until gtk gets support for the fd.o thumbnailing spec.

Looks reasonable as a first cut. 
Do we need extra support for adding a recent menu to the open toolbar
button via the ui manager, like gedit does ?

The ui manager documentation for menus in toolbars currently says:

Note that toolitem elements may contain a menu element, but only
if their associated action specifies a #GtkMenuToolButton as proxy.

Ping, did we get the menutoolbutton thing worked out ?

I'm not sure what the best course of action would be: advertise in the docs *not* to add stuff inside a toolbar item bound to a recent action or to leave the menu toolbutton stuff out.

any suggestions?

so, for a recent files submenu, the xml would be:

<menu name="file" action="some-action">
  ...
  <menu name="recent" action="recent-action" />
</menu>

is that right ?

the currently supported syntax for menutoolitems is

<toolbar>
  ...
  <toolitem name="foo" action="some-action">
    <menu name="bar" action="some-other-action">
      ...
    </menu>
</toolbar>

I would propose to combine these two and support

<toolbar>
  ...
  <toolitem name="foo" action="some-action">
    <menu name="bar" action="recent-action"/>
  </toolitem>
</toolbar>

for a toolitem with a recent-files submenu. Here, some-action
must return a GtkMenuToolItem as proxy, and recent-action must
be a GtkRecentAction.

Does that sound reasonable ?

> Does that sound reasonable ?

yes, it does.  I'll document the behaviour inside the reference.

I'll also make GtkRecentAction implement the GtkRecentChooser interface and act as a proxy to the GtkRecentChooserMenu it creates.

after poking with it some more, I think I'm completely stuck because I don't understand how the UIManager builds the widgets.

I can make this work:

  <menu name="FileMenu" action="FileAction">
    <menuitem name="FileNew" action="FileNewAction"/>
    <menuitem name="FileOpen" action="FileOpenAction"/>
    <menuitem name="FileOpenRecent" action="FileOpenRecentAction"/>
    ...
  </menu>

by making GtkRecentAction return a GTK_TYPE_IMAGE_MENU_ITEM to which I attach a GtkRecentChooserMenu using gtk_menu_item_set_submenu(), inside the overridden GtkAction::create_menu_item vfunc.

I don' understand how to do this:

  <menu name="FileMenu" action="FileAction">
    <menuitem name="FileNew" action="FileNewAction"/>
    <menuitem name="FileOpen" action="FileOpenAction"/>
    <menu name="FileOpenRecent" action="FileOpenRecentAction"/>
    ...
  </menu>

which would work with the toolbar.

Created attachment 68029 [details] [review]
a simple patch

Here is a hacky way to make it work

To be more precise, my patch makes

<toolbar>
  <toolitem name="foo" action="foo-action">
    <menu name="recent" action=recent-action"/>
  </toolitem>
</toolbar>

work as expected

One way to make it appear slightly less hacky would be  to not special-case
GtkRecentAction, but whenever a menu is needed, and the action provides a 
menu item proxy with a submenu, use the submenu instead of creating our own.

adding a class property inside GtkActionClass, like:

struct _GtkActionClass
{
  GObjectClass parent_class;

  /* activation signal */
  void       (* activate)           (GtkAction    *action);

  GType      menu_item_type;
  GType      toolbar_item_type;
+ guint      toolbar_item_has_submenu : 1;
+
  /* widget creation routines (not signals) */
  GtkWidget *(* create_menu_item)   (GtkAction *action);
  GtkWidget *(* create_tool_item)   (GtkAction *action);
  void       (* connect_proxy)      (GtkAction *action,
				     GtkWidget *proxy);
  void       (* disconnect_proxy)   (GtkAction *action,
				     GtkWidget *proxy);

  /* Padding for future expansion */
  void (*_gtk_reserved1) (void);
  void (*_gtk_reserved2) (void);
  void (*_gtk_reserved3) (void);
- void (*_gtk_reserved4) (void);
};

so that the check in the patch above would look like this:

+	    if (NODE_INFO (node->parent)->type == NODE_TYPE_TOOLITEM &&
+		GTK_ACTION_GET_CLASS (action)->toolbar_item_has_submenu)

which should be slightly less hacky.

Neatpicking, shouldn't you add toolbar_item_has_submenu after all current fields?

okay, let's get the ball rolling again.

here's two patches: the first one is a simple clean up, moving some code out from the current GtkRecentChooser implementations, as it reduces code duplication; this patch might go in independently from GtkRecentAction.

the second patch add GtkRecentAction to the build:

 * GtkRecentAction implements GtkRecentChooserIface, so I needed to relax the interface prerequisite (from GtkObject to GObject);
 * GtkRecentActionEntry struct was removed from the GtkActionGroup API as usually you just need a single action in your UI; adding it back wouldn't be a problem, though;
 * added a flag to GtkActionClass, "provides_menu": if a subclass of GtkAction provides a submenu attached to the menu item or the toolbar item, then GtkUIManager will use that submenu instead of an empty menu when doing:
  <toolitem action='openaction'>
    <menu action='recentaction'/>
  </toolitem>

bits still missing: documentation of the GtkRecentAction public API

Created attachment 84496 [details] [review]
clean up patch

Created attachment 84497 [details] [review]
GtkRecentAction

implementation and build glue, with test

I think you need to remove one of the padding function pointers to keep the class size when adding the provides_menu flag

Why is show_numbers exposed as a property on the action, and 
+  guint show_private   : 1;
+  guint show_not_found : 1;
+  guint show_tips      : 1;
+  guint show_icons     : 1;
+  guint local_only     : 1;
are not ?

(In reply to comment #20)
> I think you need to remove one of the padding function pointers to keep the
> class size when adding the provides_menu flag

yeah, it slipped through.

> Why is show_numbers exposed as a property on the action, and 
> +  guint show_private   : 1;
> +  guint show_not_found : 1;
> +  guint show_tips      : 1;
> +  guint show_icons     : 1;
> +  guint local_only     : 1;
> are not ?

because these properties are defined by the GtkRecentChooser interface and overridden inside the implementations of that interface using the _gtk_recent_chooser_install_properties() function.  show-numbers is implemented only by GtkRecentChooserMenu (and GtkRecentAction, now).

committed the clean up patch, I'll attach the latest diff for the recent action alone.

Created attachment 84564 [details] [review]
GtkRecentAction

new iteration of GtkRecentAction:

* documentation added
* removed a slot to make room for GtkActionClass::provides_menu

Created attachment 84580 [details] [review]
GtkRecentActionEntry support

this patch should be applied on top the previous one; it creates a new GtkRecentActionEntry to be used with GtkActionGroup. as I said, you'll commonly have just a single GtkRecentAction object around, so it doesn't really make much sense. I've added it just for sake of completeness.

Since, as you said, it would be pretty useless, I don't think we need it.

About the provides_menu flag, would it be nicer to make that a vfunc ?
That would keep the possibility open to let the value depend on the instance.

Any particular reason why some of the static functions in gtkrecentaction.c are
inline ?

(In reply to comment #25)

> About the provides_menu flag, would it be nicer to make that a vfunc ?
> That would keep the possibility open to let the value depend on the instance.

yeah, it 
 
> Any particular reason why some of the static functions in gtkrecentaction.c are
> inline ?

probably left overs from previous code. recent_chooser_set_property() in particular should be moved into gtk_recent_action_set_property() anyway.

should I also add a gtk_action_provides_menu() function, invoking the class vfunc?

(In reply to comment #26)

> > About the provides_menu flag, would it be nicer to make that a vfunc ?
> > That would keep the possibility open to let the value depend on the instance.
> 
> yeah, it 

damned keyboard; what I meant was:

  yeah, it makes sense to have it as a vfunc.  it could also make sense to
have a GtkWidget * return value, instead of a boolean flag, so we don't need to call gtk_action_create_menu_item(), steal the submenu and destroy the widget inside the UIManager code.  having something like:

  GtkWidget *(* provides_menu) (GtkAction *action);

return NULL by default, and the GtkMenu widget otherwise.

sounds like a good plan. I'd say add the wrapper, since we have wrappers for create_menu_item/create_tool_item too.

Created attachment 84661 [details] [review]
GtkRecentAction

new iteration, with the GtkAction::provides_menu() vfunc returning a GtkWidget instead of a boolean.

I have to admit that I think the name provides_menu did fit much better when this was still a boolean. Maybe get_menu or get_submenu ?


+            if (NODE_INFO (node->parent)->type == NODE_TYPE_TOOLITEM &&
+                GTK_ACTION_GET_CLASS (action)->provides_menu)
+              {
+                menu = gtk_action_provides_menu (action);
+              }
+            else
+              {
+                GtkWidget *tearoff;
+                GtkWidget *filler;

I'm wondering about the "else" here. What if provides_menu() returns NULL ?
Should the else be "if (menu == NULL)" instead ?

(In reply to comment #31)
> I have to admit that I think the name provides_menu did fit much better when
> this was still a boolean. Maybe get_menu or get_submenu ?

get_submenu() might be more descriptive, as we are asking for the submenu of a menuitem/toolitem.
 
> +            if (NODE_INFO (node->parent)->type == NODE_TYPE_TOOLITEM &&
> +                GTK_ACTION_GET_CLASS (action)->provides_menu)
> +              {
> +                menu = gtk_action_provides_menu (action);
> +              }
> +            else
> +              {
> +                GtkWidget *tearoff;
> +                GtkWidget *filler;
> 
> I'm wondering about the "else" here. What if provides_menu() returns NULL ?
> Should the else be "if (menu == NULL)" instead ?

you're right, of course.

new patch attached.

Created attachment 84668 [details] [review]
GtkRecentAction

Looks good now, please commit

committed in trunk.  thanks Matthias for the fast review.

2007-03-15  Emmanuele Bassi  <ebassi@gnome.org>

        * gtk/gtkaction.[ch]: Add GtkActionClass::get_submenu() vfunc:
        actions providing a menu item or a menu tool button with already
        a submenu should return the GtkMenu widget.

        * gtk/gtkuimanager.c (update_node): If an action provides its
        own submenu, use it instead of adding an empty one

        * gtk/gtkrecentaction.[ch]: Add GtkRecentAction, an action
        implementing the GtkRecentChooser interface for displaying the
        list of recently used files into menus and toolbars generated
        using GtkUIManager. (#338843)

        * gtk/Makefile.am:
        * gtk/gtk.h:
        * gtk/gtk.symbols: Add GtkRecentAction API to the build.

        * tests/testactions.c: Exercise the GtkRecentAction API.

I don't think this patch should stay as-is. The new vfunc creates
a menu, so why is it called get_submenu() then? It should be
called create_menu() to be consistent with create_menu_item() and
create_tool_item(). Also the C function should be immediately
after gtk_action_create_tool_item() IMHO.

I also see no support for using the new action in a menu, it only
seems to work as child node of a tool item.

(In reply to comment #36)
> I don't think this patch should stay as-is. The new vfunc creates
> a menu, so why is it called get_submenu() then? It should be
> called create_menu() to be consistent with create_menu_item() and
> create_tool_item().

get_submenu() gets the menu set by the action as an implicit submenu of the menu item and the tool item. create_menu() might be more consistent, but it's not really explicit about the use case.

> Also the C function should be immediately
> after gtk_action_create_tool_item() IMHO.
> 
> I also see no support for using the new action in a menu, it only
> seems to work as child node of a tool item.

this can be added.

I think this interpretation of get_submenu() is wrong. An action is
supposed to be a *model* that can have many views, so returning always
the same menu would be wrong.

Created attachment 84710 [details] [review]
patch renaming GtkAction::get_submenu to GtkAction::create_menu

as per IRC chat with mitch, s/get_submenu/create_menu/

also, the UIManager will get the menu from the action for both toolitem and menuitem nodes.

Fine with me.

committed in trunk.

2007-03-16  Emmanuele Bassi  <ebassi@gnome.org>

        * gtk/gtkaction.h:
        * gtk/gtkaction.c: Rename get_submenu() to create_menu();
        rename gtk_action_get_submenu() to gtk_action_create_menu().

        * gtk/gtkrecentaction.c: Update for GtkAction change.

        * gtk/gtkuimanager.c (update_node): Update for GtkAction change;
        also, use the menu from the GtkAction for both menuitem and
        toolitem nodes.