Merge lp:~cando/zeitgeist-datasources/fix_xchat into lp:zeitgeist-datasources/0.8

Proposed by Stefano Candori
Status: Merged
Merged at revision: 110
Proposed branch: lp:~cando/zeitgeist-datasources/fix_xchat
Merge into: lp:zeitgeist-datasources/0.8
Diff against target: 203 lines (+94/-10)
2 files modified
xchat/README (+13/-0)
xchat/zeitgeist-dataprovider.c (+81/-10)
To merge this branch: bzr merge lp:~cando/zeitgeist-datasources/fix_xchat
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve
Review via email: mp+46304@code.launchpad.net

Description of the change

In this branch i've improved the XChat dataprovider.
With these mods when the user quits from XChat, the dps sends LEAVE_EVENTS to Zeitgeist, that's very important because i guess that everybody don't use the /part command(that is handled correctly by the dps)for quitting from the channels...

To post a comment you must log in.
111. By Stefano Candori

Xchat dps: change g_slist_append to g_slist_prepend

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

There's something askew with how you use the channel_list GSList.

 1) You leak the string passed in to the g_slist_append() method. g_strdup() copies the string you pass to it and you need to free it again somehow. When that entry is removed from the list you need to g_free() the string somehow.

 2) You g_slist_remove() call will not match anything in your channel_list. It compares pointer values, not strings, for the lookup. Also - you call g_strdup(channel) which allocates new memory, which doesn't get freed. You need a matching g_free() on the return value of that call somewhere.

 3) The 'text' variable you build with g_strconcat() also needs a g_free() before you return. Otherwise you'll leak it.

 4) in on_quit() don't g_strdup() the data arg. Just cast it to a 'const gchar*'. In this case you also shouldn't g_free() it

 5) in on_quit() you leak the url and text strings. They need a g_free() as you do with 'channel' (unless you do as advised in 4)).

review: Needs Fixing
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

There's something askew with how you use the channel_list GSList.

 1) You leak the string passed in to the g_slist_append() method. g_strdup() copies the string you pass to it and you need to free it again somehow. When that entry is removed from the list you need to g_free() the string somehow.

 2) You g_slist_remove() call will not match anything in your channel_list. It compares pointer values, not strings, for the lookup. Also - you call g_strdup(channel) which allocates new memory, which doesn't get freed. You need a matching g_free() on the return value of that call somewhere.

 3) The 'text' variable you build with g_strconcat() also needs a g_free() before you return. Otherwise you'll leak it.

 4) in on_quit() don't g_strdup() the data arg. Just cast it to a 'const gchar*'. In this case you also shouldn't g_free() it

 5) in on_quit() you leak the url and text strings. They need a g_free() as you do with 'channel' (unless you do as advised in 4)).

review: Needs Fixing
112. By Stefano Candori

Fixed memory leaks

113. By Stefano Candori

Removed unuseful variable

Revision history for this message
Stefano Candori (cando) wrote :

OK, i should have fixed all these leaks...:)

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Just a nitpick :-) Can you change this line:

  channel_list = g_slist_remove(channel_list, tmp->data);

to:

  channel_list = g_slist_delete_link(channel_list, tmp);

The way you do it now you do first a linear search for the data (the while(tmp) loop) and then another linear search to remove it with g_slist_remove(). Since you already have the link you wanna remove, you should use the O(1) function g_slist_delete_link() instead. But this is a nitpick ofcourse since the channels list isn't ever gonna be that big so it matters - but style is style right? :-)

In any case - approved. Nice work Stefano!

review: Approve
114. By Stefano Candori

Changing g_slist_remove to g_slist_delete_link

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'xchat/README'
--- xchat/README 1970-01-01 00:00:00 +0000
+++ xchat/README 2011-01-18 09:25:42 +0000
@@ -0,0 +1,13 @@
1Zeitgeist plugin to the XChat irc client
2****************************************
3
4To use this plugin:
5
6* Install Zeitgeist 0.3 (or greater)
7* Type "make" and "sudo make install" for a system-wide installation; "make" and "make local-install"
8 for a local installation
9
10*************************************************************************************************************
11 N.B. Please don't install both system-wide and locally! XChat will load both plugins, getting double events
12 on your Zeitgeist.
13*************************************************************************************************************
014
=== modified file 'xchat/zeitgeist-dataprovider.c'
--- xchat/zeitgeist-dataprovider.c 2011-01-11 00:19:41 +0000
+++ xchat/zeitgeist-dataprovider.c 2011-01-18 09:25:42 +0000
@@ -22,17 +22,16 @@
22#include "xchat-plugin.h"22#include "xchat-plugin.h"
23#include <zeitgeist.h>23#include <zeitgeist.h>
2424
25
26#define PNAME "Zeitgeist"25#define PNAME "Zeitgeist"
27#define PDESC "Inform Zeitgeist about your activity"26#define PDESC "Inform Zeitgeist about your activity"
28#define PVERSION "0.1"27#define PVERSION "0.1"
2928
30static xchat_plugin *ph = NULL; /* plugin handle */29static xchat_plugin *ph = NULL; /* plugin handle */
31static ZeitgeistLog *zg_log = NULL; /* Zeitgeist-daemon hanlde*/30static ZeitgeistLog *zg_log = NULL; /* Zeitgeist-daemon hanlde*/
31static GSList *channel_list = NULL; /* List of joined channels */
3232
33static void send_event_to_zeitgeist(char *url_, char* text_, const char* ev_interpretation)33static void send_event_to_zeitgeist(char *url_, char* text_, const char* ev_interpretation)
34{34{
35 /* type is 0 for PART event, 1 for JOIN event */
36 const char *url, *origin, *mimetype, *text;35 const char *url, *origin, *mimetype, *text;
37 const char *interpretation = NULL;36 const char *interpretation = NULL;
38 const char *manifestation = NULL;37 const char *manifestation = NULL;
@@ -40,7 +39,7 @@
40 39
41 url = url_;40 url = url_;
42 origin = url;41 origin = url;
43 mimetype = "text/plain"; //?42 mimetype = "text/plain";
44 text = text_;43 text = text_;
45 interpretation = ZEITGEIST_NMO_IMMESSAGE;44 interpretation = ZEITGEIST_NMO_IMMESSAGE;
46 manifestation = ZEITGEIST_NFO_SOFTWARE_SERVICE;45 manifestation = ZEITGEIST_NFO_SOFTWARE_SERVICE;
@@ -66,7 +65,7 @@
66 text,65 text,
67 ev_interpretation);*/66 ev_interpretation);*/
68 67
69 zeitgeist_log_insert_events_no_reply (zg_log, event, NULL); 68 zeitgeist_log_insert_events_no_reply (zg_log, event, NULL);
70}69}
7170
72static int join_cb(char *word[], void *userdata)71static int join_cb(char *word[], void *userdata)
@@ -75,11 +74,16 @@
75 const char *channel = word[2];74 const char *channel = word[2];
76 char *url, *text;75 char *url, *text;
77 76
77 channel_list = g_slist_prepend(channel_list, g_strdup(channel));
78
78 url = g_strconcat("irc://", server, "/", channel, NULL);79 url = g_strconcat("irc://", server, "/", channel, NULL);
79 text = g_strconcat("You joined ", channel, NULL);80 text = g_strconcat("You joined ", channel, NULL);
80 81
81 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_ACCESS_EVENT);82 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_ACCESS_EVENT);
82 83
84 g_free(url);
85 g_free(text);
86
83 return XCHAT_EAT_NONE;87 return XCHAT_EAT_NONE;
84}88}
8589
@@ -88,11 +92,26 @@
88 const char *server = xchat_get_info(ph, "host");92 const char *server = xchat_get_info(ph, "host");
89 const char *channel = word[3];93 const char *channel = word[3];
90 char *url, *text;94 char *url, *text;
91 95 GSList *tmp = channel_list;
96
92 url = g_strconcat("irc://", server, "/", channel, NULL); 97 url = g_strconcat("irc://", server, "/", channel, NULL);
93 text = g_strconcat("You parted from ", channel, NULL);98 text = g_strconcat("You parted from ", channel, NULL);
94 99
95 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_LEAVE_EVENT);100 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_LEAVE_EVENT);
101
102 while(tmp)
103 {
104 if (g_strcmp0 (tmp->data, channel) == 0)
105 {
106 g_free(tmp->data);
107 channel_list = g_slist_delete_link(channel_list, tmp);
108 break;
109 }
110 tmp = tmp->next;
111 }
112
113 g_free(url);
114 g_free(text);
96115
97 return XCHAT_EAT_NONE;116 return XCHAT_EAT_NONE;
98}117}
@@ -104,10 +123,13 @@
104 char *url, *text;123 char *url, *text;
105 124
106 url = g_strconcat("irc://", server, "/", channel, NULL); 125 url = g_strconcat("irc://", server, "/", channel, NULL);
107 text = g_strconcat(channel, ": you sent a message: ", word[2], NULL);126 text = g_strconcat(channel, ": you sent \"", word[2],"\".", NULL);
108 127
109 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_SEND_EVENT);128 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_SEND_EVENT);
110129
130 g_free(url);
131 g_free(text);
132
111 return XCHAT_EAT_NONE;133 return XCHAT_EAT_NONE;
112}134}
113135
@@ -118,13 +140,31 @@
118 char *url, *text;140 char *url, *text;
119 141
120 url = g_strconcat("irc://", server, "/", channel, NULL); 142 url = g_strconcat("irc://", server, "/", channel, NULL);
121 text = g_strconcat(channel, ": you received a message from ", word[1],": ", word[2], NULL);143 text = g_strconcat(channel, ": you received \"", word[2],"\" from ", word[1], NULL);
122 144
123 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_RECEIVE_EVENT);145 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_RECEIVE_EVENT);
124146
147 g_free(url);
148 g_free(text);
149
125 return XCHAT_EAT_NONE;150 return XCHAT_EAT_NONE;
126}151}
127152
153static void on_quit(gpointer data, gpointer userdata)
154{
155 const char *server = xchat_get_info(ph, "host");
156 const char *channel = (const char*) data;
157 char *url, *text;
158
159 url = g_strconcat("irc://", server, "/", channel, NULL);
160 text = g_strconcat("You parted from ", channel, NULL);
161
162 send_event_to_zeitgeist(url, text, ZEITGEIST_ZG_LEAVE_EVENT);
163
164 g_free(url);
165 g_free(text);
166}
167
128void xchat_plugin_get_info(char **name, char **desc, char **version, void **reserved)168void xchat_plugin_get_info(char **name, char **desc, char **version, void **reserved)
129{169{
130 *name = PNAME;170 *name = PNAME;
@@ -132,6 +172,9 @@
132 *version = PVERSION;172 *version = PVERSION;
133}173}
134174
175static int initialized = 0;
176static int reinit_tried = 0;
177
135int xchat_plugin_init(xchat_plugin *plugin_handle,178int xchat_plugin_init(xchat_plugin *plugin_handle,
136 char **plugin_name,179 char **plugin_name,
137 char **plugin_desc,180 char **plugin_desc,
@@ -139,6 +182,16 @@
139 char *arg)182 char *arg)
140{183{
141 ph = plugin_handle;184 ph = plugin_handle;
185
186 /* Block double initalization. */
187 if (initialized != 0) {
188 xchat_print(ph, "Zeitgeist plugin already loaded");
189 /* deinit is called even when init fails, so keep track
190 * of a reinit failure. */
191 reinit_tried++;
192 return 0;
193 }
194 initialized = 1;
142195
143 *plugin_name = PNAME;196 *plugin_name = PNAME;
144 *plugin_desc = PDESC;197 *plugin_desc = PDESC;
@@ -155,3 +208,21 @@
155208
156 return 1; 209 return 1;
157}210}
211
212int xchat_plugin_deinit()
213{
214 /* A reinitialization was tried. Just give up */
215 if (reinit_tried) {
216 reinit_tried--;
217 return 1;
218 }
219
220 g_slist_foreach(channel_list, on_quit, NULL);
221 g_slist_foreach(channel_list, (GFunc)g_free, NULL);
222 g_slist_free(channel_list);
223 channel_list = NULL;
224
225 g_object_unref(zg_log);
226 zg_log = NULL;
227 return 1;
228}

Subscribers

People subscribed via source and target branches