Skip to content

Commit

Permalink
rename 'mate_image_menu_item...' to 'eel_image_menu_item_new_from_icon'
Browse files Browse the repository at this point in the history
  • Loading branch information
@sc0w
sc0w committed Mar 26, 2018
1 parent ffc9106 commit 8bebf06
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 31 deletions.
32 changes: 5 additions & 27 deletions eel/eel-editable-label.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
#include "eel-editable-label.h"
#include "eel-marshal.h"
#include "eel-accessibility.h"
#include "eel-gtk-extensions.h"

#include <libgail-util/gailmisc.h>

#include <glib/gi18n-lib.h>
Expand Down Expand Up @@ -3003,30 +3005,6 @@ activate_cb (GtkWidget *menuitem,
g_signal_emit_by_name (label, signal);
}

static GtkWidget
*mate_image_menu_item_new_from_icon (const gchar *icon_name,
const gchar *label_name)
{
GtkWidget *icon;
GtkWidget *box = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 6);

if (icon_name)
icon = gtk_image_new_from_icon_name (icon_name, GTK_ICON_SIZE_MENU);
else
icon = gtk_image_new ();

GtkWidget *label_menu = gtk_label_new_with_mnemonic (g_strconcat (label_name, " ", NULL));
GtkWidget *menuitem = gtk_menu_item_new ();

gtk_container_add (GTK_CONTAINER (box), icon);
gtk_container_add (GTK_CONTAINER (box), label_menu);

gtk_container_add (GTK_CONTAINER (menuitem), box);
gtk_widget_show_all (menuitem);

return menuitem;
}

static void
append_action_signal (EelEditableLabel *label,
GtkWidget *menu,
Expand All @@ -3035,7 +3013,7 @@ append_action_signal (EelEditableLabel *label,
const gchar *signal,
gboolean sensitive)
{
GtkWidget *menuitem = mate_image_menu_item_new_from_icon (icon_name, label_name);
GtkWidget *menuitem = eel_image_menu_item_new_from_icon (icon_name, label_name);

g_object_set_data (G_OBJECT (menuitem), "gtk-signal", (char *)signal);
g_signal_connect (menuitem, "activate",
Expand Down Expand Up @@ -3139,7 +3117,7 @@ popup_targets_received (GtkClipboard *clipboard,
append_action_signal (label, label->popup_menu, "edit-paste", _("_Paste"), "paste_clipboard",
clipboard_contains_text);

menuitem = mate_image_menu_item_new_from_icon ("edit-select-all", _("Select All"));
menuitem = eel_image_menu_item_new_from_icon ("edit-select-all", _("Select All"));
g_signal_connect_object (menuitem, "activate",
G_CALLBACK (eel_editable_label_select_all), label,
G_CONNECT_SWAPPED);
Expand All @@ -3150,7 +3128,7 @@ popup_targets_received (GtkClipboard *clipboard,
gtk_widget_show (menuitem);
gtk_menu_shell_append (GTK_MENU_SHELL (label->popup_menu), menuitem);

menuitem = mate_image_menu_item_new_from_icon (NULL, _("Input Methods"));
menuitem = eel_image_menu_item_new_from_icon (NULL, _("Input Methods"));
gtk_widget_show (menuitem);
submenu = gtk_menu_new ();
gtk_menu_item_set_submenu (GTK_MENU_ITEM (menuitem), submenu);
Expand Down
24 changes: 24 additions & 0 deletions eel/eel-gtk-extensions.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,3 +456,27 @@ eel_gtk_message_dialog_set_details_label (GtkMessageDialog *dialog,
gtk_widget_show (label);
gtk_widget_show (expander);
}

GtkWidget *
eel_image_menu_item_new_from_icon (const gchar *icon_name,
const gchar *label_name)
{
GtkWidget *icon;
GtkWidget *box = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 6);

if (icon_name)
icon = gtk_image_new_from_icon_name (icon_name, GTK_ICON_SIZE_MENU);
else
icon = gtk_image_new ();

GtkWidget *label_menu = gtk_label_new_with_mnemonic (g_strconcat (label_name, " ", NULL));
GtkWidget *menuitem = gtk_menu_item_new ();

gtk_container_add (GTK_CONTAINER (box), icon);
gtk_container_add (GTK_CONTAINER (box), label_menu);

gtk_container_add (GTK_CONTAINER (menuitem), box);
gtk_widget_show_all (menuitem);

return menuitem;
}
11 changes: 7 additions & 4 deletions eel/eel-gtk-extensions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,14 @@ GtkWidget * eel_gtk_menu_tool_button_get_button (GtkMenuTo
void eel_gtk_label_make_bold (GtkLabel *label);

/* GtkTreeView */
void eel_gtk_tree_view_set_activate_on_single_click (GtkTreeView *tree_view,
gboolean should_activate);
void eel_gtk_tree_view_set_activate_on_single_click (GtkTreeView *tree_view,
gboolean should_activate);

/* GtkMessageDialog */
void eel_gtk_message_dialog_set_details_label (GtkMessageDialog *dialog,
const gchar *details_text);
void eel_gtk_message_dialog_set_details_label (GtkMessageDialog *dialog,
const gchar *details_text);

GtkWidget * eel_image_menu_item_new_from_icon (const gchar *icon_name,
const gchar *label_name);

#endif /* EEL_GTK_EXTENSIONS_H */

8 comments on commit 8bebf06

@TomaszGasior
Copy link

@TomaszGasior TomaszGasior commented on 8bebf06 Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bad idea in my opinion. Just don't use icons in menu items. Or if icons are needed, place GtkImageMenuItem replacement in MATE's shared library to share it with other MATE's apps and components.

Also, with current implementation it's impossible to disable menu items icons.

@lukefromdc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, we are NOT going to stop using icons in menus. Lots of people objected when GNOME tried to disable them unconditionally in an early version of GTK 3.10 and GNOME then changed that to deprecated rather than removed.

We are not GNOME, and the GNOME HIG ("Human Interface Guidelines") for GNOME 3 play no role in our decisions.

@lukefromdc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into making the icon loading conditional on user having menus in icons turned on later today

@TomaszGasior
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You misunderstood my suggestion. I didn't want to say that MATE should follow GNOME's HIG. But dropping menu items has some adventages.

  1. You don't have to maintain reimplementation of GtkImageMenuItem. Just placing image before label is not good solution: you have to add ability to disable icon, you have to deal with margins and various themes, you have to maintain separate library for it or separate copies of code.

  2. Consistence. GNOME applications (like gnome-disks or dconf-editor), Firefox, nm-applet and a lot of apps are using plain menu items without icon. When MATE will continue to use icons for every menu items, it will be inconsistent with other world. Sorry, this is the reality — when GNOME is saying that menu icons are bad, a lot of apps is going to remove it.

@lukefromdc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No way in the world we are going to drop icons in menu item, no way at all. I am so opposed to that I would locally revert it in my own builds if others wanted to go in that direction, which is very unlikely here anyway.

@lukefromdc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GNOME can say what they want, other apps can do what they want. We do NOT have to obey. Mate apps are not intended to look like current GNOME apps anyway so consistancy is not an issue.

@lukefromdc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1021 makes loading icons in eel-editable-label right click menus optional, they follow the "menus-have-icons" gsettings preference and seem to work fine here dynamically, that is without having to restart Caja

@lukefromdc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1021 has been merged,menu icons now show when and only when selected by the user(as is our default in MATE)

Please sign in to comment.