Re: [Gnash-dev] [PATCH] Image icon in menu

[Bastiaan Jacques]

On Thursday 09 November 2006 04:59, Hiroyuki Ikezoe wrote:
> It's just a little patch for appearance. It's added some icons to
> menu item.

I like the esthetical value this patch adds. :)

In general, I like your patch. You haven't added icons to all menu 
items; can you find suitable icons for the missing ones too?

I also have some style nits to pick, see below:

> Index: gui/gtk.cpp
> ===================================================================
> RCS file: /sources/gnash/gnash/gui/gtk.cpp,v
> retrieving revision 1.47
> diff -d -u -p -r1.47 gtk.cpp
> --- gui/gtk.cpp       5 Nov 2006 23:10:43 -0000       1.47
> +++ gui/gtk.cpp       9 Nov 2006 03:44:13 -0000
> @@ -280,47 +280,75 @@ GtkGui::createMenu()
>
>      _popup_menu = GTK_MENU(gtk_menu_new());
>
> +#if GTK_CHECK_VERSION(2,6,0)
> +    GtkMenuItem *menuitem_play =
> +
>       GTK_MENU_ITEM(gtk_image_menu_item_new_from_stock(GTK_STOCK_MEDIA_PLA
>Y, NULL)); +    GtkMenuItem *menuitem_pause =
> +
>       GTK_MENU_ITEM(gtk_image_menu_item_new_from_stock(GTK_STOCK_MEDIA_PAU
>SE, NULL)); +    GtkMenuItem *menuitem_stop =
> +
>       GTK_MENU_ITEM(gtk_image_menu_item_new_from_stock(GTK_STOCK_MEDIA_STO
>P, NULL)); +    GtkMenuItem *menuitem_restart =
> +     GTK_MENU_ITEM(gtk_image_menu_item_new_with_label("Restart
> Movie")); +    GtkWidget *restart_image =
> +        GTK_WIDGET(gtk_image_new_from_stock(GTK_STOCK_REFRESH,
> GTK_ICON_SIZE_MENU)); +    gtk_image_menu_item_set_image
> (GTK_IMAGE_MENU_ITEM(menuitem_restart), restart_image); +   
> GtkMenuItem *menuitem_step_forward =
> +     GTK_MENU_ITEM(gtk_menu_item_new_with_label("Step Forward Frame"));
> +    GtkMenuItem *menuitem_step_backward =
> +     GTK_MENU_ITEM(gtk_menu_item_new_with_label("Step Backward
> Frame")); +    GtkMenuItem *menuitem_jump_forward =
> +        GTK_MENU_ITEM(gtk_menu_item_new_with_label("Jump Forward 10
> Frames")); +    GtkMenuItem *menuitem_jump_backward =
> +     GTK_MENU_ITEM(gtk_menu_item_new_with_label("Jump Backward 10
> Frames")); +#else /* GTK_CHECK_VERSION(2,6,0) */
>      GtkMenuItem *menuitem_play =
>       GTK_MENU_ITEM(gtk_menu_item_new_with_label("Play Movie"));
> -    gtk_menu_append(_popup_menu, GTK_WIDGET(menuitem_play));
> -    gtk_widget_show(GTK_WIDGET(menuitem_play));
>      GtkMenuItem *menuitem_pause =
>       GTK_MENU_ITEM(gtk_menu_item_new_with_label("Pause Movie"));
> -    gtk_menu_append(_popup_menu, GTK_WIDGET(menuitem_pause));
> -    gtk_widget_show(GTK_WIDGET(menuitem_pause));
>      GtkMenuItem *menuitem_stop =
>       GTK_MENU_ITEM(gtk_menu_item_new_with_label("Stop Movie"));
> -    gtk_menu_append(_popup_menu, GTK_WIDGET(menuitem_stop));
> -    gtk_widget_show(GTK_WIDGET(menuitem_stop));
>      GtkMenuItem *menuitem_restart =
>       GTK_MENU_ITEM(gtk_menu_item_new_with_label("Restart Movie"));
> -    gtk_menu_append(_popup_menu, GTK_WIDGET(menuitem_restart));
> -    gtk_widget_show(GTK_WIDGET(menuitem_restart));
>      GtkMenuItem *menuitem_step_forward =
>       GTK_MENU_ITEM(gtk_menu_item_new_with_label("Step Forward Frame"));
> -    gtk_menu_append(_popup_menu, GTK_WIDGET(menuitem_step_forward));
> -    gtk_widget_show(GTK_WIDGET(menuitem_step_forward));
>      GtkMenuItem *menuitem_step_backward =
>       GTK_MENU_ITEM(gtk_menu_item_new_with_label("Step Backward
> Frame")); -    gtk_menu_append(_popup_menu,
> GTK_WIDGET(menuitem_step_backward)); -   
> gtk_widget_show(GTK_WIDGET(menuitem_step_backward));
>      GtkMenuItem *menuitem_jump_forward =
>          GTK_MENU_ITEM(gtk_menu_item_new_with_label("Jump Forward 10
> Frames")); -    gtk_menu_append(_popup_menu,
> GTK_WIDGET(menuitem_jump_forward)); -   
> gtk_widget_show(GTK_WIDGET(menuitem_jump_forward));
>      GtkMenuItem *menuitem_jump_backward =
>       GTK_MENU_ITEM(gtk_menu_item_new_with_label("Jump Backward 10
> Frames")); +#endif

Instead of defining each menu item twice, I think it would be easier to 
read this way:

  GtkMenuItem *menuitem_play =
    GTK_MENU_ITEM(gtk_menu_item_new_with_label("Play Movie"));
...

#if GTK_CHECK_VERSION(2,6,0)
  GtkWidget *play_image =
     GTK_WIDGET(gtk_image_new_from_stock(GTK_STOCK_REFRESH,
                GTK_ICON_SIZE_MENU));
  gtk_image_menu_item_set_image (GTK_IMAGE_MENU_ITEM(menuitem_play),
                                 play_image);
...
#endif

So that's like your implementation for menuitem_restart, for all menu 
items.
 
[snip]
> +
> +    // Menu for sound
>      if (get_sound_handler()) {
> +        GtkMenuItem *menuitem_sound =
> +         GTK_MENU_ITEM(gtk_check_menu_item_new_with_label("Toggle
> Sound")); gtk_menu_append(_popup_menu, GTK_WIDGET(menuitem_sound));
> +     gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(menuitem_sound),
> +                                    get_sound_handler()->is_muted() ?

You call get_sound_handler() twice; you should store the return value of 
this function to a temporary pointer to use in your call to 
gtk_check_menu_item_set_active().

Would you please address these comments and send in a new patch, if 
needed?

Bastiaan