[Bug 24257] TpAccount, TpAccountManager: prepare never terminates if the object is invalidated

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=24257





--- Comment #9 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-11-10 13:19:30 PST ---
<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=aa085b71d2ab0d91e347484c578a200dae581a2c>:

newline between statement and "if" block in the second hunk, please

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=7c079e56bd7c6a1300d8106893d63dd505f9c66e>:
<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=66aa9a1de2590e59aef33a7658479ed15cd425d4>:
> Fix prepare for Account

I'd prefer a one-liner like "fd.o#24257: tp_account_prepare: fail pending calls
if invalidated"; likewise for the AM.

(fd.o# because many Telepathy users - GNOME, Maemo, every distro - have their
own idea of what bug numbers mean; bug# at the beginning because it's short and
has a high information density; reference the module; explain what you're
fixing, rather than just saying "fix")

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=d9a7adfca0941e0c6b468e8f793b7380f22d9f21>:

The check should be a g_return_val_if_fail, and should probably use
tp_dbus_check_valid_object_path() directly. It can replace the existing path !=
NULL check, since tp_dbus_check_valid_object_path() will itself
g_return_val_if_fail on NULL.

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=90837609f00c62ade6fb980dcdcb5a1b81f8bb3f>:

-2009 at the high end of the copyright ranges please.

>+/* FIXME: Do a purge of headers at some point */

Please sort the telepathy-glib/foo block alphabetically, at least.

>+G_DEFINE_TYPE_WITH_CODE (SimpleAccountManager,

Doesn't need a trailing semicolon either in the code or after the last
parenthesis (G_DEFINE_TYPE_WITH_CODE supplies both for you, so the two extra
semicolons create two empty statements, which some C compilers whine about).

>+/* This structure is actually unused. I included it so I could extend things
>+ * in future if I wanted without huge re-compiles. Turns out everything is
>+ * compiled into one big lib though, so changing this .c file still kicks off
>+ * a big re-compile.

This isn't worth commenting - doing the "pimpl" coding style (FooPrivate) is
not unusual. Also, it's a big relink, not a big recompile :-P

>+ */
>+struct _SimpleAccountManagerPrivate
>+{
>+  /* Also unused. Only included because private structures must be >0 bytes. */
>+  GList accounts;

If it's unused, I'd prefer "int dummy;", which appears in some of the examples
too.

>+simple_account_manager_get_property (GObject *object,

If you're responding to my coding style nitpicking anyway, I'd like to request
a blank line between cases (so between break and default, here) :-)


>+static const char *ACCOUNT_MANAGER_INTERFACES[] = {
>+    TP_IFACE_ACCOUNT_MANAGER,
...
>+      "Interfaces implemented. In this case just AccountManager",

That should just be the empty list. Interfaces is defined to be the set of
"extra interfaces" - if you're requesting the o.f.T.AM.Interfaces property and
it worked, you ought to be able to infer that o.f.T.AM is a supported interface
without any additional help :-)

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=1744d8be9481a6e09eb583906e4e419f464056bd>:
+  // TODO: call things like tp_account_manager_create_account_async.
+  // This clearly needs to be done in the prepare callback. Might have to
+  // think of a way to refactor this.

Either fix this, or file a trivial/low bug for it (and use C-style comments
rather than C++-style).

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=8ab93a727864f0117c08b789cc62a438e4adf15f>:
> +  g_assert (test->prepared == tp_account_manager_is_prepared (test->am, TP_ACCOUNT_MANAGER_FEATURE_CORE));

g_assert_intcmp (t_a_m_i_p (), ==, t->p) ?

> +assert_failed_action (gpointer script_data,

Don't you want to free the GError here? If tests don't leak, it's easier to
valgrind the library. (Either put a const GError * in the struct, or copy any
unowned errors.)


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.