Created attachment 124404 [details] [review]
Use g_timeout_add_seconds where possible

Comment on attachment 124404 [details] [review]
Use g_timeout_add_seconds where possible

>Index: browser-plugin/test-glow-button.c
>===================================================================
>--- browser-plugin/test-glow-button.c	(revision 5850)
>+++ browser-plugin/test-glow-button.c	(working copy)

Unneeded, it's just a test program.

>Index: src/plugins/totem-plugins-engine.c
>===================================================================
>--- src/plugins/totem-plugins-engine.c	(revision 5850)
>+++ src/plugins/totem-plugins-engine.c	(working copy)
>@@ -375,7 +375,7 @@
> 
> 	totem_plugins_engine_load_all ();
> 
>-	garbage_collect_id = g_timeout_add_full (G_PRIORITY_LOW, 20000, garbage_collect_cb, NULL, NULL);
>+	garbage_collect_id = g_timeout_add_seconds_full (G_PRIORITY_LOW, 20, garbage_collect_cb, NULL, NULL);

This is only used to garbage collect Python itself, and call totem_python_garbage_collect which is an empty function.
So this should only be called when HAVE_PYTHON is TRUE, then "#if 0"'ed, because it's useless.

>Index: src/plugins/totem-python-module.c
>===================================================================
>--- src/plugins/totem-python-module.c	(revision 5850)
>+++ src/plugins/totem-python-module.c	(working copy)
>@@ -541,7 +541,7 @@
> 			/* loop */;
> 
> 		/* This helps to force Python to give up its shell reference */
>-		g_timeout_add (1000, finalise_collect_cb, NULL);
>+		g_timeout_add_seconds (1, finalise_collect_cb, NULL);

This should be an idle instead, as the function just loops in the main context anyway. I'm pretty sure this is only ever called on occasion and Totem will have exited by the time the function would have been called.

>Index: src/plugins/gromit/totem-gromit.c
>===================================================================
>--- src/plugins/gromit/totem-gromit.c	(revision 5850)
>+++ src/plugins/gromit/totem-gromit.c	(working copy)
>@@ -69,7 +69,7 @@
> 	TotemPluginClass parent_class;
> } TotemGromitPluginClass;
> 
>-#define INTERVAL 10000
>+#define INTERVAL 10 /* seconds */
> 
> static const char *start_cmd[] =	{ NULL, "-a", "-k", "none", NULL };
> static const char *toggle_cmd[] =	{ NULL, "-t", NULL };
>@@ -238,7 +238,7 @@
> 
> 	launch (visibility_cmd);
> 	launch (clear_cmd);
>-	plugin->id = g_timeout_add (INTERVAL, totem_gromit_timeout_cb, plugin);
>+	plugin->id = g_timeout_add_seconds (INTERVAL, totem_gromit_timeout_cb, plugin);
> }

Fine.

> static gboolean
>Index: src/test-properties-page.c
>===================================================================
>--- src/test-properties-page.c	(revision 5850)
>+++ src/test-properties-page.c	(working copy)

No need to modify the test programs.

>Index: src/eggdesktopfile.c
>===================================================================
>--- src/eggdesktopfile.c	(revision 5850)
>+++ src/eggdesktopfile.c	(working copy)
>@@ -987,7 +987,7 @@
> 					     NULL);
> }
> 
>-#define EGG_DESKTOP_FILE_SN_TIMEOUT_LENGTH (30 /* seconds */ * 1000)
>+#define EGG_DESKTOP_FILE_SN_TIMEOUT_LENGTH (30 /* seconds */)
> 
> typedef struct {
>   GdkDisplay *display;
>@@ -1017,8 +1017,8 @@
>   sn_data->display = g_object_ref (display);
>   sn_data->startup_id = g_strdup (startup_id);
> 
>-  g_timeout_add (EGG_DESKTOP_FILE_SN_TIMEOUT_LENGTH,
>-		 startup_notification_timeout, sn_data);
>+  g_timeout_add_seconds (EGG_DESKTOP_FILE_SN_TIMEOUT_LENGTH,
>+			 startup_notification_timeout, sn_data);
> }
> #endif /* GTK 2.12 */

Those changes need to be done in the canonical upstream for that code.

Created attachment 124440 [details] [review]
Use g_timeout_add_seconds where possible (updated)

I've kept the changes to the test programs, since they do no harm. I've created bug #564156 for the timeout changes in libegg, and will update our copies once that's committed.

Looks good apart from the calls in the plugin engine which need to be "#if 0"'d as they don't do anything. No need to place an idle handler if the function it runs does nothing. Might want to add a comment about why it's disabled as well.

Committed with the #if 0s and a comment about them.

2008-12-12  Philip Withnall  <philip@tecnocode.co.uk>

	* browser-plugin/test-glow-button.c (main):
	* src/plugins/gromit/totem-gromit.c (totem_gromit_clear):
	* src/plugins/totem-plugins-engine.c (totem_plugins_engine_init),
	(totem_plugins_engine_shutdown):
	* src/plugins/totem-python-module.c (totem_python_shutdown):
	* src/test-properties-page.c (main): Use g_timeout_add_seconds
	instead of g_timeout_add where possible to allow for event grouping.
	(Closes: #564073)