Indicator Applet

Merge lp:~james-w/indicator-applet/messages-icons into lp:indicator-applet/messages0.2

  1. messages-icons
  2. Merge into messages
Proposed by James Westby
Status: Rejected
Rejected by: Mark Shuttleworth
Proposed branch: lp:~james-w/indicator-applet/messages-icons
Merge into: lp:indicator-applet/messages0.2
Diff against target: None lines
To merge this branch: bzr merge lp:~james-w/indicator-applet/messages-icons
Reviewer Review Type Date Requested Status
Mark Shuttleworth (community) Disapprove
Matthew Paul Thomas (community) ux Disapprove
Review via email: mp+5229@code.launchpad.net
To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote :

Hi,

This change adds images to the servers in indicator-messages
based on the icon from the desktop file if available.

They are not in the drawing at https://wiki.ubuntu.com/MessagingMenu
but we have the information, and I think it looks good and helps
quickly identify the app you are looking for, and ties in the
servers to the main menu entries, tasklist entries and title bar
icon.

Thanks,

James

Revision history for this message
James Westby (james-w) wrote :

I've just pushed a new revision that fixes the icon disappearing
due to it's refcount dropping.

Thanks,

James

Revision history for this message
Matthew Paul Thomas (mpt) wrote :

Thanks for the contribution, but I'd rather we didn't do this, for two reasons. First, the applications are generally not as important as the actual message sources, and giving icons only to the latter makes them more visible. Second, without an icon the application name items subtly double as headings for the application sections, and they'd lose that effect if they had icons too.

I've updated the spec to clarify this. <https://wiki.ubuntu.com/MessagingMenu?action=diff&rev2=10&rev1=8> (BTW, whenever I include -- or omit -- something in a mockup, there's usually a precise reason, so feel free to ask if I haven't documented it anywhere.)

review: Disapprove (ux)
Revision history for this message
James Westby (james-w) wrote :

On Tue, 2009-04-07 at 12:15 +0000, Matthew Paul Thomas wrote:
> Review: Disapprove ux
> Thanks for the contribution, but I'd rather we didn't do this, for two reasons.

Fair enough.

> Second, without an icon the application name items subtly double as headings
> for the application sections, and they'd lose that effect if they had icons too.

There is an indent to the messages that also makes the application name
appear as a header.

Also, when there are no messages without icons I don't think it looks
very attractive, maybe something else can be done to alleviate that, or
perhaps you disagree.

> I've updated the spec to clarify this. <https://wiki.ubuntu.com/MessagingMenu?action=diff&rev2=10&rev1=8>
> (BTW, whenever I include -- or omit -- something in a mockup, there's usually a precise reason,
> so feel free to ask if I haven't documented it anywhere.)

I appreciate that, but I disagreed, and putting the patch together took
little time and allowed me to show what it would look like.

Thanks,

James

Revision history for this message
Mark Shuttleworth (sabdfl) wrote :

I'm interested to see a mockup. James, this is cool stuff, and I don't
want to discourage you! It might be more efficient to whip up a mockup
in the Gimp or Inkscape for review before diving to code.

Mark

Revision history for this message
James Westby (james-w) wrote :

>
> I'm interested to see a mockup. James, this is cool stuff, and I don't
> want to discourage you! It might be more efficient to whip up a mockup
> in the Gimp or Inkscape for review before diving to code.

Ah,

I never included a reference to a little video I made here, sorry.

  http://jameswestby.net/images/empathy-indicator.ogv

As for whether this was better to code or mockup, by the time I had
dug far enough in to the code to see that this wasn't implemented
rather than just buggy as I initially suspected I could see how to
do the work, not to mention that I am far more adept at coding
than Gimp or Inkscape.

Thanks,

James

Revision history for this message
Mark Shuttleworth (sabdfl) wrote :

James Westby wrote:
> I never included a reference to a little video I made here, sorry.
>
> http://jameswestby.net/images/empathy-indicator.ogv
>
 Looking at that, I think the current spec is correct, -1 to this change.

 review -1
 status rejected

The icons create unnecessary clutter. Our general policy is to decrease,
not increase the number of icons in menus. We make some exceptions to
this, but they have to be carefully justified and debated.

> As for whether this was better to code or mockup, by the time I had
> dug far enough in to the code to see that this wasn't implemented
> rather than just buggy as I initially suspected I could see how to
> do the work, not to mention that I am far more adept at coding
> than Gimp or Inkscape.
>
You can do better than this, James. I don't want to discourage you, but
this was not a very smart response or approach. There are other areas
that require your coding attention, I think primarily of a specific
project assigned to you from me in person. Digging in first is a
kneejerk reaction - if you thought it was a bug, file it first to see
what conversations it triggers.

Please take an afternoon to learn enough Gimp or Inkscape to be able to
produce that mockup in 20 minutes. They are both great pieces of free
software, and will help you to participate in the design conversations
productively.

If you think I'm out of line, think again carefully. I want you to
participate, but I have every right to ask you to do so effectively, and
you're missing a trick here.

Mark

review: Disapprove
Revision history for this message
Andrew M (andrew-miniatureworldmaker) wrote :

James, another option which i personally find useful is to just make the change in the code (as you have done) in your local repo. Take a screen shot and submit the screen shot. Then if the patch is excepted, you can push the code or make the required changes.

I don't think using GIMP / Inkscape is necessarily the fastest way to do things however it allows non-technical people to collaborate.

Hope this helps.

Unmerged revisions

99. By James Westby

Add images to the messaging menu.

Use GtkImageMenuItem as the parent, and then set the image from the
desktop file icon if available.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'autogen.sh'
2--- autogen.sh 2008-12-05 00:34:45 +0000
3+++ autogen.sh 2009-04-05 11:03:12 +0000
4@@ -1,6 +1,6 @@
5 #!/bin/sh
6
7-PKG_NAME="indicator-applet"
8+PKG_NAME="indicator-messages"
9
10 which gnome-autogen.sh || {
11 echo "You need gnome-common from GNOME SVN"
12
13=== modified file 'src/app-menu-item.c'
14--- src/app-menu-item.c 2009-03-18 19:22:08 +0000
15+++ src/app-menu-item.c 2009-04-05 11:32:26 +0000
16@@ -68,7 +68,7 @@
17
18
19
20-G_DEFINE_TYPE (AppMenuItem, app_menu_item, GTK_TYPE_MENU_ITEM);
21+G_DEFINE_TYPE (AppMenuItem, app_menu_item, GTK_TYPE_IMAGE_MENU_ITEM);
22
23 static void
24 app_menu_item_class_init (AppMenuItemClass *klass)
25@@ -188,15 +188,24 @@
26 update_label (AppMenuItem * self)
27 {
28 AppMenuItemPrivate * priv = APP_MENU_ITEM_GET_PRIVATE(self);
29+ GIcon *icon;
30
31 if (priv->count_on_label && !priv->unreadcount < 1) {
32 /* TRANSLATORS: This is the name of the program and the number of indicators. So it
33 would read something like "Mail Client (5)" */
34- gchar * label = g_strdup_printf(_("%s (%d)"), g_app_info_get_name(priv->appinfo), priv->unreadcount);
35+ gchar * label = g_strdup_printf(_("%s (%d)"), app_menu_item_get_name(self), priv->unreadcount);
36 gtk_label_set_text(GTK_LABEL(priv->name), label);
37 g_free(label);
38 } else {
39- gtk_label_set_text(GTK_LABEL(priv->name), g_app_info_get_name(priv->appinfo));
40+ gtk_label_set_text(GTK_LABEL(priv->name), app_menu_item_get_name(self));
41+ }
42+
43+ icon = app_menu_item_get_icon(self);
44+ if (icon) {
45+ g_object_set(self, "image", gtk_image_new_from_gicon(icon, GTK_ICON_SIZE_MENU), NULL);
46+ g_object_unref(icon);
47+ } else {
48+ g_object_set(self, "image", NULL, NULL);
49 }
50
51 return;
52@@ -220,7 +229,7 @@
53 g_return_if_fail(priv->appinfo != NULL);
54
55 update_label(self);
56- g_signal_emit(G_OBJECT(self), signals[NAME_CHANGED], 0, g_app_info_get_name(priv->appinfo), TRUE);
57+ g_signal_emit(G_OBJECT(self), signals[NAME_CHANGED], 0, app_menu_item_get_name(self), TRUE);
58
59 return;
60 }
61@@ -300,3 +309,15 @@
62 return g_app_info_get_name(priv->appinfo);
63 }
64 }
65+
66+GIcon *
67+app_menu_item_get_icon (AppMenuItem * appitem)
68+{
69+ AppMenuItemPrivate * priv = APP_MENU_ITEM_GET_PRIVATE(appitem);
70+
71+ if (priv->appinfo == NULL) {
72+ return NULL;
73+ } else {
74+ return g_app_info_get_icon(priv->appinfo);
75+ }
76+}
77
78=== modified file 'src/app-menu-item.h'
79--- src/app-menu-item.h 2009-03-15 17:19:02 +0000
80+++ src/app-menu-item.h 2009-04-05 11:32:26 +0000
81@@ -43,14 +43,14 @@
82 typedef struct _AppMenuItemClass AppMenuItemClass;
83
84 struct _AppMenuItemClass {
85- GtkMenuItemClass parent_class;
86+ GtkImageMenuItemClass parent_class;
87
88 void (* count_changed) (guint count);
89 void (* name_changed) (gchar * name);
90 };
91
92 struct _AppMenuItem {
93- GtkMenuItem parent;
94+ GtkImageMenuItem parent;
95 };
96
97 GType app_menu_item_get_type (void);
98@@ -58,6 +58,7 @@
99 guint app_menu_item_get_count (AppMenuItem * appitem);
100 IndicateListenerServer * app_menu_item_get_server (AppMenuItem * appitem);
101 const gchar * app_menu_item_get_name (AppMenuItem * appitem);
102+GIcon * app_menu_item_get_icon (AppMenuItem * appitem);
103
104 G_END_DECLS
105

Subscribers

People subscribed via source and target branches