Merge lp:~sense/notify-osd/fix-465801 into lp:notify-osd/trunk

[Sense Egbert Hofstede]

[Sense Egbert Hofstede (sense)]

These changes fix bug #465801 and have been tested and found to be working.
The variable dirname in src/log.c is now a const gchar* and is not freed. Instead of hardcoding the use of ~/.cache, now the function g_get_user_cache_dir() is used. Therefore <glib.h> is included in src/log.c

[Robert Collins (lifeless)]

Looks sane to me, though I'm not a notify-OSD committer.

It would be good to factor out the logfile name selection and add a test
to demonstrate that your needs are met, but if you don't feel like
doing that, perhaps a comment [so that this doesn't get changed
incorrectly in the future] would be useful.

-Rob

[Ted Gould (ted)]

This should use g_build_path instead of the g_strdup_printf. It should look like:

g_build_path(dirname, "notify-osd.log", NULL);

That way it handles all the fishyness that could be in the path. Technically it's also cross-platform safe, but I doubt anyone will port Notify OSD to windows ;)

[lp:~sense/notify-osd/fix-465801]

410. By Sense Egbert Hofstede on 2009-11-01

Use g_build_filename () in src/log.c to determine the log file path.

[Sense Egbert Hofstede (sense)]

I've replaced g_strdup_printf () with g_build_filename ().
However, I don't fully understand Robert Collins. Did you suggest to move the name selection to a function? And what tests would you suggest to add?

[Mirco Müller (macslow)]

I also am not sure what Robert means with "your needs are met". That aside your changes up to rev410 look good. Thanks Ted, for you suggestion to make writing to the cache-file cross-platform :)

[Robert Collins (lifeless)]

What I mean is:
 - the change is being made because something is wrong
 - a test that demonstrates the wrongness would prevent it being
reintroduced later
 - 'your needs' are whatever you want notify-osd to do differently than
it does.

-Rob

[Sense Egbert Hofstede (sense)]

What I want Notify OSD to do different is storing the log file. Currently it is hard coded to save the file in .cache, but I would like Notify OSD to adhere to the FreeDesktop standards and use the appointed variable for getting the cache directory.

How to test: a test doesn't need to be written, it can be tested by exporting a different directory to $XDG_CACHE_HOME and restart the version of Notify OSD in this branch.

[Mirco Müller (macslow)]

I'll test-run and merge that with other notify-osd related work once I'm back home from UDS next week.

[Mirco Müller (macslow)]

BTW, thanks for the fix. I'll add that to notify-osd SRU for Karmic (and hope it'll get accepted by the desktop-team).

review: Approve

[David Barth (dbarth)]

Thanks for the patch. Per company policy, we need you to accept the Canonical Contributor Agreement, and the copyright assignment that goes with it. More information at: http://www.canonical.com/contributors

=== modified file 'src/log.c'
--- src/log.c	2009-04-09 07:33:35 +0000
+++ src/log.c	2009-11-01 10:48:11 +0000
@@ -30,6 +30,7 @@
 #include <sys/time.h>
 #include <time.h>
 #include <stdio.h>
+#include <glib.h>
 
 #include "bubble.h"
 
@@ -53,11 +54,10 @@
 	g_return_if_fail (homedir != NULL);
 
 	// Make sure the cache directory is there, if at all possible
-	char *dirname = g_strdup_printf ("%s/.cache", homedir);
+	const gchar *dirname = g_get_user_cache_dir ();
 	g_mkdir_with_parents (dirname, 0700);
 
-	char *filename = 
-		g_strdup_printf ("%s/.cache/notify-osd.log", homedir);
+	char *filename = g_build_filename (dirname, "notify-osd.log", NULL);
 
 	logfile = fopen (filename, "w");
 	if (logfile == NULL)
@@ -65,7 +65,6 @@
 			   filename);
 
 	g_free (filename);
-	g_free (dirname);
 
 	/* discard all debug messages unless DEBUG is set */
 	if (! g_getenv ("DEBUG"))