Re: [evolution-patches] Mailer bug #127516 Panel notification applet for new mail (bounties)
- From: Not Zed <notzed ximian com>
- To: Miguel Angel Lopez Hernandez <miguel gulev org mx>
- Cc: evolution-patches ximian com
- Subject: Re: [evolution-patches] Mailer bug #127516 Panel notification applet for new mail (bounties)
- Date: Tue, 13 Jan 2004 07:18:44 +1030
Thanks for the patch, this is a much-needed feature.
However, this is all a bit late, so it might not get in to 2.0, in
future i
suggest you submit more incremental patches as you're working, might
particularly save effort if you've gone totally off track (though this
seems ok). Actually the patch is incomplete anyway so it wont make
2.0 ui/feature freeze ...
The main issue i'd have with the ui is that you:
- extract the folder name from the uri, which is 'messy' and more
machine-friendly than human
- have confusing array of settings, is it really necessary to have a
different sound for different folders, etc. The options seems a bit
strange, the ui is a bit confusing.
And now for the patches ...
> eggmarshalers.c
> eggmarshalers.h
Don't include these, they are generated files, which may be glib
specific.
Setup the makefile to generate it like (or use) e-util-marshal.list.
> eggtrayicon.c
> eggtrayicon.h
You have a diff for these, don't need to include the source as well.
> em-panel-applet.c
> /* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 8 c-style:
"K&R" -*- */
This needs a copyright/license header.
And i think it should be copyright 2004 Ximian (or Novell), as that is
the whole point of the copyright assignment we're asking people for
(have you submitted a copyright assignment form?).
> #include <gtk/gtk.h>
> #include <gnome.h>
> #include <gconf/gconf-client.h>
> #include "e-util/eggtrayicon.h"
> #include "e-util/eggtraymanager.h"
> #include <libxml/parser.h>
> #include <libxml/tree.h>
> #include <libxml/xmlmemory.h>
> #include "em-folder-tree.h"
> #include "em-folder-selector.h"
> #include "em-panel-applet.h"
> #include "em-mailer-prefs.h"
>
> static gboolean em_tray_blink_icon (EvolutionTray *evo_tray);
> static void em_tray_update_status (EvolutionTray *evo_tray);
> static gboolean em_tray_folder_registered (gchar *uri);
> static FolderTrayNotify * em_tray_get_folder_info (gchar *uri);
> static FolderTrayNotify * em_tray_get_folders_from_xml (const char
*xml);
> static gboolean em_tray_icon_press (GtkWidget *widget, GdkEventButton
*event, EvolutionTray *evo_tray);
> static void em_tray_about (GtkMenuItem *menuitem, gpointer user_data);
> static GdkPixbuf * em_tray_blank_icon ();
> static void em_tray_create_ui (EvolutionTray *evo_tray);
>
> static EvolutionTray *evo_tray;
> static GtkWidget *image, *evbox;
Do these really need to be global?
> static gint notify_type;
This should be part of the tray.
> static GdkPixbuf *icon;
Is this left over? A couple of references but they look like they
should be tray->icon.
> static gchar *show_uri;
What is this for, it's only ever assigned.
> static gboolean
> em_tray_blink_icon (EvolutionTray *evo_tray)
> {
> evo_tray->blink = !evo_tray->blink;
>
> if (egg_tray_manager_check_running (evo_tray->current_screen))
{
> if (evo_tray->newmail &&
egg_tray_manager_check_running (evo_tray->current_screen)) {
> if (evo_tray->blink)
> gtk_image_set_from_pixbuf (GTK_IMAGE
(image), evo_tray->blank_icon);
> else
> gtk_image_set_from_pixbuf (GTK_IMAGE
(image), icon);
>
> return TRUE;
> } else {
> gtk_image_set_from_pixbuf (GTK_IMAGE (image),
evo_tray->icon);
You use a mix of evo_tray->icon and icon?
> {
> xmlNodePtr node;
> xmlDocPtr doc;
> FolderTrayNotify *folder;
>
>
> if (!(doc = xmlParseDoc ((char*) xml)))
> return NULL;
>
> node = doc->children;
> if (strcmp (node->name, "folder") != 0) {
> xmlFreeDoc (doc);
> return NULL;
> }
>
> folder = g_new0 (FolderTrayNotify, 1);
>
> folder->id = g_strdup (xmlGetProp (node, "id"));
>
> for (node = node->children; node; node = node->next) {
> if (!strcmp (node->name, "name"))
> folder->name = g_strdup (xmlNodeGetContent
(node));
>
> if (!strcmp (node->name, "image"))
> folder->image = g_strdup (xmlNodeGetContent
(node));
>
> if (!strcmp (node->name, "sound"))
> folder->sound = g_strdup (xmlNodeGetContent
(node));
>
> if (!strcmp (node->name, "beep"))
> folder->beep = g_strdup (xmlNodeGetContent
(node));
xmlNodeGetContent returns a string you need to free with xmlFree, so
this leaks.
> void
> em_tray_folders_refresh ()
> {
> GSList *list, *l;
> GtkTreeIter iter;
> FolderTrayNotify *folder;
>
> list = gconf_client_get_list (evo_tray->conf_client,
> "/apps/evolution/mail/notify/
folders",
> GCONF_VALUE_STRING,
> NULL);
> if (!list)
> return;
>
> gtk_list_store_clear (GTK_LIST_STORE (evo_tray->model));
>
> for (l = list; l; l = l->next) {
> folder = em_tray_get_folders_from_xml (l->data);
>
> gtk_list_store_append (GTK_LIST_STORE (evo_tray-
>model), &iter);
> gtk_list_store_set (GTK_LIST_STORE (evo_tray->model),
> &iter,
> NOTIFY_PIXBUF_COLUMN,
em_tray_get_image_scaled (folder->image),
> NOTIFY_NAME_COLUMN, folder->name,
> NOTIFY_SOUND_COLUMN, folder-
>sound,
> NOTIFY_ID_COLUMN, folder->id,
> NOTIFY_IMAGE_COLUMN, folder-
>image,
> NOTIFY_BEEP_COLUMN, folder->beep,
> -1);
> }
gconf_client_get_list returns a list and strings you need to free.
> static void
> em_tray_about (GtkMenuItem *menuitem, gpointer user_data)
> {
> static GtkWidget *about = NULL;
> GdkPixbuf *pixbuf = NULL;
> const gchar *authors[] = {"Miguel Angel López Hernández
<miguel gulev org mx>", NULL};
> const gchar *documenters[] = {NULL};
> const gchar *translator_credits = "translator_credits";
>
> if (about != NULL) {
> gdk_window_raise (about->window);
> gdk_window_show (about->window);
> return;
> }
>
> pixbuf = gdk_pixbuf_new_from_file (EVOLUTION_IMAGES "/ico-
mail.png", NULL);
>
> about = gnome_about_new ("Evolution Notification Applet",
> "0.0.1",
> "Copyright \xc2\xa9 2003-2004 Miguel
Angel López Hernández",
This should be Copyright Ximian/Novell too? Although this is part of a
separate program so i dunno.
> void
> em_tray_notify (gboolean notify, gchar *uri, gint type)
> {
> char *sound, *image_name;
> char *name, *tooltip;
> gboolean beep;
> FolderTrayNotify *folder;
>
> switch (type) {
> case MAIL_CONFIG_NOTIFY_ALL:
> beep = gconf_client_get_bool (evo_tray->conf_client,
"/apps/evolution/mail/notify/beep", NULL);
> sound = gconf_client_get_string (evo_tray-
>conf_client, "/apps/evolution/mail/notify/sound", NULL);
> image_name = gconf_client_get_string (evo_tray-
>conf_client, "/apps/evolution/mail/notify/image", NULL);
you leak image_name.
> em-panel-applet.h
> /* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 8 c-style:
"K&R" -*- */
> #ifndef __EM_PANEL_APPLET_H__
> #define __EM_PANEL_APPLET_H__
>
> #include "e-util/eggtrayicon.h"
> #include <glade/glade.h>
>
> #define DEFAULT_SOUND_NOTIFY "/usr/share/sounds/email.wav"
Is this defined anywhere in libgnome?
> #define DEFAULT_IMAGE_NOTIFY EVOLUTION_IMAGES "/ico-mail.png"
>
> typedef enum {
> MAIL_CONFIG_NOTIFY_NONE,
> MAIL_CONFIG_NOTIFY_ALL,
> MAIL_CONFIG_NOTIFY_SELECTED,
> MAIL_NOTIFY_TYPES,
> } MailConfigNewMailNotifyType;
MailConfig stuff should go into mail-config.[ch]
> static struct {
> const char *name;
> const char *description;
> } MailNotifyType[MAIL_NOTIFY_TYPES] = {
> { N_("None"),
> N_("Do not notify when new mail arrived.") },
> { N_("All Folders"),
> N_("Notify when new mail arrived to any folder.") },
> { N_("Selected Folders"),
> N_("Notify when new mail arrived to any of the selected
folders.") },
> };
This should be namespaced, i.e. start with EM, or MailConfig
> typedef struct {
> GConfClient *conf_client;
>
> GdkScreen *current_screen;
>
> GtkTreeModel *model;
>
> EggTrayIcon *tray_icon;
> GtkTooltips *tray_icon_tooltip;
> GtkWidget *popup_menu;
> GdkPixbuf *icon;
> GdkPixbuf *blank_icon;
>
> gboolean blink;
> gboolean newmail;
>
> gboolean created;
> } EvolutionTray;
>
> typedef struct {
> gchar *id;
> gchar *name;
> gchar *image;
> gchar *sound;
> gchar *beep;
> } FolderTrayNotify;
This should be namespaced.
> enum {
> NOTIFY_PIXBUF_COLUMN,
> NOTIFY_NAME_COLUMN,
> NOTIFY_SOUND_COLUMN,
> NOTIFY_ID_COLUMN,
> NOTIFY_IMAGE_COLUMN,
> NOTIFY_BEEP_COLUMN,
> NOTIFY_NUM_COLUMNS,
> };
So should this. Its questionable if it should even be public.
> void em_tray_init ();
> void em_tray_notify (gboolean notify, gchar *uri, gint type);
> GtkTreeModel * em_tray_get_store ();
> void em_tray_add_folder (GtkWidget *button, gpointer user_data);
> void em_tray_folders_refresh ();
> char * em_folder_tray_notify_to_xml (FolderTrayNotify *folder_notify);
> GdkPixbuf * em_tray_get_image_scaled (gchar *image_file);
>
> #endif
> e-util.diff
This diff contains some odd things, like changes to eggtrayicon which
you then include in whole.
> ? eggmarshalers.c
> ? eggmarshalers.h
> ? eggtraymanager.c
> ? eggtraymanager.h
> mail.diff
> --- em-mailer-prefs.c 11 Dec 2003 19:24:10 -0000 1.4
> +++ em-mailer-prefs.c 11 Jan 2004 10:28:24 -0000
> + list = NULL;
> + folder_notify = g_new0 (FolderTrayNotify, 1);
> + for (cont = gtk_tree_model_get_iter_first ((GtkTreeModel *) prefs-
>notify_folders_store, &iter); cont;
> + cont = gtk_tree_model_iter_next ((GtkTreeModel *) prefs-
>notify_folders_store, &iter)) {
> + gtk_tree_model_get ((GtkTreeModel *) prefs->notify_folders_store,
&iter,
> + NOTIFY_NAME_COLUMN, &name,
> + NOTIFY_SOUND_COLUMN, &sound,
> + NOTIFY_ID_COLUMN, &id,
> + NOTIFY_IMAGE_COLUMN, &image,
> + NOTIFY_BEEP_COLUMN, &beep,
> + -1);
> +
> + folder_notify->name = name;
> + folder_notify->sound = sound;
> + folder_notify->id = id;
> + folder_notify->image = image;
> + folder_notify->beep = beep;
> +
> + folder_xml = em_folder_tray_notify_to_xml (folder_notify);
> +
> + list = g_slist_append (list, folder_xml);
> + }
> + gconf_client_set_list (prefs->gconf, "/apps/evolution/mail/notify/
folders",GCONF_VALUE_STRING, list, NULL);
> + em_tray_folders_refresh ();
>
you have to free this list yourself after setting it. There are
probably other places, just check all gconf usage.
> + <schema>
> + <key>/schemas/apps/evolution/mail/notify/image</key>
> + <applyto>/apps/evolution/mail/notify/image</applyto>
> + <owner>evolution-mail</owner>
> + <type>string</type>
> + <default></default>
> + <locale name="C">
> + <short>Name of image</short>
> + <long>
> + Name of image to blink when new mail arrives and
> + all folders are selected
> + </long>
> + </locale>
> + </schema>
This setting seems a bit odd, from the ui perspective. Or at least
the ui for it seems odd.
> Index: mail-config.h
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/mail-config.h,v
> retrieving revision 1.113
> diff -u -r1.113 mail-config.h
> --- mail-config.h 1 Dec 2003 18:28:26 -0000 1.113
> +++ mail-config.h 11 Jan 2004 10:28:26 -0000
> @@ -73,12 +73,6 @@
> } MailConfigDisplayStyle;
>
> typedef enum {
> - MAIL_CONFIG_NOTIFY_NOT,
> - MAIL_CONFIG_NOTIFY_BEEP,
> - MAIL_CONFIG_NOTIFY_PLAY_SOUND,
> -} MailConfigNewMailNotify;
This shouldn't be moved out of mail-config (as i said above).
> -typedef enum {
> MAIL_CONFIG_XMAILER_NONE = 0,
> MAIL_CONFIG_XMAILER_EVO = 1,
> MAIL_CONFIG_XMAILER_OTHER = 2,
> Index: mail-folder-cache.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/mail-folder-cache.c,v
> retrieving revision 1.77
> diff -u -r1.77 mail-folder-cache.c
> --- mail-folder-cache.c 10 Dec 2003 18:36:13 -0000 1.77
> +++ mail-folder-cache.c 11 Jan 2004 10:28:26 -0000
> @@ -53,6 +53,9 @@
> #include "mail-config.h"
> #include "em-folder-tree-model.h"
>
> +/* New mail notification */
> +#include "em-panel-applet.h"
no need for hte comment
> @@ -147,25 +150,9 @@
> static gboolean
> notify_idle_cb (gpointer user_data)
> {
> - GConfClient *gconf;
> - char *filename;
> -
> - gconf = mail_config_get_gconf_client ();
> -
> - switch (notify_type) {
> - case MAIL_CONFIG_NOTIFY_PLAY_SOUND:
> - filename = gconf_client_get_string (gconf, "/apps/evolution/mail/
notify/sound", NULL);
> - if (filename != NULL) {
> - gnome_sound_play (filename);
> - g_free (filename);
> - }
> - break;
> - case MAIL_CONFIG_NOTIFY_BEEP:
> - gdk_beep ();
> - break;
> - default:
> - break;
> - }
> + GString *uri = (GString *) user_data;
> +
> + em_tray_notify (TRUE, uri->str, notify_type);
>
> time (&last_notify);
>
> @@ -189,6 +176,7 @@
> struct _folder_update *up;
> struct _store_info *si;
> time_t now;
> + GString *uri;
>
> component = mail_component_peek ();
> model = mail_component_peek_tree_model (component);
> @@ -238,8 +226,10 @@
> }
>
> time (&now);
> - if (notify_type != 0 && up->new && notify_idle_id == 0 && (now -
last_notify >= 5))
> - notify_idle_id = g_idle_add_full (G_PRIORITY_LOW, notify_idle_cb,
NULL, NULL);
> + if (notify_type != 0 && up->new && notify_idle_id == 0 && (now -
last_notify >= 5)) {
> + uri = g_string_new (notify_type == MAIL_CONFIG_NOTIFY_ALL ? up-
>path : up->uri);
> + notify_idle_id = g_idle_add_full (G_PRIORITY_LOW, notify_idle_cb,
uri, NULL);
> + }
There's no reason to use a GString for this, use a simple char *, and
you leak it anyway, you need to pass a free function to idle_add_full.
>
> free_update(up);
>
> @@ -353,6 +343,7 @@
>
> up = g_malloc0(sizeof(*up));
> up->path = g_strdup(mfi->path);
> + up->uri = g_strdup(mfi->uri); // For use with
MAIL_CONFIG_NOTIFY_SELECTED notification
c++ comments not allowed, and besides the comment isn't needed.
> up->unread = unread;
> up->new = new ? 1 : 0;
> up->store = mfi->store_info->store;
>
On Sun, 2004-01-11 at 05:38 -0600, Miguel Angel Lopez Hernandez wrote:
> Hi all,
>
> I've finished the panel notification applet, you can setup it by means
> of the "New Mail Notification" tab I've added to the Mailer Preferences.
> The notification types includes:
>
> + None
> - Do not notify when new mail arrives.
>
> + All Folders
> - Notify when new mail arrives to any folder.
> - You can setup the sound to play and image to blink in the panel
> notification area when new mail arrives.
>
> + Selected Folders
> - Notify when new mail arrives to any of the selected folders.
> - You can setup the sound to play and image to blink in the panel
> notification area for every selected folder
>
> I attach the following files:
> e-util.diff: diff file for the e-util directory.
> mail.diff: diff file for the mail directory.
> eggmarshalers.c/h: Needed to run eggtraymanager (put into e-util
> directory).
> eggtraymanager.c/h: put into e-util directory.
> em-panel-applet.c/h : The notification applet (put into mail directory)
>
> Well, this is not only my first evolution patch but my very first gnome
> patch so, I hope this is good enough :)
>
> P.D. I almost forget it... the Screenshots are in:
> http://www.gulev.org.mx/~miguel/ScreenshotsBountie/
>
> Greetings,
> Miguel
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
