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
=== modified file 'autogen.sh'
--- autogen.sh 2008-12-05 00:34:45 +0000
+++ autogen.sh 2009-04-05 11:03:12 +0000
@@ -1,6 +1,6 @@
1#!/bin/sh1#!/bin/sh
22
3PKG_NAME="indicator-applet"3PKG_NAME="indicator-messages"
44
5which gnome-autogen.sh || {5which gnome-autogen.sh || {
6 echo "You need gnome-common from GNOME SVN"6 echo "You need gnome-common from GNOME SVN"
77
=== modified file 'src/app-menu-item.c'
--- src/app-menu-item.c 2009-03-18 19:22:08 +0000
+++ src/app-menu-item.c 2009-04-05 11:32:26 +0000
@@ -68,7 +68,7 @@
6868
6969
7070
71G_DEFINE_TYPE (AppMenuItem, app_menu_item, GTK_TYPE_MENU_ITEM);71G_DEFINE_TYPE (AppMenuItem, app_menu_item, GTK_TYPE_IMAGE_MENU_ITEM);
7272
73static void73static void
74app_menu_item_class_init (AppMenuItemClass *klass)74app_menu_item_class_init (AppMenuItemClass *klass)
@@ -188,15 +188,24 @@
188update_label (AppMenuItem * self)188update_label (AppMenuItem * self)
189{189{
190 AppMenuItemPrivate * priv = APP_MENU_ITEM_GET_PRIVATE(self);190 AppMenuItemPrivate * priv = APP_MENU_ITEM_GET_PRIVATE(self);
191 GIcon *icon;
191192
192 if (priv->count_on_label && !priv->unreadcount < 1) {193 if (priv->count_on_label && !priv->unreadcount < 1) {
193 /* TRANSLATORS: This is the name of the program and the number of indicators. So it194 /* TRANSLATORS: This is the name of the program and the number of indicators. So it
194 would read something like "Mail Client (5)" */195 would read something like "Mail Client (5)" */
195 gchar * label = g_strdup_printf(_("%s (%d)"), g_app_info_get_name(priv->appinfo), priv->unreadcount);196 gchar * label = g_strdup_printf(_("%s (%d)"), app_menu_item_get_name(self), priv->unreadcount);
196 gtk_label_set_text(GTK_LABEL(priv->name), label);197 gtk_label_set_text(GTK_LABEL(priv->name), label);
197 g_free(label);198 g_free(label);
198 } else {199 } else {
199 gtk_label_set_text(GTK_LABEL(priv->name), g_app_info_get_name(priv->appinfo));200 gtk_label_set_text(GTK_LABEL(priv->name), app_menu_item_get_name(self));
201 }
202
203 icon = app_menu_item_get_icon(self);
204 if (icon) {
205 g_object_set(self, "image", gtk_image_new_from_gicon(icon, GTK_ICON_SIZE_MENU), NULL);
206 g_object_unref(icon);
207 } else {
208 g_object_set(self, "image", NULL, NULL);
200 }209 }
201210
202 return;211 return;
@@ -220,7 +229,7 @@
220 g_return_if_fail(priv->appinfo != NULL);229 g_return_if_fail(priv->appinfo != NULL);
221230
222 update_label(self);231 update_label(self);
223 g_signal_emit(G_OBJECT(self), signals[NAME_CHANGED], 0, g_app_info_get_name(priv->appinfo), TRUE);232 g_signal_emit(G_OBJECT(self), signals[NAME_CHANGED], 0, app_menu_item_get_name(self), TRUE);
224233
225 return;234 return;
226}235}
@@ -300,3 +309,15 @@
300 return g_app_info_get_name(priv->appinfo);309 return g_app_info_get_name(priv->appinfo);
301 }310 }
302}311}
312
313GIcon *
314app_menu_item_get_icon (AppMenuItem * appitem)
315{
316 AppMenuItemPrivate * priv = APP_MENU_ITEM_GET_PRIVATE(appitem);
317
318 if (priv->appinfo == NULL) {
319 return NULL;
320 } else {
321 return g_app_info_get_icon(priv->appinfo);
322 }
323}
303324
=== modified file 'src/app-menu-item.h'
--- src/app-menu-item.h 2009-03-15 17:19:02 +0000
+++ src/app-menu-item.h 2009-04-05 11:32:26 +0000
@@ -43,14 +43,14 @@
43typedef struct _AppMenuItemClass AppMenuItemClass;43typedef struct _AppMenuItemClass AppMenuItemClass;
4444
45struct _AppMenuItemClass {45struct _AppMenuItemClass {
46 GtkMenuItemClass parent_class;46 GtkImageMenuItemClass parent_class;
4747
48 void (* count_changed) (guint count);48 void (* count_changed) (guint count);
49 void (* name_changed) (gchar * name);49 void (* name_changed) (gchar * name);
50};50};
5151
52struct _AppMenuItem {52struct _AppMenuItem {
53 GtkMenuItem parent;53 GtkImageMenuItem parent;
54};54};
5555
56GType app_menu_item_get_type (void);56GType app_menu_item_get_type (void);
@@ -58,6 +58,7 @@
58guint app_menu_item_get_count (AppMenuItem * appitem);58guint app_menu_item_get_count (AppMenuItem * appitem);
59IndicateListenerServer * app_menu_item_get_server (AppMenuItem * appitem);59IndicateListenerServer * app_menu_item_get_server (AppMenuItem * appitem);
60const gchar * app_menu_item_get_name (AppMenuItem * appitem);60const gchar * app_menu_item_get_name (AppMenuItem * appitem);
61GIcon * app_menu_item_get_icon (AppMenuItem * appitem);
6162
62G_END_DECLS63G_END_DECLS
6364

Subscribers

People subscribed via source and target branches