After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 564073 - Use g_timeout_add_seconds where possible
Use g_timeout_add_seconds where possible
Status: RESOLVED FIXED
Product: totem
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: General Totem maintainer(s)
General Totem maintainer(s)
Depends on: 564156
Blocks:
 
 
Reported: 2008-12-11 07:00 UTC by Philip Withnall
Modified: 2008-12-12 16:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use g_timeout_add_seconds where possible (3.32 KB, patch)
2008-12-11 07:01 UTC, Philip Withnall
none Details | Review
Use g_timeout_add_seconds where possible (updated) (2.89 KB, patch)
2008-12-11 17:50 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2008-12-11 07:00:27 UTC
Prompted by http://gould.cx/ted/blog/Saving_the_world_one__w_at_a_time, we should use g_timeout_add_seconds over g_timeout_add where possible. Patch coming up.
Comment 1 Philip Withnall 2008-12-11 07:01:36 UTC
Created attachment 124404 [details] [review]
Use g_timeout_add_seconds where possible
Comment 2 Bastien Nocera 2008-12-11 14:39:41 UTC
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.
Comment 3 Philip Withnall 2008-12-11 17:50:57 UTC
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.
Comment 4 Bastien Nocera 2008-12-12 14:46:21 UTC
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.
Comment 5 Philip Withnall 2008-12-12 16:33:59 UTC
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)