Bug 59921

Created attachment 73715 [details] [review]
Use g_timeout_add_seconds wherever possible

Comment on attachment 73715 [details] [review]
Use g_timeout_add_seconds wherever possible

Review of attachment 73715 [details] [review]:
-----------------------------------------------------------------

Thanks for looking into this, but please don't blindly replace g_timeout_add (n * 1000, ...) with g_timeout_add_seconds (n, ...). Not every call to g_timeout_add causes extra wakeups, and not every call to g_timeout_add can safely be replaced by g_timeout_add_seconds.

::: examples/client/extended-client.c
@@ +307,4 @@
>    g_signal_connect (cm, "got-info",
>        G_CALLBACK (connection_manager_got_info), NULL);
>  
> +  timer = g_timeout_add_seconds (5, time_out, NULL);

This is only example code anyway, but yes add_seconds is better here, and it's somewhat important to be "correct" in an example.

::: telepathy-glib/base-media-call-content.c
@@ +73,4 @@
>  #include "telepathy-glib/util.h"
>  #include "telepathy-glib/util-internal.h"
>  
> +#define DTMF_PAUSE_MS (3)

MS in the name denotes milliseconds, to avoid confusion between the various common time measurements (s/ms/µs/ns): if this change was acceptable, then the constant's name should change to DTMF_PAUSE_S or DTMF_PAUSE_SEC.

However, I don't think this change is really suitable anyway:

@@ +1123,4 @@
>                      self->priv->current_dtmf_state);
>                  break;
>                case DTMF_CHAR_CLASS_PAUSE:
> +                self->priv->tones_pause_timeout_id = g_timeout_add_seconds (

DTMF (aka "touch-tone" and various other names) is a way to communicate machine-readable information to, for instance, switchboards ("to speak to the Sales department, press 1").

If a switchboard/dial-string contains "p", which means "pause for 3 seconds", then it should pause for 3 seconds, not for 2.25 or 3.9 (both of which are entirely possible in GLib's current implementation of g_timeout_set_expiration).

This code only executes during VoIP calls, so we can't really reduce wakeups here: we'll be waking up frequently anyway, to process new audio data. So I think g_timeout_add is really the correct thing here.

::: telepathy-glib/run.c
@@ +101,4 @@
>      }
>  }
>  
> +#define DIE_TIME 5

I'd prefer a rename to DIE_TIME_SEC, while you're changing the affected lines anyway.

The changes in this file make sense, although they basically won't save any wakeups: this timeout happens once during startup, and once every time Gabble finds that it has no active IM connections. That's something like 2 wakeups per IM session - pretty insignificant.

::: tests/dbus/account-manager.c
@@ +121,4 @@
>      guint timeout)
>  {
>    script_append_action (test, quit_action, NULL);
> +  test->timeout_id = g_timeout_add_seconds (timeout, test_timed_out, test);

These changes are in a regression test which is constantly active, so you won't avoid any wakeups. If the timeout actually goes off, the test fails.

If you want to change this file at all, please change it by removing this timeout completely, and changing script_start_with_deadline (Test *, guint) to just script_start (Test *).

The file has a global 10-second timeout, which is enough to stop it getting stuck on failure.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/109.