This branch contains a data migration step, obviously. It will not be safe to downgrade past the first release with this branch merged. Next steps for this megabranch go something like this: * Reimplement parameter type-coercion in terms of tp_variant_convert(), (extending that function if necessary), possibly falling back to how we do it now if tp_variant_convert() fails. tp_variant_convert is reasonably liberal (comparable to tp_asv_get_*), but doesn't do questionable things that we do in MC, like string <-> integer, or really stupid things that we only do because of our GKeyFile history, like string <-> string list. * When we Get(Parameters), if the account has no untyped parameters, return the (typed) parameters with the types we know they have. * Migration step: when an account's CM becomes ready, if it has any untyped parameters, and its backend supports typed parameters, find out their types from the CM and re-save them. (We might need to do this per backend rather than in general, or have a special case to not migrate secrets out of gnome-keyring (where they are always untyped) into plain text, or something. I don't think secrets other than 'password' actually exist in real CMs, though?) * When we Get(Parameters), if the account has untyped parameters and the CM is not ready, make some sort of sensible assumption (for instance use the types from telepathy-spec and Haze, and assume that anything else is a string). If a parameter is stored in type A but the CM expects it in type B, we should try to convert it for the RequestConnection() call, but IMO we should *not* convert the stored value unless/until the user actually calls UpdateParameters. Created attachment 88294 [details] [review] Rename default account-store backend from default to keyfile If we're going to change the default, leaving it called 'default' makes little sense! Created attachment 88295 [details] [review] McdAccountManagerDefault: segregate attributes and parameters Instead of lumping everything into a GKeyFile, store attributes and parameters separately. Created attachment 88296 [details] [review] Add an account-store backend for files containing a stringified GVariant Created attachment 88297 [details] [review] Default account backend: store each account as a stringified GVariant in a file Created attachment 88298 [details] [review] Test some new corner cases in the default account storage backend Now that we can load individual accounts from any of several directories, we should test that precedence and deletion behave correctly. --- Created attachment 88299 [details] [review] Default accounts backend: store attributes in a properly typed form Created attachment 88300 [details] [review] Default account storage backend: store parameters with types if possible Comment on attachment 88294 [details] [review] Rename default account-store backend from default to keyfile Review of attachment 88294 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88295 [details] [review] McdAccountManagerDefault: segregate attributes and parameters Review of attachment 88295 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88296 [details] [review] Add an account-store backend for files containing a stringified GVariant Review of attachment 88296 [details] [review]: ----------------------------------------------------------------- ++ ::: tests/account-store-variant-file.c @@ +1,5 @@ > +/* > + * MC account storage inspector: MC 5.14 GVariant-file backend > + * > + * Copyright © 2010 Nokia Corporation > + * Copyright © 2010-2012 Collabora Ltd. Update the copyright? Comment on attachment 88297 [details] [review] Default account backend: store each account as a stringified GVariant in a file Review of attachment 88297 [details] [review]: ----------------------------------------------------------------- one minor comment ::: src/mcd-account-manager-default.c @@ +422,5 @@ > + for (iter = g_get_system_data_dirs (); > + iter != NULL && *iter != NULL; > + iter++) > + { > + gchar *other = account_file_in (*iter, account_name); 'other' is leaked. Comment on attachment 88298 [details] [review] Test some new corner cases in the default account storage backend Review of attachment 88298 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88299 [details] [review] Default accounts backend: store attributes in a properly typed form Review of attachment 88299 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 88300 [details] [review] Default account storage backend: store parameters with types if possible Review of attachment 88300 [details] [review]: ----------------------------------------------------------------- ++ This branch also needs Attachment #88303 [details], which is on Bug #54874. Created attachment 88305 [details] [review] fixup! Default account backend: store each account as a stringified GVariant in a file Don't leak @other. To be squashed into Attachment #88297 [details] (In reply to comment #12) > > + * Copyright © 2010-2012 Collabora Ltd. > > Update the copyright? Sure; I wrote the vast majority of this in 2012, but I have touched the patches while rebasing. I made the obvious change; I'm not going to ask you to review that :-) Comment on attachment 88305 [details] [review] fixup! Default account backend: store each account as a stringified GVariant in a file Review of attachment 88305 [details] [review]: ----------------------------------------------------------------- ++ Fixed in git for 5.17.0. I opened Bug #71093 for the rest. |
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.
This is part 6 of my account storage megabranch, and the last part for now. It is a prerequisite for fixing this: /* FIXME: this is ridiculous. MC stores the parameters for the account, so * it should be able to expose them on D-Bus even if the CM is uninstalled. * It shouldn't need to iterate across the parameters supported by the CM. * But it does, because MC doesn't store the types of parameters. So it * needs the CM (or .manager file) to be around to tell it whether "true" * is a string or a boolean… which I have not actually done yet. I have not yet tested this without Bug #54874, but it could probably be rebased away. I have not yet tested this on live data (Empathy/Shell, rather than the regression tests) either.