Review of attachment 238329 [details] [review]:

::: backends/telepathy/tp-backend.vala
@@ +86,2 @@
       /* First handle adding any missing persona stores. */
+      GLib.List<unowned Account> accounts = null;

This should have type ‘GLib.List<unowned Account>?’, since it’s nullable.

@@ +170,3 @@
         {
           this._prepare_pending = true;
           this._account_manager = AccountManager.dup ();

If AccountManager.dup() can return null, the type of Tp.Backend._account_manager needs to be made nullable. Might be useful to add a comment to its declaration which specifies it’s null iff prepare() failed.

@@ +174,3 @@
+          if (this._account_manager == null)
+            {
+              throw new TelepathyGLib.Error.NOT_AVAILABLE ("Unable to connect to D-Bus");

Throwing a tp-glib error seems a bit odd. Would it better to wrap it in a new folks Backend error domain?

(In reply to comment #1)
> This should have type ‘GLib.List<unowned Account>?’, since it’s nullable.

Fair enough, I'd assumed the compiler would have told me if I got this wrong. (I basically only learned Vala this week :-)

> If AccountManager.dup() can return null, the type of
> Tp.Backend._account_manager needs to be made nullable. Might be useful to add a
> comment to its declaration which specifies it’s null iff prepare() failed.

Sure.

> Throwing a tp-glib error seems a bit odd. Would it better to wrap it in a new
> folks Backend error domain?

I'd been going for "this is basically a workaround for tp_account_manager_dup() not raising a GError, so use a GError that it might conceivably have raised". After all, the same function will throw a Tp error if we *can* get a TpAccountManager, but its prepare_async() fails.

I suppose one possibility would be to call tp_dbus_daemon_dup() first (which currently throws a dbus-glib error, but will be a GDBus error if we ever port Telepathy to GDBus). If tp_dbus_daemon_dup() worked, then tp_account_manager_dup() can't fail - telepathy-glib doesn't explicitly document this, but it could.

(In reply to comment #2)
> (In reply to comment #1)
> > This should have type ‘GLib.List<unowned Account>?’, since it’s nullable.
> 
> Fair enough, I'd assumed the compiler would have told me if I got this wrong.
> (I basically only learned Vala this week :-)

You want to pass ‘--enable-experimental-non-null’ for that. It’s not enabled by default because there are sufficiently many bugs in the bindings that it gets too noisy. I did some work to make folks null-safe a while ago, but we’ve probably regressed quite a bit since then. I think if I were to do more work on that I’d want to improve valac to be a bit cleverer about tracking nullability in implicitly-typed variables first.

> > Throwing a tp-glib error seems a bit odd. Would it better to wrap it in a new
> > folks Backend error domain?
> 
> I'd been going for "this is basically a workaround for tp_account_manager_dup()
> not raising a GError, so use a GError that it might conceivably have raised".
> After all, the same function will throw a Tp error if we *can* get a
> TpAccountManager, but its prepare_async() fails.

Ah, if the existing code already throws a Tp error, then that’s enough justification for me to leave the patch as it is in this respect.

> I suppose one possibility would be to call tp_dbus_daemon_dup() first (which
> currently throws a dbus-glib error, but will be a GDBus error if we ever port
> Telepathy to GDBus). If tp_dbus_daemon_dup() worked, then
> tp_account_manager_dup() can't fail - telepathy-glib doesn't explicitly
> document this, but it could.

I don’t see that as any better than throwing a Tp error — it’s still a non-folks error.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME'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.gnome.org/GNOME/folks/issues/55.