Re: GDM API Change Request

[Federico Mena Quintero <federico ximian com>]

On Fri, 2006-01-27 at 11:09 -0800, Brian Cameron wrote:

[I've CCed Hans Petter as he has been hacking on GDM recently]

> Release Team:
> 
> Yesterday Sebastien Bacher proposed the following patch to GDM which
> I think would be good to add to GDM for the 2.14 release, but it affects
> API.  Since the API freeze has passed, I wanted to get permission to
> add this change.  Note that this change just adds a new feature to
> the GDM XML theme format, and doesn't break any existing interfaces
> exposed.

The change looks good to me.  I've put some comments in the patch.

> This change adds a new button type to the GDM XML format called
> "options_button" which brings up the same menu that you see when you
> hit the F10 key in the GDM greeter program.  This menu offers all
> the options that you normally see when you select the System,
> Session, and Language Buttons
> 
> This allows themes to use the new options_button so the theme only
> has a single button which allows access to all GDM options instead
> of having three buttons.  GDM still supports the three button style,
> so existing themes that use them will not be affected.
> 
> If the release team approves this change before Monday's UI freeze,
> I intend to update the three themes that get installed with GDM
> (circles, happygnome and happygnome-list) to use this new button
> instead of the three buttons (if this is okay).
> 
> Can this change go in?

Only if you update the GDM documentation to mention this new syntax,
since it's a new API.  Also, please add the "Since: 2.14" thing to it :)

> 
> Thanks,
> 
> Brian
> 
> P.S.  The patch is pretty straightforward.  The menu_position_func
>        just provides some logic to control where the menu appears so
>        it will popup where the button is placed instead of always
>        below the entry field.
> 
> === modified file 'gui/greeter/greeter_canvas_item.c'
> --- gui/greeter/greeter_canvas_item.c	
> +++ gui/greeter/greeter_canvas_item.c	
> @@ -200,6 +200,31 @@
>   	c->blue = (rgb & 0xff) * 0x101;
>   	c->pixel = 0;
>   }
> +
> +static void
> +menu_position_func (GtkMenu           *menu,
> +                    int               *x,
> +                    int               *y,
> +                    gboolean          *push_in,
> +                    GreeterItemInfo *item)
> +{
> +	 GtkAllocation rect;
> +	 GtkRequisition  requisition;
> +
> +	 rect = item->allocation;
> +	 gtk_widget_size_request (GTK_WIDGET (menu), &requisition);
> +	*x=rect.x;
> +	*y=rect.y-requisition.height-4;
> +	*push_in=TRUE;
> + }

Please put spaces around equal signs and operators.

You are moving the menu upward by its own height.  Why do you need to
move it up another 4 pixels?

> +
> + static void
> + greeter_options_handler (GreeterItemInfo *item, GtkWidget *menubar)
> + {
> +	 
> gtk_menu_popup(GTK_MENU(gtk_menu_item_get_submenu(gtk_container_get_children(GTK_CONTAINER(menubar))->data)), 
> NULL, NULL,
> +			(GtkMenuPositionFunc)menu_position_func,
> +			item, 0, gtk_get_current_event_time());
> + }

This is horrible.  Can't you just use this instead:

  gtk_menu_shell_activate_item (menubar, first_menu_item_in_menubar, FALSE);

... or do you do that precisely because you need a position_func?  What
happens if you don't use a position_func?

> 
>   void
>   greeter_item_create_canvas_item (GreeterItemInfo *item)
> @@ -396,6 +421,9 @@
>   				   "width", (double)rect.width,
>   				   NULL);
> 
> +	    greeter_item_register_action_callback ("options_button",
> +						   (ActionFunc)greeter_options_handler,
> +						   menubar);
>   	    /* Here add a tooltip, so that the user knows about F10 */
>   	    tooltips = gtk_tooltips_new ();
>   	    gtk_tooltips_set_tip (tooltips, GTK_WIDGET (entry),
> 
> === modified file 'gui/greeter/greeter_parser.c'
> --- gui/greeter/greeter_parser.c	
> +++ gui/greeter/greeter_parser.c	
> @@ -1094,6 +1094,11 @@
>           {
>   	  g_free (*translated_text);
>   	  *translated_text = g_strdup (_("_Configure"));
> +	}
> +      else if (g_ascii_strcasecmp ((char *) prop, "options") == 0)
> +        {
> +	  g_free (*translated_text);
> +	  *translated_text = g_strdup (_("_Options"));
>   	}
>         else if (g_ascii_strcasecmp ((char *) prop, "caps-lock-warning") 
> == 0)
>           {
> _______________________________________________
> release-team mailing list
> release-team gnome org
> http://mail.gnome.org/mailman/listinfo/release-team
> 
-- 
Federico Mena Quintero <federico ximian com>