Bug 54875

Summary: make the default account backend store GVariants on disk
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: mission-controlAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: jonny.lamb
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=variants-all-round-54875
Whiteboard: one-way-migration
i915 platform: i915 features:
Bug Depends on: 54780    
Bug Blocks:    
Attachments: Rename default account-store backend from default to keyfile
McdAccountManagerDefault: segregate attributes and parameters
Add an account-store backend for files containing a stringified GVariant
Default account backend: store each account as a stringified GVariant in a file
Test some new corner cases in the default account storage backend
Default accounts backend: store attributes in a properly typed form
Default account storage backend: store parameters with types if possible
fixup! Default account backend: store each account as a stringified GVariant in a file

Description Simon McVittie 2012-09-13 15:25:16 UTC
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.
Comment 1 Simon McVittie 2012-09-13 15:52:28 UTC
This branch contains a data migration step, obviously. It will not be safe to downgrade past the first release with this branch merged.
Comment 2 Simon McVittie 2012-11-08 11:32:55 UTC
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.
Comment 3 Simon McVittie 2013-10-29 14:22:40 UTC
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!
Comment 4 Simon McVittie 2013-10-29 14:24:30 UTC
Created attachment 88295 [details] [review]
McdAccountManagerDefault: segregate attributes and  parameters

Instead of lumping everything into a GKeyFile, store attributes and
parameters separately.
Comment 5 Simon McVittie 2013-10-29 14:25:32 UTC
Created attachment 88296 [details] [review]
Add an account-store backend for files containing a  stringified GVariant
Comment 6 Simon McVittie 2013-10-29 14:25:47 UTC
Created attachment 88297 [details] [review]
Default account backend: store each account as a  stringified GVariant in a file
Comment 7 Simon McVittie 2013-10-29 14:26:04 UTC
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.
---
Comment 8 Simon McVittie 2013-10-29 14:26:21 UTC
Created attachment 88299 [details] [review]
Default accounts backend: store attributes in a  properly typed form
Comment 9 Simon McVittie 2013-10-29 14:26:38 UTC
Created attachment 88300 [details] [review]
Default account storage backend: store parameters with  types if possible
Comment 10 Guillaume Desmottes 2013-10-29 14:55:44 UTC
Comment on attachment 88294 [details] [review]
Rename default account-store backend from default to  keyfile

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

++
Comment 11 Guillaume Desmottes 2013-10-29 15:01:47 UTC
Comment on attachment 88295 [details] [review]
McdAccountManagerDefault: segregate attributes and  parameters

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

++
Comment 12 Guillaume Desmottes 2013-10-29 15:03:50 UTC
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 13 Guillaume Desmottes 2013-10-29 15:10:28 UTC
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 14 Guillaume Desmottes 2013-10-29 15:16:02 UTC
Comment on attachment 88298 [details] [review]
Test some new corner cases in the default account  storage backend

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

++
Comment 15 Guillaume Desmottes 2013-10-29 15:17:06 UTC
Comment on attachment 88299 [details] [review]
Default accounts backend: store attributes in a  properly typed form

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

++
Comment 16 Guillaume Desmottes 2013-10-29 15:19:36 UTC
Comment on attachment 88300 [details] [review]
Default account storage backend: store parameters with  types if possible

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

++
Comment 17 Simon McVittie 2013-10-29 16:00:04 UTC
This branch also needs Attachment #88303 [details], which is on Bug #54874.
Comment 18 Simon McVittie 2013-10-29 16:10:12 UTC
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]
Comment 19 Simon McVittie 2013-10-29 16:11:26 UTC
(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 20 Guillaume Desmottes 2013-10-31 08:08:58 UTC
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]:
-----------------------------------------------------------------

++
Comment 21 Simon McVittie 2013-10-31 14:50:02 UTC
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.