The specification says that, when constructing a new account's object path (which is its persistent identifier): The account manager SHOULD choose acct such that if an account is deleted, its object path will be re-used if and only if the new account is in some sense "the same" (incorporating the 'account' parameter in some way is recommended) MC doesn't really respect this all that well: it guesses based on the 'account' parameter, but this doesn't cut it for protocols like IRC. Since spec version something recent, there's been a method that CMs can implement to turn a dictionary of parameters into a unique identifier for the account, suitable for MC to use when building the object path: <http://telepathy.freedesktop.org/spec/Protocol.html#Method:IdentifyAccount>. MC should use it, where possible. This also allows MC to prevent you creating two copies of the same account. For backwards compatibility, the object paths for existing accounts should be preserved, but we could stash the CM's preferred identifier alongside the account to avoid having to repeatedly retrieve it as discussed on bug 34416.
Hijacking this a bit: ExternalPasswordStorage and AccountStorage, which are related to this, are *still* drafts... (In reply to comment #8) > Actually, no, they're still marked as DRAFT in the spec. Let's just undraft > them: the implementation in MC isn't great, but it'll do. I noticed they don't have any regression test coverage, started writing a test, ran into "ugh, how much of a CM do I need to implement?" and lost interest for now. I'd like to fix the fact that it calls IdentifyAccount() repeatedly - I think it ought to call it exactly once, on account creation, and save the result in the account, as Will said on Bug #34416. I wonder whether telepathy-haze should even persist libpurple accounts on disk, and use the AccountStorage interface to manage them... although it would probably have to make sure that all accounts are disabled at the libpurple level before it connects any of them.
Created attachment 87679 [details] [review] Regression tests: use a simulated CM instead of just holding the bus name We'll need this if we want to call IdentifyAccount. --- Actually, with a more gradual approach (not trying to solve AccountStorage immediately), it isn't so bad.
Created attachment 87680 [details] [review] Don't give storage plugins the full parameter set, just 'account' We broke plugin API since the last release anyway, so this isn't a new API break. The doc-comments all claim that the string is the result of IdentifyAccount, because that's about to be true. :-)
Created attachment 87681 [details] [review] Call IdentifyAccount to get new accounts' names It's exposed through the plugin API so that exemplary plugins can use the same utility functions to decide how to name accounts for the ::created signal, if they want to.
Created attachment 87682 [details] [review] McdCreateAccountData: correctly free the user-data In theory this is a bugfix, but nothing within MC actually calls this function with a non-NULL GDestroyNotify anyway, and it isn't API.
Comment on attachment 87680 [details] [review] Don't give storage plugins the full parameter set, just 'account' Review of attachment 87680 [details] [review]: ----------------------------------------------------------------- ::: mission-control-plugins/account.c @@ +287,4 @@ > * MC has not previously seen before (ie one created by a 3rd party > * in the back-end that the plugin in question provides an interface to). > * > + * Changed in 5.17: instead of a map from string to GValue, the last Yeah in principle this should have been 5.UNRELEASED, but if we can't get this merged before we next stable-branch, then we're not trying hard enough. ::: src/mcd-account-manager-sso.c @@ +971,5 @@ > g_hash_table_insert (params, g_strdup (MC_ACCOUNT_KEY), &value); > > + /* FIXME: We should call IdentifyAccount(params) really. But this > + * plugin probably doesn't even compile any more, so, whatever; > + * just use the "account name". */ If people want to just delete this (pseudo-)plugin, I'd be in favour.
Comment on attachment 87681 [details] [review] Call IdentifyAccount to get new accounts' names Review of attachment 87681 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +220,4 @@ > AC_DEFINE([TP_DISABLE_SINGLE_INCLUDE], [], [Avoid individual headers]) > > PKG_CHECK_MODULES([GLIB], > + [glib-2.0 >= 2.36, gobject-2.0, gmodule-no-export-2.0, gio-2.0]) GTask because life's too short. ::: mission-control-plugins/account.c @@ +312,5 @@ > +void > +mcp_account_manager_identify_account_async (McpAccountManager *mcpa, > + const gchar *manager, > + const gchar *protocol, > + GVariant *parameters, GVariant because I don't want to add any new API that's shaped like dbus-glib. I should probably add some doc-comments to these. ::: src/mcd-storage.c @@ +622,5 @@ > + { > + g_task_return_pointer (task, g_strdup (identification), g_free); > + } > + else if (g_error_matches (error, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED) || > + g_error_matches (error, DBUS_GERROR, DBUS_GERROR_SERVICE_UNKNOWN)) I want to trap ServiceUnknown and either raise a different error or carry on with the "can't identify" code path, because it seems bad if MC raises ServiceUnknown in response to a CreateAccount call - that's indistinguishable from MC itself not existing! tests/twisted/account-manager/bad-cm.py wants this to return NotImplemented, which is raised by _mcd_account_set_parameters. The spec says so. If the CM raises InvalidArgument from IdentifyAccount (which is undocumented, but reasonable), we'll pass that on as the result of CreateAccount, which is what the spec says we should do. If the CM raises InvalidHandle (which is undocumented, but is a reasonable way to represent "that identifier is bad"), we'll pass *that* on as the result of CreateAccount, which is undocumented but reasonable. I'm tempted to document InvalidArgument and InvalidHandle as possible errors raised by IdentifyAccount, and InvalidHandle as a possible error raised by CreateAccount (with InvalidHandle having its usual secondary meaning of "invalid identifier-that-is-equivalent-to-a-handle").
Comment on attachment 87679 [details] [review] Regression tests: use a simulated CM instead of just holding the bus name Review of attachment 87679 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 87680 [details] [review] Don't give storage plugins the full parameter set, just 'account' Review of attachment 87680 [details] [review]: ----------------------------------------------------------------- ++ ::: src/mcd-account-manager-sso.c @@ +971,5 @@ > g_hash_table_insert (params, g_strdup (MC_ACCOUNT_KEY), &value); > > + /* FIXME: We should call IdentifyAccount(params) really. But this > + * plugin probably doesn't even compile any more, so, whatever; > + * just use the "account name". */ Fine with me, we have a more recent plugin in Empathy anyway.
Comment on attachment 87681 [details] [review] Call IdentifyAccount to get new accounts' names Review of attachment 87681 [details] [review]: ----------------------------------------------------------------- Fine with me. ++
Comment on attachment 87682 [details] [review] McdCreateAccountData: correctly free the user-data Review of attachment 87682 [details] [review]: ----------------------------------------------------------------- ++
Fixed in git for 5.17.0, thanks
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.